From bd1bf2a636d92747257baec9aacfcc0b2e040cf5 Mon Sep 17 00:00:00 2001 From: Qianqian <130200611+Sevenannn@users.noreply.github.com> Date: Tue, 12 Nov 2024 17:49:48 -0800 Subject: [PATCH 1/4] Skip casting to binary when inner expr is value (#60) * Skip casting to binary when inner expr is value * Update datafusion/sql/src/unparser/expr.rs Co-authored-by: Jack Eadie --------- Co-authored-by: Jack Eadie --- datafusion/sql/src/unparser/expr.rs | 65 ++++++++++++++++++++--------- 1 file changed, 46 insertions(+), 19 deletions(-) diff --git a/datafusion/sql/src/unparser/expr.rs b/datafusion/sql/src/unparser/expr.rs index 8f6ffa51f76a3..d475fca6ed938 100644 --- a/datafusion/sql/src/unparser/expr.rs +++ b/datafusion/sql/src/unparser/expr.rs @@ -180,25 +180,7 @@ impl Unparser<'_> { }) } Expr::Cast(Cast { expr, data_type }) => { - let inner_expr = self.expr_to_sql_inner(expr)?; - match data_type { - DataType::Dictionary(_, _) => match inner_expr { - // Dictionary values don't need to be cast to other types when rewritten back to sql - ast::Expr::Value(_) => Ok(inner_expr), - _ => Ok(ast::Expr::Cast { - kind: ast::CastKind::Cast, - expr: Box::new(inner_expr), - data_type: self.arrow_dtype_to_ast_dtype(data_type)?, - format: None, - }), - }, - _ => Ok(ast::Expr::Cast { - kind: ast::CastKind::Cast, - expr: Box::new(inner_expr), - data_type: self.arrow_dtype_to_ast_dtype(data_type)?, - format: None, - }), - } + Ok(self.cast_to_sql(expr, data_type)?) } Expr::Literal(value) => Ok(self.scalar_to_sql(value)?), Expr::Alias(Alias { expr, name: _, .. }) => self.expr_to_sql_inner(expr), @@ -865,6 +847,29 @@ impl Unparser<'_> { }) } + // Explicit type cast on ast::Expr::Value is not needed by underlying engine for certain types + // For example: CAST(Utf8("binary_value") AS Binary) and CAST(Utf8("dictionary_value") AS Dictionary) + fn cast_to_sql(&self, expr: &Box, data_type: &DataType) -> Result { + let inner_expr = self.expr_to_sql_inner(expr)?; + match inner_expr { + ast::Expr::Value(_) => match data_type { + DataType::Dictionary(_, _) | DataType::Binary => Ok(inner_expr), + _ => Ok(ast::Expr::Cast { + kind: ast::CastKind::Cast, + expr: Box::new(inner_expr), + data_type: self.arrow_dtype_to_ast_dtype(data_type)?, + format: None, + }), + }, + _ => Ok(ast::Expr::Cast { + kind: ast::CastKind::Cast, + expr: Box::new(inner_expr), + data_type: self.arrow_dtype_to_ast_dtype(data_type)?, + format: None, + }), + } + } + /// DataFusion ScalarValues sometimes require a ast::Expr to construct. /// For example ScalarValue::Date32(d) corresponds to the ast::Expr CAST('datestr' as DATE) fn scalar_to_sql(&self, v: &ScalarValue) -> Result { @@ -2167,6 +2172,28 @@ mod tests { } } + #[test] + fn test_cast_value_to_binary_expr() { + let tests = [( + Expr::Cast(Cast { + expr: Box::new(Expr::Literal(ScalarValue::Utf8(Some( + "blah".to_string(), + )))), + data_type: DataType::Binary, + }), + "'blah'", + )]; + for (value, expected) in tests { + let dialect = CustomDialectBuilder::new().build(); + let unparser = Unparser::new(&dialect); + + let ast = unparser.expr_to_sql(&value).expect("to be unparsed"); + let actual = format!("{ast}"); + + assert_eq!(actual, expected); + } + } + #[test] fn custom_dialect_use_char_for_utf8_cast() -> Result<()> { let default_dialect = CustomDialectBuilder::default().build(); From 4e7058f068f5af02ad467619c3772093f50485f6 Mon Sep 17 00:00:00 2001 From: Qianqian <130200611+Sevenannn@users.noreply.github.com> Date: Wed, 13 Nov 2024 14:55:53 -0800 Subject: [PATCH 2/4] Fix binary view cast (#63) --- datafusion/sql/src/unparser/expr.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/datafusion/sql/src/unparser/expr.rs b/datafusion/sql/src/unparser/expr.rs index d475fca6ed938..52a448726619e 100644 --- a/datafusion/sql/src/unparser/expr.rs +++ b/datafusion/sql/src/unparser/expr.rs @@ -853,7 +853,9 @@ impl Unparser<'_> { let inner_expr = self.expr_to_sql_inner(expr)?; match inner_expr { ast::Expr::Value(_) => match data_type { - DataType::Dictionary(_, _) | DataType::Binary => Ok(inner_expr), + DataType::Dictionary(_, _) | DataType::Binary | DataType::BinaryView => { + Ok(inner_expr) + } _ => Ok(ast::Expr::Cast { kind: ast::CastKind::Cast, expr: Box::new(inner_expr), @@ -2182,6 +2184,13 @@ mod tests { data_type: DataType::Binary, }), "'blah'", + Expr::Cast(Cast { + expr: Box::new(Expr::Literal(ScalarValue::Utf8(Some( + "blah".to_string(), + )))), + data_type: DataType::BinaryView, + }), + "'blah'", )]; for (value, expected) in tests { let dialect = CustomDialectBuilder::new().build(); From 25fd1288d50b41a7291c2d46532149b55851c63a Mon Sep 17 00:00:00 2001 From: Sevenannn Date: Thu, 14 Nov 2024 14:37:59 -0800 Subject: [PATCH 3/4] fix --- datafusion/sql/src/unparser/expr.rs | 36 ++++++++++++++++------------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/datafusion/sql/src/unparser/expr.rs b/datafusion/sql/src/unparser/expr.rs index 52a448726619e..cfc6b29ab0275 100644 --- a/datafusion/sql/src/unparser/expr.rs +++ b/datafusion/sql/src/unparser/expr.rs @@ -2176,22 +2176,26 @@ mod tests { #[test] fn test_cast_value_to_binary_expr() { - let tests = [( - Expr::Cast(Cast { - expr: Box::new(Expr::Literal(ScalarValue::Utf8(Some( - "blah".to_string(), - )))), - data_type: DataType::Binary, - }), - "'blah'", - Expr::Cast(Cast { - expr: Box::new(Expr::Literal(ScalarValue::Utf8(Some( - "blah".to_string(), - )))), - data_type: DataType::BinaryView, - }), - "'blah'", - )]; + let tests = [ + ( + Expr::Cast(Cast { + expr: Box::new(Expr::Literal(ScalarValue::Utf8(Some( + "blah".to_string(), + )))), + data_type: DataType::Binary, + }), + "'blah'", + ), + ( + Expr::Cast(Cast { + expr: Box::new(Expr::Literal(ScalarValue::Utf8(Some( + "blah".to_string(), + )))), + data_type: DataType::BinaryView, + }), + "'blah'", + ), + ]; for (value, expected) in tests { let dialect = CustomDialectBuilder::new().build(); let unparser = Unparser::new(&dialect); From bd0efa7bbda07e61acdb28f8166c5f4b3c4c6b56 Mon Sep 17 00:00:00 2001 From: Sevenannn Date: Thu, 14 Nov 2024 15:03:51 -0800 Subject: [PATCH 4/4] Fix clippy error --- datafusion/sql/src/unparser/expr.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/datafusion/sql/src/unparser/expr.rs b/datafusion/sql/src/unparser/expr.rs index cfc6b29ab0275..13554905e03be 100644 --- a/datafusion/sql/src/unparser/expr.rs +++ b/datafusion/sql/src/unparser/expr.rs @@ -849,7 +849,7 @@ impl Unparser<'_> { // Explicit type cast on ast::Expr::Value is not needed by underlying engine for certain types // For example: CAST(Utf8("binary_value") AS Binary) and CAST(Utf8("dictionary_value") AS Dictionary) - fn cast_to_sql(&self, expr: &Box, data_type: &DataType) -> Result { + fn cast_to_sql(&self, expr: &Expr, data_type: &DataType) -> Result { let inner_expr = self.expr_to_sql_inner(expr)?; match inner_expr { ast::Expr::Value(_) => match data_type {