feat: implement more expr_to_sql functionality #9578

devinjdangelo · 2024-03-12T13:02:43Z

Which issue does this PR close?

related to #9495

Rationale for this change

Implements additional Expr -> ast::Expr conversions

What changes are included in this PR?

Implement cast expressions
Implement arrow -> ast::DataType conversion
Implement Aggregate expression conversions (for built in aggregate functions)
Implement Date32 and Date64 scalar conversions
In support of Date scalar conversions, datafusion scalars now map to SQL expressions
Binary Exprs now always wrapped in ast::Expr::Nested to preserve order of operations encoded in LogicalPlan

Are these changes tested?

Yes

Are there any user-facing changes?

More supported conversions

devinjdangelo · 2024-03-12T13:42:46Z

datafusion/sql/Cargo.toml

@@ -39,6 +39,7 @@ unicode_expressions = []
 [dependencies]
 arrow = { workspace = true }
 arrow-schema = { workspace = true }
+chrono = { workspace = true }


I know we are sensitive to new dependencies so wanted to flag this proposed addition to datafusion-sql @alamb ...

It is used in this PR to facilitate converting arrow Date types to a formatted string which can be placed in a SQL expression like CAST('2024-01-01' to DATE)

It may be a good solution to put the unparser behind a feature flag and then any unparser specific dependencies can only be enabled if one needs unparser support.

I think arrow already depends on chrono so this change doesn't add any new actual dependency (just an explicit one)

I went ahead and put unparser behind a (default) feature flag anyway. Could come in handy if we need a more invasive dependency later or someone just does not want this functionality.

alamb

Thank you @devinjdangelo

I actually took a stab at writing tests for this functionality (the roundtrip tests) and I got hung up on the lack of #8736

I feel like with just a little plumbing that could work and then writing tests for this feature would be easier.

I just need to find about 10 more hours in each day 😆

alamb · 2024-03-13T00:14:58Z

datafusion/sql/src/unparser/expr.rs

-            ScalarValue::Date32(None) => Ok(ast::Value::Null),
-            ScalarValue::Date64(Some(_d)) => not_impl_err!("Unsupported scalar: {v:?}"),
-            ScalarValue::Date64(None) => Ok(ast::Value::Null),
+            ScalarValue::Date32(Some(d)) => {


I wonder if there is some function we can use in arrow-rs do to this conversion

Perhaps https://docs.rs/arrow/latest/arrow/array/types/struct.Date32Type.html#method.to_naive_date ?

Good call. I was able to remove the chrono explicit dependency and downcast to arrow arrays and use those methods instead.

devinjdangelo · 2024-03-13T01:12:55Z

I actually took a stab at writing tests for this functionality (the roundtrip tests) and I got hung up on the lack of #8736

Same here. I can take a more dedicated crack at round-trip tests in a follow on PR. I wrote tests following the existing pattern for now, which was not too difficult either.

I just need to find about 10 more hours in each day 😆

☝️ this

alamb

Looks really nice to me -- thank you @devinjdangelo

alamb · 2024-03-13T15:23:50Z

datafusion/sql/Cargo.toml

@@ -33,11 +33,13 @@ name = "datafusion_sql"
 path = "src/lib.rs"

 [features]
-default = ["unicode_expressions"]
+default = ["unicode_expressions", "unparser"]


alamb · 2024-03-13T15:24:19Z

datafusion/sql/src/unparser/expr.rs

@@ -315,14 +489,78 @@ mod tests {
    #[test]
    fn expr_to_sql_ok() -> Result<()> {
        let tests: Vec<(Expr, &str)> = vec![
-            (col("a").gt(lit(4)), r#"a > 4"#),
+            ((col("a") + col("b")).gt(lit(4)), r#"((a + b) > 4)"#),


alamb · 2024-03-13T15:25:07Z

datafusion/sql/src/unparser/expr.rs

@@ -347,7 +585,7 @@ mod tests {

        let actual = format!("{}", ast);

-        let expected = r#"'a' > 4"#;
+        let expected = r#"('a' > 4)"#;


these parens are due to using Expr::Nested right? I think it is a good design to add explicit parens to ensure the evaluation order is correctly reflected

alamb · 2024-03-13T16:01:50Z

Perhaps we can also add a note in https://github.com/apache/arrow-datafusion?tab=readme-ov-file#crate-features about the unparser feature

devinjdangelo · 2024-03-13T23:23:50Z

datafusion/sql/src/unparser/expr.rs

+                r#"CAST(a AS INTEGER UNSIGNED)"#,
+            ),
+            (
+                Expr::Literal(ScalarValue::Date64(Some(0))),


Since it is not possible to directly represent a Date scalar in a SQL string, these non round trip tests are still needed.

alamb

Thanks again @devinjdangelo

alamb · 2024-03-14T10:09:58Z

The windows CI failure seem unrelated

devinjdangelo added 2 commits March 12, 2024 08:54

more impls

2ea1c93

fix tests

05f1a71

github-actions bot added the sql SQL Planner label Mar 12, 2024

devinjdangelo mentioned this pull request Mar 12, 2024

Basic support for converting Expr to SQL string #9495

Closed

5 tasks

devinjdangelo added 2 commits March 12, 2024 09:34

cargo update dfcli

aedb46d

fix custom_dialect test

15f9a05

devinjdangelo commented Mar 12, 2024

View reviewed changes

alamb reviewed Mar 13, 2024

View reviewed changes

devinjdangelo added 4 commits March 12, 2024 20:25

add tests and feature flag

48c33f5

fix comment

b18ae76

remove chrono use arrow-array conversions

b8bcd27

fix cargo lock again

937e3c8

devinjdangelo changed the title ~~wip feat: implement more expr_to_sql functionality~~ feat: implement more expr_to_sql functionality Mar 13, 2024

devinjdangelo marked this pull request as ready for review March 13, 2024 01:14

devinjdangelo added 3 commits March 12, 2024 21:17

fix count distinct

4b7d5f5

retry windows ci

0cda50c

retry windows ci again

a7e4be6

alamb approved these changes Mar 13, 2024

View reviewed changes

alamb mentioned this pull request Mar 13, 2024

Initial support LogicalPlan to SQL String #9596

Merged

devinjdangelo added 2 commits March 13, 2024 19:05

Merge branch 'main' into more_unparser_impls

7228490

add roundtrip tests

a2e8b61

github-actions bot added the documentation Improvements or additions to documentation label Mar 13, 2024

devinjdangelo commented Mar 13, 2024

View reviewed changes

cargo fmt

9564e8c

alamb approved these changes Mar 14, 2024

View reviewed changes

alamb merged commit 5911d18 into apache:main Mar 14, 2024
23 of 24 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement more expr_to_sql functionality #9578

feat: implement more expr_to_sql functionality #9578

devinjdangelo commented Mar 12, 2024 •

edited

Loading

devinjdangelo Mar 12, 2024

devinjdangelo Mar 12, 2024

alamb Mar 13, 2024

devinjdangelo Mar 13, 2024

alamb left a comment

alamb Mar 13, 2024

devinjdangelo Mar 13, 2024

devinjdangelo commented Mar 13, 2024

alamb left a comment

alamb Mar 13, 2024

alamb Mar 13, 2024

alamb Mar 13, 2024

alamb commented Mar 13, 2024

devinjdangelo Mar 13, 2024

alamb left a comment

alamb commented Mar 14, 2024

feat: implement more expr_to_sql functionality #9578

feat: implement more expr_to_sql functionality #9578

Conversation

devinjdangelo commented Mar 12, 2024 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devinjdangelo commented Mar 13, 2024

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Mar 13, 2024

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb commented Mar 14, 2024

devinjdangelo commented Mar 12, 2024 •

edited

Loading