-
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
msqe-case-insensitive-enabled #14830
msqe-case-insensitive-enabled #14830
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #14830 +/- ##
============================================
+ Coverage 61.75% 63.78% +2.03%
- Complexity 207 1472 +1265
============================================
Files 2436 2708 +272
Lines 133233 151423 +18190
Branches 20636 23376 +2740
============================================
+ Hits 82274 96585 +14311
- Misses 44911 47591 +2680
- Partials 6048 7247 +1199
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
pinot-query-planner/src/main/java/org/apache/pinot/query/catalog/PinotCatalog.java
Outdated
Show resolved
Hide resolved
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 for the patch @albertobastos! I've left a few minor comments and questions.
...-tests/src/test/java/org/apache/pinot/integration/tests/MultiStageEngineIntegrationTest.java
Show resolved
Hide resolved
pinot-query-planner/src/main/java/org/apache/pinot/query/QueryEnvironment.java
Show resolved
Hide resolved
pinot-query-planner/src/main/java/org/apache/pinot/query/catalog/PinotCatalog.java
Outdated
Show resolved
Hide resolved
...on-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java
Show resolved
Hide resolved
...on-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java
Outdated
Show resolved
Hide resolved
...on-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java
Show resolved
Hide resolved
...-tests/src/test/java/org/apache/pinot/integration/tests/MultiStageEngineIntegrationTest.java
Show resolved
Hide resolved
...on-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java
Show resolved
Hide resolved
...on-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java
Show resolved
Hide resolved
pinot-query-planner/src/main/java/org/apache/pinot/query/QueryEnvironment.java
Show resolved
Hide resolved
pinot-query-planner/src/main/java/org/apache/pinot/query/catalog/PinotCatalog.java
Outdated
Show resolved
Hide resolved
pinot-query-planner/src/main/java/org/apache/pinot/query/catalog/PinotCatalog.java
Outdated
Show resolved
Hide resolved
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.
Next comments batch ready @yashmayya , thanks for looking into it!
...on-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java
Show resolved
Hide resolved
pinot-query-planner/src/main/java/org/apache/pinot/query/catalog/PinotCatalog.java
Outdated
Show resolved
Hide resolved
pinot-query-planner/src/main/java/org/apache/pinot/query/catalog/PinotCatalog.java
Outdated
Show resolved
Hide resolved
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 @albertobastos!
closes #14675
Backwards compatibility:
When
enable.case.insensitive=true
(the default value) responses using MSQE will return the contents oftablesQueried
always in lowercase. That's due to how internally Calcite is used for column validation and how it handles a case-insensitive scenario. Given that it happens when a cluster is configured as case-insensitive, hopefully the consumer will also be prepared to ignore case when using that response field.