-
Notifications
You must be signed in to change notification settings - Fork 5.5k
feat(pinot): Enable case sensitivity support in the Pinot connector #26239
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
base: master
Are you sure you want to change the base?
Conversation
Reviewer's GuideIntroduce a new configuration flag to enable case-sensitive identifier matching in the Pinot connector, centralize name normalization through a normalizeIdentifier method, remove forced lowercase on column metadata names, and update tests and documentation accordingly. Class diagram for PinotConfig and PinotMetadata changesclassDiagram
class PinotConfig {
-String brokerAuthenticationPassword
-String queryOptions
-boolean caseSensitiveNameMatchingEnabled
+boolean isCaseSensitiveNameMatchingEnabled()
+PinotConfig setCaseSensitiveNameMatchingEnabled(boolean)
}
class PinotMetadata {
-String connectorId
-PinotConnection pinotPrestoConnection
-PinotConfig pinotConfig
+String normalizeIdentifier(ConnectorSession, String)
}
PinotMetadata --> PinotConfig: uses
Class diagram for PinotColumnMetadata changesclassDiagram
class PinotColumnMetadata {
+String getName()
+String getPinotName()
}
PinotColumnMetadata --|> ColumnMetadata
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey there - I've reviewed your changes - here's some feedback:
- The new case-sensitive config property isn’t namespaced under "pinot.", which is inconsistent with other Pinot connector settings—consider renaming it to "pinot.case-sensitive-name-matching".
- The documentation in pinot.rst needs to be updated to include the new case-sensitive-name-matching configuration and its default behavior.
- Make sure normalizeIdentifier is applied consistently for schema, table, and column names (not just column handles) when case-sensitive matching is enabled.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new case-sensitive config property isn’t namespaced under "pinot.", which is inconsistent with other Pinot connector settings—consider renaming it to "pinot.case-sensitive-name-matching".
- The documentation in pinot.rst needs to be updated to include the new case-sensitive-name-matching configuration and its default behavior.
- Make sure normalizeIdentifier is applied consistently for schema, table, and column names (not just column handles) when case-sensitive matching is enabled.
## Individual Comments
### Comment 1
<location> `presto-pinot-toolkit/src/main/java/com/facebook/presto/pinot/PinotConfig.java:117` </location>
<code_context>
private String brokerAuthenticationPassword;
private String queryOptions;
+ private boolean caseSensitiveNameMatchingEnabled;
@NotNull
</code_context>
<issue_to_address>
**nitpick:** Default value for caseSensitiveNameMatchingEnabled is not explicitly set.
Explicitly initialize the field or document its default value to prevent ambiguity and ensure consistent behavior.
</issue_to_address>
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
private String brokerAuthenticationPassword; | ||
|
||
private String queryOptions; | ||
private boolean caseSensitiveNameMatchingEnabled; |
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.
nitpick: Default value for caseSensitiveNameMatchingEnabled is not explicitly set.
Explicitly initialize the field or document its default value to prevent ambiguity and ensure consistent behavior.
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! (docs)
Pull branch, local doc build, looks good. Thank you!
Description
Enable case sensitivity support in the Pinot connector
Motivation and Context
Previously, the Pinot connector always converted schema, table, and column names to lowercase. This caused issues when working with identifiers that differ only by case.
This PR introduces a new catalog configuration,
case-sensitive-name-matching
, which enables exact case-sensitive matching of schema, table, and column names in the Pinot connector.Impact
Enable case sensitivity support in the Pinot connector
Test Plan
Currently, there is no Pinot image/Server usage in the existing Pinot tests, so I have attached test results from my Pinot instance.
Opened an issue to enhance Pinot test and add integration tests - #26235
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.