diff --git a/rust/cubesql/cubesql/src/compile/rewrite/rules/filters.rs b/rust/cubesql/cubesql/src/compile/rewrite/rules/filters.rs index bac53c2026a93..39774ce7458ca 100644 --- a/rust/cubesql/cubesql/src/compile/rewrite/rules/filters.rs +++ b/rust/cubesql/cubesql/src/compile/rewrite/rules/filters.rs @@ -2807,7 +2807,7 @@ impl FilterRules { ScalarValue::TimestampNanosecond(_, _) | ScalarValue::Date32(_) | ScalarValue::Date64(_) => { - if let Some(timestamp) = + if let Ok(Some(timestamp)) = Self::scalar_to_native_datetime(&literal) { let value = format_iso_timestamp(timestamp); @@ -2842,7 +2842,10 @@ impl FilterRules { continue; } } - x => panic!("Unsupported filter scalar: {:?}", x), + x => { + log::trace!("Unsupported filter scalar: {x:?}"); + continue; + } }; subst.insert( @@ -3442,6 +3445,7 @@ impl FilterRules { } // Transform ?expr IN (?literal) to ?expr = ?literal + // TODO it's incorrect: inner expr can be null, or can be non-literal (and domain in not clear) fn transform_filter_in_to_equal( &self, negated_var: &'static str, @@ -3501,7 +3505,10 @@ impl FilterRules { let values = list .into_iter() .map(|literal| FilterRules::scalar_to_value(literal)) - .collect::>(); + .collect::, _>>(); + let Ok(values) = values else { + return false; + }; if let Some((member_name, cube)) = Self::filter_member_name( egraph, @@ -3552,8 +3559,8 @@ impl FilterRules { } } - fn scalar_to_value(literal: &ScalarValue) -> String { - match literal { + fn scalar_to_value(literal: &ScalarValue) -> Result { + Ok(match literal { ScalarValue::Utf8(Some(value)) => value.to_string(), ScalarValue::Int64(Some(value)) => value.to_string(), ScalarValue::Boolean(Some(value)) => value.to_string(), @@ -3564,18 +3571,24 @@ impl FilterRules { ScalarValue::TimestampNanosecond(_, _) | ScalarValue::Date32(_) | ScalarValue::Date64(_) => { - if let Some(timestamp) = Self::scalar_to_native_datetime(literal) { - return format_iso_timestamp(timestamp); + if let Some(timestamp) = Self::scalar_to_native_datetime(literal)? { + format_iso_timestamp(timestamp) + } else { + log::trace!("Unsupported filter scalar: {literal:?}"); + return Err("Unsupported filter scalar"); } - - panic!("Unsupported filter scalar: {:?}", literal); } - x => panic!("Unsupported filter scalar: {:?}", x), - } + x => { + log::trace!("Unsupported filter scalar: {x:?}"); + return Err("Unsupported filter scalar"); + } + }) } - fn scalar_to_native_datetime(literal: &ScalarValue) -> Option { - match literal { + fn scalar_to_native_datetime( + literal: &ScalarValue, + ) -> Result, &'static str> { + Ok(match literal { ScalarValue::TimestampNanosecond(_, _) | ScalarValue::Date32(_) | ScalarValue::Date64(_) => { @@ -3589,13 +3602,17 @@ impl FilterRules { } else if let Some(array) = array.as_any().downcast_ref::() { array.value_as_datetime(0) } else { - panic!("Unexpected array type: {:?}", array.data_type()) + log::trace!("Unexpected array type: {:?}", array.data_type()); + return Err("Unexpected array type"); }; timestamp } - _ => panic!("Unsupported filter scalar: {:?}", literal), - } + x => { + log::trace!("Unsupported filter scalar: {x:?}"); + return Err("Unsupported filter scalar"); + } + }) } fn transform_is_null( @@ -3865,10 +3882,15 @@ impl FilterRules { Some(MemberType::Time) => (), _ => continue, } - let values = vec![ - FilterRules::scalar_to_value(&low), - FilterRules::scalar_to_value(&high), - ]; + + let Ok(low) = FilterRules::scalar_to_value(&low) else { + return false; + }; + let Ok(high) = FilterRules::scalar_to_value(&high) else { + return false; + }; + + let values = vec![low, high]; subst.insert( filter_member_var, diff --git a/rust/cubesql/cubesql/src/compile/test/test_filters.rs b/rust/cubesql/cubesql/src/compile/test/test_filters.rs index 096cabbb282aa..cfb5cea2a54c0 100644 --- a/rust/cubesql/cubesql/src/compile/test/test_filters.rs +++ b/rust/cubesql/cubesql/src/compile/test/test_filters.rs @@ -1,4 +1,5 @@ use cubeclient::models::{V1LoadRequestQuery, V1LoadRequestQueryFilterItem}; +use datafusion::physical_plan::displayable; use pretty_assertions::assert_eq; use crate::compile::{ @@ -60,3 +61,75 @@ GROUP BY } ); } + +#[tokio::test] +async fn test_filter_dim_in_null() { + if !Rewriter::sql_push_down_enabled() { + return; + } + init_testing_logger(); + + let query_plan = convert_select_to_query_plan( + // language=PostgreSQL + r#" + SELECT + dim_str0 + FROM + MultiTypeCube + WHERE dim_str1 IN (NULL) + "# + .to_string(), + DatabaseProtocol::PostgreSQL, + ) + .await; + + let physical_plan = query_plan.as_physical_plan().await.unwrap(); + println!( + "Physical plan: {}", + displayable(physical_plan.as_ref()).indent() + ); + + // For now this tests only that query is rewritable + // TODO support this as "notSet" filter + + assert!(query_plan + .as_logical_plan() + .find_cube_scan_wrapped_sql() + .wrapped_sql + .sql + .contains(r#"\"expr\":\"${MultiTypeCube.dim_str1} IN (NULL)\""#)); +} + +#[tokio::test] +async fn test_filter_superset_is_null() { + if !Rewriter::sql_push_down_enabled() { + return; + } + init_testing_logger(); + + let query_plan = convert_select_to_query_plan( + // language=PostgreSQL + r#" +SELECT dim_str0 FROM MultiTypeCube WHERE (dim_str1 IS NULL OR dim_str1 IN (NULL) AND (1<>1)) + "# + .to_string(), + DatabaseProtocol::PostgreSQL, + ) + .await; + + let physical_plan = query_plan.as_physical_plan().await.unwrap(); + println!( + "Physical plan: {}", + displayable(physical_plan.as_ref()).indent() + ); + + // For now this tests only that query is rewritable + // TODO support this as "notSet" filter + + assert!(query_plan + .as_logical_plan() + .find_cube_scan_wrapped_sql() + .wrapped_sql + .sql + .contains(r#"\"expr\":\"(${MultiTypeCube.dim_str1} IS NULL OR (${MultiTypeCube.dim_str1} IN (NULL) AND FALSE))\""#)); +}