feat(plugin-prometheus): Enable case-sensitive identifier support for Prometheus connector#26260
Conversation
Reviewer's GuideIntroduce an optional case-sensitive identifier matching mode for Prometheus connector by extending configuration, updating metadata resolution logic to normalize and compare table names based on the mode, and enriching tests to cover both default and case-sensitive behaviors. Class diagram for updated PrometheusConnectorConfig and PrometheusMetadataclassDiagram
class PrometheusConnectorConfig {
URI prometheusURI
String trustStorePath
String truststorePassword
boolean verifyHostName
boolean caseSensitiveNameMatchingEnabled
+getPrometheusURI()
+setVerifyHostName(boolean)
+isCaseSensitiveNameMatchingEnabled()
+setCaseSensitiveNameMatchingEnabled(boolean)
}
class PrometheusMetadata {
PrometheusClient prometheusClient
boolean caseSensitiveNameMatchingEnabled
+normalizeIdentifier(ConnectorSession, String)
}
PrometheusMetadata --> PrometheusConnectorConfig : uses config
PrometheusConnectorConfig <|-- PrometheusConnectorConfig : config property added
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `presto-prometheus/src/test/java/com/facebook/presto/plugin/prometheus/TestPrometheusMixedCase.java:63-64` </location>
<code_context>
+ .setCaseSensitiveNameMatchingEnabled(false));
}
@Test
</code_context>
<issue_to_address>
**suggestion (testing):** Missing test for case-insensitive mode with mixed-case table names.
Please add a test for the default case-insensitive mode to confirm that queries with varying table name casing (e.g., 'UP', 'Up', 'up') all work as expected.
</issue_to_address>
### Comment 2
<location> `presto-prometheus/src/test/java/com/facebook/presto/plugin/prometheus/TestPrometheusRetrieveUpValueIntegrationTests.java:57` </location>
<code_context>
{
this.server = new PrometheusServer();
this.client = createPrometheusClient(server);
+ this.config = new PrometheusConnectorConfig();
}
</code_context>
<issue_to_address>
**suggestion (testing):** Missing explicit tests for case sensitivity in PrometheusMetadata unit tests.
Please add unit tests for PrometheusMetadata with caseSensitiveNameMatchingEnabled set to both true and false, ensuring correct table handle resolution and metadata retrieval for mixed-case table names.
Suggested implementation:
```java
private PrometheusServer server;
private PrometheusClient client;
private PrometheusConnectorConfig config;
@BeforeClass
protected void createQueryRunner()
{
this.server = new PrometheusServer();
this.client = createPrometheusClient(server);
this.config = new PrometheusConnectorConfig();
}
@Test
public void testCaseSensitiveTableHandleResolution()
{
PrometheusConnectorConfig caseSensitiveConfig = new PrometheusConnectorConfig(true); // true for case sensitivity
PrometheusMetadata metadata = new PrometheusMetadata(client, caseSensitiveConfig);
// Assume "MixedCaseTable" exists in Prometheus
SchemaTableName mixedCaseTable = new SchemaTableName("default", "MixedCaseTable");
ConnectorTableHandle handle = metadata.getTableHandle(mixedCaseTable);
assertNotNull(handle, "Table handle should be resolved for exact case match");
// Try with different case
SchemaTableName lowerCaseTable = new SchemaTableName("default", "mixedcasetable");
ConnectorTableHandle lowerHandle = metadata.getTableHandle(lowerCaseTable);
assertNull(lowerHandle, "Table handle should not be resolved for case mismatch when case sensitivity is enabled");
}
@Test
public void testCaseInsensitiveTableHandleResolution()
{
PrometheusConnectorConfig caseInsensitiveConfig = new PrometheusConnectorConfig(false); // false for case insensitivity
PrometheusMetadata metadata = new PrometheusMetadata(client, caseInsensitiveConfig);
// Assume "MixedCaseTable" exists in Prometheus
SchemaTableName mixedCaseTable = new SchemaTableName("default", "MixedCaseTable");
ConnectorTableHandle handle = metadata.getTableHandle(mixedCaseTable);
assertNotNull(handle, "Table handle should be resolved for exact case match");
// Try with different case
SchemaTableName lowerCaseTable = new SchemaTableName("default", "mixedcasetable");
ConnectorTableHandle lowerHandle = metadata.getTableHandle(lowerCaseTable);
assertNotNull(lowerHandle, "Table handle should be resolved for case mismatch when case sensitivity is disabled");
}
```
- Ensure that `PrometheusConnectorConfig` has a constructor that accepts a boolean for `caseSensitiveNameMatchingEnabled`.
- Make sure that the Prometheus test server contains a table named "MixedCaseTable" for these tests to pass.
- If `PrometheusMetadata` or `PrometheusConnectorConfig` do not support these options, you will need to add them.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
b5d3322 to
ef3ff6c
Compare
steveburnett
left a comment
There was a problem hiding this comment.
Thanks for the doc! Only a few minor comments, overall it looks good.
|
Thanks for the release note! Nits: |
5f71407 to
147b24e
Compare
Thanks for the review! I’ve addressed all the comments. |
ba85ae8 to
6ccf6bf
Compare
steveburnett
left a comment
There was a problem hiding this comment.
LGTM! (docs)
Pull updated branch, new local doc build, looks good. Thanks!
6ccf6bf to
090d572
Compare
090d572 to
226f22a
Compare
|
@faizdani |
cc21463 to
4b005e7
Compare
4b005e7 to
85cde9f
Compare
|
Assigning this issue to @faizdani because you are the default assignee for issue follow-up scheme |
|
@faizdani |
|
Assigning this issue to @faizdani because you are the default assignee for issue follow-up scheme |
|
@faizdani |
|
Assigning this issue to @faizdani because you are the default assignee for issue follow-up scheme |
|
@faizdani |
|
Assigning this issue to @faizdani because you are the default assignee for issue follow-up scheme |
|
@faizdani |
|
Assigning this issue to @faizdani because you are the default assignee for issue follow-up scheme |
|
@faizdani |
agrawalreetika
left a comment
There was a problem hiding this comment.
Thanks for the PR. Let few comments.
Also, please correct typo in the commit message,
85cde9f to
ef6e26d
Compare
|
Assigning this issue to @faizdani because you are the default assignee for issue follow-up scheme |
|
@faizdani |
… Prometheus connector (prestodb#26260) ## Description Added catalog-level config to support case-sensitive-name-matching This PR implements case sensitivity handling for Prometheus metric (table) names: 1. Added case-sensitive name matching support controlled by the case-sensitive-name-matching configuration property 2. Implemented normalizeIdentifier in PrometheusMetadata to respect case sensitivity settings 3. Modified PrometheusClient to preserve case when case-sensitive matching is enabled When case-sensitive-name-matching is set to true: 1. Metric (table) names will preserve their original case 2. Queries must use the exact case as defined in Prometheus When case-sensitive-name-matching is set to false (default for backward compatibility): 1. All names are normalized to lowercase 2. Queries are case-insensitive **Note:** https://github.com/prestodb/presto/blob/76d8bf2f42476c36e429e7515797f452ca79aef3/presto-prometheus/src/main/java/com/facebook/presto/plugin/prometheus/PrometheusClient.java#L148 https://github.com/prestodb/presto/blob/76d8bf2f42476c36e429e7515797f452ca79aef3/presto-prometheus/src/main/java/com/facebook/presto/plugin/prometheus/PrometheusClient.java#L151 These three columns (labels, timestamp, value) are the fixed for all Prometheus metric tables. They represent: _labels_: Map of label key-value pairs from Prometheus metrics _timestamp_: Time when the metric was recorded _value_: The numeric value of the metric So the reason we have case-sensitive identifier support is extended to only tables but not to columns. ## Motivation and Context <!---Why is this change required? What problem does it solve?--> <!---If it fixes an open issue, please link to the issue here.--> ## Impact <!---Describe any public API or user-facing feature change or any performance impact--> ## Test Plan <!---Please fill in how you tested your change--> ## Contributor checklist - [ ] Please make sure your submission complies with our [contributing guide](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md), in particular [code style](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#code-style) and [commit standards](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#commit-standards). - [ ] PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced. - [ ] Documented new properties (with its default value), SQL syntax, functions, or other functionality. - [ ] If release notes are required, they follow the [release notes guidelines](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines). - [ ] Adequate tests were added if applicable. - [ ] CI passed. ## Release Notes Please follow [release notes guidelines](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines) and fill in the release notes below. ``` == RELEASE NOTES == Prometheus Connector Changes * Add mixed case-sensitive identifier support for Prometheus connector.
Description
Added catalog-level config to support case-sensitive-name-matching
This PR implements case sensitivity handling for Prometheus metric (table) names:
When case-sensitive-name-matching is set to true:
When case-sensitive-name-matching is set to false (default for backward compatibility):
Note:
presto/presto-prometheus/src/main/java/com/facebook/presto/plugin/prometheus/PrometheusClient.java
Line 148 in 76d8bf2
presto/presto-prometheus/src/main/java/com/facebook/presto/plugin/prometheus/PrometheusClient.java
Line 151 in 76d8bf2
These three columns (labels, timestamp, value) are the fixed for all Prometheus metric tables. They represent:
labels: Map of label key-value pairs from Prometheus metrics
timestamp: Time when the metric was recorded
value: The numeric value of the metric
So the reason we have case-sensitive identifier support is extended to only tables but not to columns.
Motivation and Context
Impact
Test Plan
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.