-
Notifications
You must be signed in to change notification settings - Fork 5.5k
feat: Add max-prefixes-count session property #25550
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
Conversation
5e0f8e6 to
2227c73
Compare
steveburnett
left a comment
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.
Please add documentation for the new configuration and session properties in this PR.
https://github.com/prestodb/presto/blob/master/presto-docs/src/main/sphinx/admin/properties.rst
2227c73 to
1bd5d27
Compare
Reviewer's GuideThis PR refactors the hard-coded max prefixes limit into a configurable session and server property, wiring it through FeaturesConfig and SystemSessionProperties, and updating InformationSchemaMetadata to respect the dynamic limit when calculating table prefixes. Tests have been adapted and extended to validate the new configuration end-to-end. Entity relationship diagram for session and config propertieserDiagram
FEATURES_CONFIG {
int maxPrefixesCount
}
SYSTEM_SESSION_PROPERTIES {
string MAX_PREFIXES_COUNT
}
SESSION {
int maxPrefixesCount
}
FEATURES_CONFIG ||--o| SYSTEM_SESSION_PROPERTIES : exposes
SYSTEM_SESSION_PROPERTIES ||--o| SESSION : sets
Class diagram for configurable max prefixes countclassDiagram
class FeaturesConfig {
- int maxPrefixesCount
+ setMaxPrefixesCount(Integer): FeaturesConfig
+ getMaxPrefixesCount(): int
}
class SystemSessionProperties {
+ MAX_PREFIXES_COUNT: String
+ getMaxPrefixesCount(Session): int
}
class InformationSchemaMetadata {
+ getTableLayoutForConstraint(...): ConnectorTableLayoutResult
}
FeaturesConfig <.. SystemSessionProperties : uses
SystemSessionProperties <.. InformationSchemaMetadata : uses
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 and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `presto-main-base/src/test/java/com/facebook/presto/metadata/TestInformationSchemaMetadata.java:163` </location>
<code_context>
+ .setCatalog(TEST_CATALOG)
.setSchema("information_schema")
.setTransactionId(transactionId)
+ .setSystemProperty(MAX_PREFIXES_COUNT, String.valueOf(maxPrefixesCount))
.build()
.toConnectorSession();
</code_context>
<issue_to_address>
**suggestion (testing):** Missing test for boundary and invalid values of maxPrefixesCount session property.
Add tests for maxPrefixesCount at its minimum (1), a large value, and invalid values (0, negative) to verify correct handling and enforcement of the @Min(1) constraint.
Suggested implementation:
```java
private ConnectorSession createNewSession(TransactionId transactionId, int maxPrefixesCount)
{
return testSessionBuilder()
.setCatalog(TEST_CATALOG)
.setSchema("information_schema")
.setTransactionId(transactionId)
.setSystemProperty(MAX_PREFIXES_COUNT, String.valueOf(maxPrefixesCount))
.build()
.toConnectorSession();
}
@Test
public void testMaxPrefixesCountSessionPropertyBoundaries()
{
TransactionId transactionId = TransactionId.create();
// Minimum valid value
ConnectorSession sessionMin = createNewSession(transactionId, 1);
assertEquals(Integer.parseInt(sessionMin.getSystemProperty(MAX_PREFIXES_COUNT)), 1);
// Large valid value
int largeValue = 1000000;
ConnectorSession sessionLarge = createNewSession(transactionId, largeValue);
assertEquals(Integer.parseInt(sessionLarge.getSystemProperty(MAX_PREFIXES_COUNT)), largeValue);
// Invalid value: zero
try {
createNewSession(transactionId, 0);
fail("Expected exception for maxPrefixesCount = 0");
}
catch (IllegalArgumentException | RuntimeException e) {
// Expected
}
// Invalid value: negative
try {
createNewSession(transactionId, -5);
fail("Expected exception for maxPrefixesCount = -5");
}
catch (IllegalArgumentException | RuntimeException e) {
// Expected
}
}
```
If the session property validation does not currently throw an exception for invalid values, you may need to add or adjust validation logic elsewhere in the codebase to enforce the @Min(1) constraint.
</issue_to_address>
### Comment 2
<location> `presto-main-base/src/test/java/com/facebook/presto/sql/analyzer/TestFeaturesConfig.java:62` </location>
<code_context>
public void testDefaults()
{
assertRecordedDefaults(ConfigAssertions.recordDefaults(FeaturesConfig.class)
+ .setMaxPrefixesCount(100)
.setCpuCostWeight(75)
.setMemoryCostWeight(10)
</code_context>
<issue_to_address>
**suggestion (testing):** Missing test for invalid max-prefixes-count property in FeaturesConfig.
Add a test to ensure that invalid max-prefixes-count values (such as 0 or negative) are properly rejected according to the @Min(1) constraint.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
steveburnett
left a comment
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 documentation! A few requests.
1bd5d27 to
579dc36
Compare
steveburnett
left a comment
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 updated branch, new local doc build. Looks good, thanks for the updates!
...src/main/java/com/facebook/presto/connector/informationSchema/InformationSchemaMetadata.java
Outdated
Show resolved
Hide resolved
579dc36 to
eaae348
Compare
...src/main/java/com/facebook/presto/connector/informationSchema/InformationSchemaMetadata.java
Outdated
Show resolved
Hide resolved
presto-main-base/src/main/java/com/facebook/presto/sql/analyzer/FeaturesConfig.java
Outdated
Show resolved
Hide resolved
presto-main-base/src/main/java/com/facebook/presto/SystemSessionProperties.java
Outdated
Show resolved
Hide resolved
presto-main-base/src/test/java/com/facebook/presto/metadata/TestInformationSchemaMetadata.java
Show resolved
Hide resolved
Introduce a new session property `max-prefixes-count` to tune prefixes.
Co-authored-by: Prashant Sharma <[email protected]>
eaae348 to
a089af0
Compare
agrawalreetika
left a comment
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
|
@czentgr imported this issue as lakehouse/presto #25550 |
|
@mohsaka imported this issue as lakehouse/presto #25550 |
Description
An internal cherry-pick to make MAX_PREFIXES_COUNT configurable via session properties and config.properties. This helps improve performance of SHOW QUERY
Motivation and Context
Impact
Test Plan
UTs
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.