Add option to control whether to normalize ident #5124
Conversation
github-actions
bot
added
core
There was a problem hiding this comment.
The idea and code look good to me @jiacai2050 -- thank you!
I think we need two more tests to ensure this feature is not broken in the future:
- A test for parse_float_as_decimal
- a "sql level" test for this feature -- perhaps in https://github.com/apache/arrow-datafusion/blob/master/datafusion/core/tests/sqllogictests/test_files/ddl.slt ?
Here is an example of changing a setting in sql: https://github.com/apache/arrow-datafusion/blob/1f7885bb48dd33ce7b9df995214393bbff080e08/datafusion/core/tests/sqllogictests/test_files/information_schema.slt#L28-L30
|
@alamb So I only add sqllogictests. |
| let table_ref = object_name_to_table_reference( | ||
| name, | ||
| self.options.enable_ident_normalization, | ||
| )?; |
There was a problem hiding this comment.
This code could potentially be improved by making a self.object_name_to_table_reference rather than having to pass self.options.enable_ident_normalization, at each callsite, but we can do that as a follow on PR
There was a problem hiding this comment.
Make sense, I can fix this in following PR.
| query R | ||
| select 10000000000000000000.01 | ||
| ---- | ||
| 10000000000000000000.01 |
There was a problem hiding this comment.
Another query you can do is select arrow_typeof(10000000000000000000.01) to see the actual type of the expression
There was a problem hiding this comment.
Don't know this function before, will add this test soon.
|
Thanks again @jiacai2050 |
|
Benchmark runs are scheduled for baseline = 11e8906 and contender = f8607c2. f8607c2 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Closes #4551.
Rationale for this change
See issue above.
What changes are included in this PR?
enable_ident_normalizationin ParserOptionsAre these changes tested?
Add new UT: parse_ident_normalization
Are there any user-facing changes?
Yes, users can configure this new option when parse SQL