Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Utf8View in Unparser expr_to_sql #13462

Merged
merged 6 commits into from
Nov 18, 2024

Conversation

phillipleblanc
Copy link
Contributor

Which issue does this PR close?

Closes #13461

Rationale for this change

Now that Utf8View is being returned by DataFusion, we need to ensure that when we encounter it as part of unparsing an expression, we support it properly.

What changes are included in this PR?

Correctly maps the Utf8View Arrow data type to the existing Utf8 type for unparsing.

Are these changes tested?

Yes

Are there any user-facing changes?

No API changes.

@github-actions github-actions bot added the sql SQL Planner label Nov 18, 2024
phillipleblanc and others added 2 commits November 18, 2024 20:54
Co-authored-by: Sherin Jacob <jacob@protoship.io>
@jcsherin
Copy link
Contributor

jcsherin commented Nov 18, 2024

Maybe it will be good to add a IS NULL or IS NOT NULL test as well.

        let expr = col("a").is_not_null();

        let ast = unparser.expr_to_sql(&expr)?;
        let actual = format!("{}", ast);

        let expected = r#"a IS NOT NULL"#.to_string();

I noticed that the unparser will generate an invalid a = NULL or a <> NULL if the user writes the following expression,

let expr = col("a").eq(ScalarValue::Utf8View(None));
let ast = unparser.expr_to_sql(&expr)?;

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

Not a problem as long as the user correctly uses expr.is_null() or expr.is_not_null().

@jcsherin
Copy link
Contributor

Thanks @phillipleblanc. LGTM 👍

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @phillipleblanc and @jcsherin -- this looks good to me

@alamb alamb merged commit adcf90f into apache:main Nov 18, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sql SQL Planner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Utf8View in the Unparser expr_to_sql method
3 participants