Planner: normalize_ident only when enable_ident_normalization is enabled #5785
Conversation
… to honor planner options
|
While working on this PR, I also noticed that there are places not related to the planner which also normalize names such as column. I wonder if it's valuable to also make these method configurable to enable ident normalization? |
I had some thoughts about this previously (not this specific function, but still applicable): #5183 (comment) Generally I'm not sure how exactly to be able to toggle normalization at that level (the common crate) without API changes to dependent functions, like |
datafusion/sql/src/planner.rs
Outdated
| } | ||
|
|
||
| fn normalize_ident(id: Ident, enable_normalization: bool) -> String { | ||
| pub fn normalize_ident(id: Ident, enable_normalization: bool) -> String { |
There was a problem hiding this comment.
What would you think about making crate::utils::normalize_ident pub instead? making something public that only does something if true is passed in seems somewhat strange to me
|
@ayushdg please let me know if you would to make changes to this PR or if we should merge it as is |
…r-ident-normalization
…ent to the struct implementation
There was a problem hiding this comment.
In cases like this with helper methods that get called from SqlToRel impl methods is there a better approach than adding a normalizer argument to each of these methods?
https://github.com/apache/arrow-datafusion/blob/c5e935a15f22d16d8ca244e68b128f7c5ff28375/datafusion/sql/src/statement.rs#L54-L65
There was a problem hiding this comment.
I can't think of any at the moment
There was a problem hiding this comment.
I can't think of any at the moment
|
Thanks @alamb let me know if anything else is needed, or if the pr is good to merge as is. |
…led (apache#5785) * Make planner::normalize_ident public and refactor planner impl method to honor planner options * Undo pre-commit config change * Add tests * Add identNormalizer struct to sqlToRel and move calls to normalize_ident to the struct implementation
3 participants
Which issue does this PR close?
Closes #5626
Rationale for this change
Some part of the codebase use
sqlToRel's options forenable_ident_normalizationto decide whether to normalize identifiers or not, but it isn't enforced consistently throughout the codebase.What changes are included in this PR?
Make planner:normalize_ident public.
All methods implementing
SqlToReluseenable_ident_normalizationwhile normalizing.Are these changes tested?
Are there any user-facing changes?
No.