-
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 a hint about normalization in error message (#14089) #14113
Add a hint about normalization in error message (#14089) #14113
Conversation
datafusion/common/src/error.rs
Outdated
@@ -167,6 +167,12 @@ impl Display for SchemaError { | |||
valid_fields, | |||
} => { | |||
write!(f, "No field named {}", field.quoted_flat_name())?; | |||
write!( | |||
f, | |||
". You can use double quotes to refer to the \"{}\" column", |
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.
its not only double quotes, its been 3 symbols at least.
Prob we can refer to case sensitivity instead, sort of
DataFusion column names are case sensitive. For case insensitive mode set datafusion.sql_parser.enable_ident_normalization option
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 think it is important to provide an alternate syntax for users of end systems who can not control the DataFusion settings. For example, for our InfluxDB users they can't set the configuration settings
I suspect the @TheBuilderJR 's users would likely not be able to change this setting
What about something like this (only in cases where the schema contains a column that matches except for case)
Column names are case sensitive. Use double quotes to refer to the "z" column
or set the datafusion.sql_parser.enable_ident_normalization configuration
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 @cj-zhukov and @comphead -- this is going to make DataFusion much easier to use
datafusion/common/src/error.rs
Outdated
@@ -167,6 +167,12 @@ impl Display for SchemaError { | |||
valid_fields, | |||
} => { | |||
write!(f, "No field named {}", field.quoted_flat_name())?; | |||
write!( | |||
f, | |||
". You can use double quotes to refer to the \"{}\" column", |
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 think it is important to provide an alternate syntax for users of end systems who can not control the DataFusion settings. For example, for our InfluxDB users they can't set the configuration settings
I suspect the @TheBuilderJR 's users would likely not be able to change this setting
What about something like this (only in cases where the schema contains a column that matches except for case)
Column names are case sensitive. Use double quotes to refer to the "z" column
or set the datafusion.sql_parser.enable_ident_normalization configuration
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.
lgtm thanks @cj-zhukov
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.
Thanks @cj-zhukov and @comphead -- this is a great improvement in my opinion
Which issue does this PR close?
Closes #14089.
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?