From 8274f5eeb2e8b7dcd22fe32e45b3d9dc05a75b53 Mon Sep 17 00:00:00 2001 From: Runji Wang Date: Tue, 26 Dec 2023 20:59:41 +0800 Subject: [PATCH 1/6] fix bool from_text Signed-off-by: Runji Wang --- src/common/src/array/data_chunk.rs | 2 -- src/common/src/types/mod.rs | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/src/common/src/array/data_chunk.rs b/src/common/src/array/data_chunk.rs index 90c2560cadcb2..a1ef272ec0eca 100644 --- a/src/common/src/array/data_chunk.rs +++ b/src/common/src/array/data_chunk.rs @@ -760,8 +760,6 @@ impl DataChunkTestExt for DataChunk { { let datum = match val_str { "." => None, - "t" => Some(true.into()), - "f" => Some(false.into()), "(empty)" => Some("".into()), _ => Some(ScalarImpl::from_text(val_str.as_bytes(), ty).unwrap()), }; diff --git a/src/common/src/types/mod.rs b/src/common/src/types/mod.rs index 7b6f393725e71..0262df6f56529 100644 --- a/src/common/src/types/mod.rs +++ b/src/common/src/types/mod.rs @@ -872,7 +872,7 @@ impl ScalarImpl { let res = match data_type { DataType::Varchar => Self::Utf8(str.to_string().into()), DataType::Boolean => { - Self::Bool(bool::from_str(str).map_err(|_| FromSqlError::from_text(str))?) + Self::Bool(str_to_bool(str).map_err(|_| FromSqlError::from_text(str))?) } DataType::Int16 => { Self::Int16(i16::from_str(str).map_err(|_| FromSqlError::from_text(str))?) From 2890b7dd358e1ba50acb5e8a1cca2c02c93078cc Mon Sep 17 00:00:00 2001 From: Runji Wang Date: Tue, 26 Dec 2023 21:11:51 +0800 Subject: [PATCH 2/6] reproduce the bug in unit test Signed-off-by: Runji Wang --- src/expr/core/src/expr/expr_some_all.rs | 64 +++++++++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/src/expr/core/src/expr/expr_some_all.rs b/src/expr/core/src/expr/expr_some_all.rs index 2cc84a0be79fd..2b7ad54244385 100644 --- a/src/expr/core/src/expr/expr_some_all.rs +++ b/src/expr/core/src/expr/expr_some_all.rs @@ -262,3 +262,67 @@ impl Build for SomeAllExpression { )) } } + +#[cfg(test)] +mod tests { + use risingwave_common::array::DataChunk; + use risingwave_common::row::Row; + use risingwave_common::test_prelude::DataChunkTestExt; + use risingwave_common::types::ToOwnedDatum; + use risingwave_common::util::iter_util::ZipEqDebug; + use risingwave_expr::expr::build_from_pretty; + + use super::*; + + #[tokio::test] + async fn test_some() { + let expr = SomeAllExpression::new( + build_from_pretty("0:int4"), + build_from_pretty("$0:boolean"), + Type::Some, + build_from_pretty("$1:boolean"), + ); + let (input, expected) = DataChunk::from_pretty( + "B[] B + {t,f} t + {f,t} t", + ) + .split_column_at(1); + + // test eval + let output = expr.eval(&input).await.unwrap(); + assert_eq!(&output, expected.column_at(0)); + + // test eval_row + for (row, expected) in input.rows().zip_eq_debug(expected.rows()) { + let result = expr.eval_row(&row.to_owned_row()).await.unwrap(); + assert_eq!(result, expected.datum_at(0).to_owned_datum()); + } + } + + #[tokio::test] + async fn test_all() { + let expr = SomeAllExpression::new( + build_from_pretty("0:int4"), + build_from_pretty("$0:boolean"), + Type::All, + build_from_pretty("$1:boolean"), + ); + let (input, expected) = DataChunk::from_pretty( + "B[] B + {f,f} f + {t} t", + ) + .split_column_at(1); + + // test eval + let output = expr.eval(&input).await.unwrap(); + assert_eq!(&output, expected.column_at(0)); + + // test eval_row + for (row, expected) in input.rows().zip_eq_debug(expected.rows()) { + let result = expr.eval_row(&row.to_owned_row()).await.unwrap(); + assert_eq!(result, expected.datum_at(0).to_owned_datum()); + } + } +} From 7f2814a0c34784492c9f9b3ab7b648dbba9e2788 Mon Sep 17 00:00:00 2001 From: Runji Wang Date: Tue, 26 Dec 2023 21:18:17 +0800 Subject: [PATCH 3/6] fix "{}" literal Signed-off-by: Runji Wang --- src/common/src/types/mod.rs | 3 +++ src/expr/core/src/expr/expr_some_all.rs | 2 ++ 2 files changed, 5 insertions(+) diff --git a/src/common/src/types/mod.rs b/src/common/src/types/mod.rs index 0262df6f56529..469825c35df76 100644 --- a/src/common/src/types/mod.rs +++ b/src/common/src/types/mod.rs @@ -929,6 +929,9 @@ impl ScalarImpl { } let mut builder = elem_type.create_array_builder(0); for s in str[1..str.len() - 1].split(',') { + if s.is_empty() { + continue; + } builder.append(Some(Self::from_text(s.trim().as_bytes(), elem_type)?)); } Self::List(ListValue::new(builder.finish())) diff --git a/src/expr/core/src/expr/expr_some_all.rs b/src/expr/core/src/expr/expr_some_all.rs index 2b7ad54244385..bea4aa9fea314 100644 --- a/src/expr/core/src/expr/expr_some_all.rs +++ b/src/expr/core/src/expr/expr_some_all.rs @@ -284,6 +284,7 @@ mod tests { ); let (input, expected) = DataChunk::from_pretty( "B[] B + {} f {t,f} t {f,t} t", ) @@ -310,6 +311,7 @@ mod tests { ); let (input, expected) = DataChunk::from_pretty( "B[] B + {} t {f,f} f {t} t", ) From 15242226170ef678f08b97c5810f1dd52bcfd8ca Mon Sep 17 00:00:00 2001 From: Runji Wang Date: Tue, 26 Dec 2023 22:25:44 +0800 Subject: [PATCH 4/6] fix the bug Signed-off-by: Runji Wang --- src/expr/core/src/expr/expr_some_all.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/expr/core/src/expr/expr_some_all.rs b/src/expr/core/src/expr/expr_some_all.rs index bea4aa9fea314..a507bf2fdd1e3 100644 --- a/src/expr/core/src/expr/expr_some_all.rs +++ b/src/expr/core/src/expr/expr_some_all.rs @@ -160,12 +160,17 @@ impl Expression for SomeAllExpression { ); let func_results = self.func.eval(&data_chunk).await?; - let mut func_results_iter = func_results.as_bool().iter(); + let bools = func_results.as_bool(); + let mut offset = 0; Ok(Arc::new( num_array .into_iter() .map(|num| match num { - Some(num) => self.resolve_bools(func_results_iter.by_ref().take(num)), + Some(num) => { + let range = offset..offset + num; + offset += num; + self.resolve_bools(range.map(|i| bools.value_at(i))) + } None => None, }) .collect::() From 1f3abe7de1192103d2b702c130874796eb17d73b Mon Sep 17 00:00:00 2001 From: Runji Wang Date: Tue, 26 Dec 2023 22:45:25 +0800 Subject: [PATCH 5/6] add more test cases Signed-off-by: Runji Wang --- src/common/src/types/mod.rs | 5 ++++- src/expr/core/src/expr/expr_some_all.rs | 24 ++++++++++++++++-------- 2 files changed, 20 insertions(+), 9 deletions(-) diff --git a/src/common/src/types/mod.rs b/src/common/src/types/mod.rs index 469825c35df76..d5b1d0705edb1 100644 --- a/src/common/src/types/mod.rs +++ b/src/common/src/types/mod.rs @@ -931,8 +931,11 @@ impl ScalarImpl { for s in str[1..str.len() - 1].split(',') { if s.is_empty() { continue; + } else if s.eq_ignore_ascii_case("null") { + builder.append_null(); + } else { + builder.append(Some(Self::from_text(s.trim().as_bytes(), elem_type)?)); } - builder.append(Some(Self::from_text(s.trim().as_bytes(), elem_type)?)); } Self::List(ListValue::new(builder.finish())) } diff --git a/src/expr/core/src/expr/expr_some_all.rs b/src/expr/core/src/expr/expr_some_all.rs index a507bf2fdd1e3..87f95dd896317 100644 --- a/src/expr/core/src/expr/expr_some_all.rs +++ b/src/expr/core/src/expr/expr_some_all.rs @@ -288,10 +288,14 @@ mod tests { build_from_pretty("$1:boolean"), ); let (input, expected) = DataChunk::from_pretty( - "B[] B - {} f - {t,f} t - {f,t} t", + "B[] B + . . + {} f + {NULL} . + {NULL,f} . + {NULL,t} t + {t,f} t + {f,t} t", // <- regression test for #14214 ) .split_column_at(1); @@ -315,10 +319,14 @@ mod tests { build_from_pretty("$1:boolean"), ); let (input, expected) = DataChunk::from_pretty( - "B[] B - {} t - {f,f} f - {t} t", + "B[] B + . . + {} t + {NULL} . + {NULL,t} . + {NULL,f} f + {f,f} f + {t} t", // <- regression test for #14214 ) .split_column_at(1); From 1426ccf7a0ba127379f5ec3910b44be6f6b3ca09 Mon Sep 17 00:00:00 2001 From: Runji Wang Date: Wed, 27 Dec 2023 13:49:10 +0800 Subject: [PATCH 6/6] add comment about the bug Signed-off-by: Runji Wang --- src/expr/core/src/expr/expr_some_all.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/expr/core/src/expr/expr_some_all.rs b/src/expr/core/src/expr/expr_some_all.rs index 87f95dd896317..8b7bd4c9667d7 100644 --- a/src/expr/core/src/expr/expr_some_all.rs +++ b/src/expr/core/src/expr/expr_some_all.rs @@ -49,6 +49,8 @@ impl SomeAllExpression { } } + // Notice that this function may not exhaust the iterator, + // so never pass an iterator created `by_ref`. fn resolve_bools(&self, bools: impl Iterator>) -> Option { match self.expr_type { Type::Some => {