-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Add support for Dictionary
to AST datatype in unparser
#14783
Conversation
Dictionary
to AST datatypeDictionary
to AST datatype in unparser
Thanks @cetra3 @phillipleblanc do you have any suggestions about where we could test this one? |
d2c76df
to
38c6e91
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @cetra3
Ideally we would write a test for this., but since I couldn't find any existing tests for data types, I don't think it is strictly necessary here
Maybe @phillipleblanc has some idea of how to write such a test
There are tests at the bottom of this file that can be added - see for example this PR I made for supporting Utf8View: #13462 |
DataType::Dictionary(_, _) => { | ||
not_impl_err!("Unsupported DataType: conversion: {data_type:?}") | ||
} | ||
DataType::Dictionary(_, val) => self.arrow_dtype_to_ast_dtype(val), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add a test for this new type, see this example for supporting the Utf8View type:
#[test]
fn test_utf8_view_to_sql() -> Result<()> {
let dialect = CustomDialectBuilder::new()
.with_utf8_cast_dtype(ast::DataType::Char(None))
.build();
let unparser = Unparser::new(&dialect);
let ast_dtype = unparser.arrow_dtype_to_ast_dtype(&DataType::Utf8View)?;
assert_eq!(ast_dtype, ast::DataType::Char(None));
let expr = cast(col("a"), DataType::Utf8View);
let ast = unparser.expr_to_sql(&expr)?;
let actual = format!("{}", ast);
let expected = r#"CAST(a AS CHAR)"#.to_string();
assert_eq!(actual, expected);
let expr = col("a").eq(lit(ScalarValue::Utf8View(Some("hello".to_string()))));
let ast = unparser.expr_to_sql(&expr)?;
let actual = format!("{}", ast);
let expected = r#"(a = 'hello')"#.to_string();
assert_eq!(actual, expected);
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();
assert_eq!(actual, expected);
let expr = col("a").is_null();
let ast = unparser.expr_to_sql(&expr)?;
let actual = format!("{}", ast);
let expected = r#"a IS NULL"#.to_string();
assert_eq!(actual, expected);
Ok(())
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a test for this. Let me know if I need to flesh it out more
4cf08c9
to
e496e52
Compare
datafusion/sql/src/unparser/expr.rs
Outdated
@@ -1586,7 +1586,7 @@ impl Unparser<'_> { | |||
not_impl_err!("Unsupported DataType: conversion: {data_type:?}") | |||
} | |||
DataType::Interval(_) => { | |||
not_impl_err!("Unsupported DataType: conversion: {data_type:?}") | |||
Ok(ast::DataType::Interval) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added this as well, it seems to be a bit of a trivial conversion so not sure a test is warranted
e496e52
to
b790c86
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @cetra3 and @phillipleblanc
Box::new(DataType::Utf8), | ||
))?; | ||
|
||
assert_eq!(ast_dtype, ast::DataType::Varchar(None)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
Which issue does this PR close?
No issue raised, just something we've seen in production
Rationale for this change
Ensures we can convert dictionary & interval types to ast datatypes
What changes are included in this PR?
Adds a small change to support dictionaries
Are these changes tested?
I am actually not too sure how best to test this, might need a bit of guidance here if we wanna add a test
Are there any user-facing changes?
No