Config: Add support default sql varchar to view types #15104
Conversation
github-actions
bot
added
sql
github-actions
bot
added
documentation
There was a problem hiding this comment.
Thanks @zhuqi-lucas ! This looks great. I have a few comments. Let me know what you think
datafusion/common/src/config.rs
Outdated
| /// If true, permit `VARCHAR` default convert to `Utf8View` in the logical plan. | ||
| /// If false, `VARCHAR` will be converted to `Utf8` in the logical plan. | ||
| /// Default is false. | ||
| pub support_varchar_to_view_types: bool, default = false |
There was a problem hiding this comment.
I recommend naming this slightly differently to make it clearer what it is doing
| /// If true, permit `VARCHAR` default convert to `Utf8View` in the logical plan. | |
| /// If false, `VARCHAR` will be converted to `Utf8` in the logical plan. | |
| /// Default is false. | |
| pub support_varchar_to_view_types: bool, default = false | |
| /// If true, `VARCHAR` is mapped to `Utf8View` during SQL planning. | |
| /// If false, `VARCHAR` is mappped to `Utf8` during SQL planning. | |
| /// Default is false. | |
| pub default_varchar_views: bool, default = false |
There was a problem hiding this comment.
Thank you @alamb for good suggestion, addressed in latest PR.
| @@ -5673,3 +5673,43 @@ async fn test_fill_null_all_columns() -> Result<()> { | |||
| assert_batches_sorted_eq!(expected, &results); | |||
| Ok(()) | |||
| } | |||
|
|
|||
| #[tokio::test] | |||
| async fn test_sql_support_sql_to_view_types() -> Result<()> { | |||
There was a problem hiding this comment.
Instead of adding rust based tests, could you please make these sqllogictest based instead? sqllogictest is easier to maintain and run
The instructions are here
https://github.com/apache/datafusion/tree/main/datafusion/sqllogictest
Perhaps as a new test in https://github.com/apache/datafusion/blob/main/datafusion/sqllogictest/test_files/ddl.slt
There was a problem hiding this comment.
Good suggestion! Addressed in latest PR.
Thank you @alamb for review, great suggestions. Addressed in latest PR. |
datafusion/common/src/config.rs
Outdated
| /// If true, `VARCHAR` is mapped to `Utf8View` during SQL planning. | ||
| /// If false, `VARCHAR` is mapped to `Utf8` during SQL planning. | ||
| /// Default is false. | ||
| pub default_varchar_views: bool, default = false |
There was a problem hiding this comment.
Could we define this parameter more clearly and readably, such as map_varchar_to_utf8view?
There was a problem hiding this comment.
Could we define this parameter more clearly and readably, such as
map_varchar_to_utf8view?
Thank you @Weijun-H for review and good suggestion! Addressed in latest PR.
|
Thanks @zhuqi-lucas and @alamb |
Which issue does this PR close?
This is the first step of our incremental work for this issue:
#15096
Rationale for this change
Config: Add support default sql varchar to view types
What changes are included in this PR?
Config: Add support default sql varchar to view types
Are these changes tested?
Yes
Are there any user-facing changes?
Support default sql varchar to utf8view