fix(plugin-pinot): Add validation for schema names in Pinot #26725
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
prestodb-ci
requested review from
a team,
bibith4 and
wanglinsong
and removed request for
a team
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds centralized schema name constant and enforces that the Pinot connector only exposes and accepts the single supported schema "default", throwing NOT_FOUND when queries reference any other schema. Sequence diagram for Pinot schema name validation in getTableHandlesequenceDiagram
actor User
participant PrestoClient
participant PrestoCoordinator
participant PinotMetadata
User->>PrestoClient: Submit query
PrestoClient->>PrestoCoordinator: Send SQL
PrestoCoordinator->>PinotMetadata: getTableHandle(session, tableName)
PinotMetadata->>PinotMetadata: normalizedSchema = normalizeIdentifier(session, tableName.schemaName)
alt schema is default
PinotMetadata->>PinotMetadata: pinotTableName = getPinotTableNameFromPrestoTableName(tableName.tableName)
PinotMetadata-->>PrestoCoordinator: new PinotTableHandle(connectorId, SCHEMA_NAME, pinotTableName)
PrestoCoordinator-->>PrestoClient: Query planned and executed
PrestoClient-->>User: Return results
else schema is not default
PinotMetadata-->>PrestoCoordinator: throw PrestoException(NOT_FOUND)
PrestoCoordinator-->>PrestoClient: Error response
PrestoClient-->>User: Schema does not exist error
end
Updated class diagram for PinotMetadata schema handlingclassDiagram
class PinotMetadata {
+String connectorId
+PinotConnection pinotPrestoConnection
+PinotConfig pinotConfig
+static String SCHEMA_NAME
+PinotMetadata(ConnectorId connectorId, PinotConnection pinotPrestoConnection, PinotConfig pinotConfig)
+List~String~ listSchemaNames(ConnectorSession session)
+PinotTableHandle getTableHandle(ConnectorSession session, SchemaTableName tableName)
+List~SchemaTableName~ listTables(ConnectorSession session, String schemaName)
-String getPinotTableNameFromPrestoTableName(String prestoTableName)
}
class PinotConnection {
+List~String~ getTableNames()
}
class PinotTableHandle {
+PinotTableHandle(String connectorId, String schemaName, String pinotTableName)
}
class SchemaTableName {
+SchemaTableName(String schemaName, String tableName)
+String getSchemaName()
+String getTableName()
}
class ConnectorSession {
}
class ConnectorId {
}
class PinotConfig {
}
PinotMetadata --> PinotConnection : uses
PinotMetadata --> PinotConfig : uses
PinotMetadata --> PinotTableHandle : creates
PinotMetadata --> SchemaTableName : uses
PinotMetadata --> ConnectorSession : uses
PinotMetadata --> ConnectorId : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider making
SCHEMA_NAMEprivate static final(and potentially renaming to something likeDEFAULT_SCHEMA_NAME) since it is an internal implementation detail and not intended as part of the public API surface for this class. - For consistency with
getTableHandle, you may want to validate theschemaNameargument inlistTables(e.g., normalize and compare againstSCHEMA_NAME, returning an empty list or throwing if it doesn't match) rather than ignoring the passed schema.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider making `SCHEMA_NAME` `private static final` (and potentially renaming to something like `DEFAULT_SCHEMA_NAME`) since it is an internal implementation detail and not intended as part of the public API surface for this class.
- For consistency with `getTableHandle`, you may want to validate the `schemaName` argument in `listTables` (e.g., normalize and compare against `SCHEMA_NAME`, returning an empty list or throwing if it doesn't match) rather than ignoring the passed schema.
## Individual Comments
### Comment 1
<location> `presto-pinot-toolkit/src/main/java/com/facebook/presto/pinot/PinotMetadata.java:51-54` </location>
<code_context>
private final String connectorId;
private final PinotConnection pinotPrestoConnection;
private final PinotConfig pinotConfig;
+ public static final String SCHEMA_NAME = "default";
@Inject
</code_context>
<issue_to_address>
**suggestion:** Consider reducing the visibility of SCHEMA_NAME if it is only intended for internal use.
If nothing outside this class (or package) needs this constant, consider making `SCHEMA_NAME` private or package-private to avoid exposing it as part of the public API and to preserve flexibility for future changes.
```suggestion
private final String connectorId;
private final PinotConnection pinotPrestoConnection;
private final PinotConfig pinotConfig;
private static final String SCHEMA_NAME = "default";
```
</issue_to_address>
### Comment 2
<location> `presto-pinot-toolkit/src/main/java/com/facebook/presto/pinot/PinotMetadata.java:81-89` </location>
<code_context>
@Override
public PinotTableHandle getTableHandle(ConnectorSession session, SchemaTableName tableName)
{
+ if (!SCHEMA_NAME.equals(normalizeIdentifier(session, tableName.getSchemaName()))) {
+ throw new PrestoException(NOT_FOUND, format("Schema %s does not exist", tableName.getSchemaName()));
+ }
String pinotTableName = getPinotTableNameFromPrestoTableName(tableName.getTableName());
</code_context>
<issue_to_address>
**suggestion:** Align the error message with the enforced schema to make the failure mode clearer.
Because this connector only works with the `SCHEMA_NAME` schema, consider making the error say that explicitly (e.g., mention that only that schema is supported or show the normalized expected value). That way, users who pass another schema understand the real constraint rather than seeing a generic "does not exist" message.
```suggestion
@Override
public PinotTableHandle getTableHandle(ConnectorSession session, SchemaTableName tableName)
{
String normalizedSchemaName = normalizeIdentifier(session, tableName.getSchemaName());
if (!SCHEMA_NAME.equals(normalizedSchemaName)) {
throw new PrestoException(
NOT_FOUND,
format(
"Schema '%s' is not supported by the Pinot connector. Only schema '%s' is supported.",
tableName.getSchemaName(),
SCHEMA_NAME));
}
String pinotTableName = getPinotTableNameFromPrestoTableName(tableName.getTableName());
return new PinotTableHandle(connectorId, tableName.getSchemaName(), pinotTableName);
}
```
</issue_to_address>
### Comment 3
<location> `presto-pinot-toolkit/src/main/java/com/facebook/presto/pinot/PinotMetadata.java:125` </location>
<code_context>
public List<SchemaTableName> listTables(ConnectorSession session, String schemaNameOrNull)
</code_context>
<issue_to_address>
**suggestion (bug_risk):** listTables ignores schemaNameOrNull, which can be inconsistent with the stricter schema check in getTableHandle.
Since `getTableHandle` now enforces `SCHEMA_NAME`, `listTables` should probably also honor `schemaNameOrNull`. If `schemaNameOrNull` is non-null and its normalized value differs from `SCHEMA_NAME`, consider returning an empty list rather than listing tables from the default schema, to keep discovery consistent with `getTableHandle`’s access rules.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
presto-pinot-toolkit/src/main/java/com/facebook/presto/pinot/PinotMetadata.java
Outdated
Show resolved
Hide resolved
| ImmutableList.Builder<SchemaTableName> builder = ImmutableList.builder(); | ||
| for (String table : pinotPrestoConnection.getTableNames()) { | ||
| builder.add(new SchemaTableName("default", table)); | ||
| builder.add(new SchemaTableName(SCHEMA_NAME, table)); |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): listTables ignores schemaNameOrNull, which can be inconsistent with the stricter schema check in getTableHandle.
Since getTableHandle now enforces SCHEMA_NAME, listTables should probably also honor schemaNameOrNull. If schemaNameOrNull is non-null and its normalized value differs from SCHEMA_NAME, consider returning an empty list rather than listing tables from the default schema, to keep discovery consistent with getTableHandle’s access rules.
agrawalreetika
force-pushed
the
pinot-schema-validation
branch
4 times, most recently
from
b98931b to
57b05d3
Compare
agrawalreetika
force-pushed
the
pinot-schema-validation
branch
from
57b05d3 to
dff918d
Compare
bibith4
reviewed
Dec 5, 2025
| assertEquals(withAnotherSchema.getTableName(), TestPinotSplitManager.realtimeOnlyTable.getTableName()); | ||
| // Invalid schema should now fail | ||
| assertThrows(PrestoException.class, () -> metadata.getTableHandle(session, | ||
| new SchemaTableName("foo", TestPinotSplitManager.realtimeOnlyTable.getTableName()))); |
Contributor
There was a problem hiding this comment.
@agrawalreetika Can we use static import for TestPinotSplitManager.realtimeOnlyTable
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Add validation for schema names in Pinot
Motivation and Context
For Pinot connector in Presto, schema name is set to
default- https://github.com/prestodb/presto/blob/master/presto-pinot-toolkit/src/main/java/com/facebook/presto/pinot/PinotMetadata.java#L63Apart from the schema name validation in ShowQueriesRewrite for some of the sql queries, there is no validation for schema names in the connector.
This change is to have the schema name validation in the connector.
Impact
Add validation for schema names in Pinot.
Before,
Here below query should fail as schema name is not
default-After,
Test Plan
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.