fix(plugin-druid): Add validation for schema names in Druid#26723
fix(plugin-druid): Add validation for schema names in Druid#26723agrawalreetika merged 1 commit intoprestodb:masterfrom
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds schema name validation to the Druid connector by enforcing that queried schema names match the configured Druid schema and raising a NOT_FOUND PrestoException when they do not. Sequence diagram for Druid schema validation in getTableHandlesequenceDiagram
actor User
participant PrestoCoordinator
participant DruidMetadata
participant DruidClient
User->>PrestoCoordinator: Submit query (SELECT ... FROM wikipedia1.table)
PrestoCoordinator->>DruidMetadata: getTableHandle(session, tableName)
DruidMetadata->>DruidClient: getSchema()
DruidClient-->>DruidMetadata: configuredSchema
DruidMetadata->>DruidMetadata: normalizeIdentifier(session, configuredSchema)
DruidMetadata->>DruidMetadata: normalizeIdentifier(session, tableName.getSchemaName())
alt schema matches configured schema
DruidMetadata->>DruidClient: getTables()
DruidClient-->>DruidMetadata: tableNames
DruidMetadata-->>PrestoCoordinator: ConnectorTableHandle
PrestoCoordinator-->>User: Query continues to planning/execution
else schema does not match
DruidMetadata-->>PrestoCoordinator: throw PrestoException(NOT_FOUND)
PrestoCoordinator-->>User: Error Schema wikipedia1 does not exist
end
Class diagram for updated DruidMetadata schema validationclassDiagram
class DruidMetadata {
+listSchemaNames(ConnectorSession session) List~String~
+getTableHandle(ConnectorSession session, SchemaTableName tableName) ConnectorTableHandle
}
class DruidClient {
+getSchema() String
+getTables() List~String~
}
class ConnectorSession
class SchemaTableName {
+getSchemaName() String
+getTableName() String
}
class PrestoException {
+PrestoException(StandardErrorCode errorCode, String message)
}
class StandardErrorCode {
<<enumeration>>
NOT_FOUND
}
DruidMetadata --> DruidClient : uses
DruidMetadata --> ConnectorSession : uses
DruidMetadata --> SchemaTableName : uses
DruidMetadata --> PrestoException : throws
PrestoException --> StandardErrorCode : uses
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 - here's some feedback:
- Consider normalizing and caching the configured schema name once (e.g., in the constructor) instead of calling
normalizeIdentifierondruidClient.getSchema()for everygetTableHandleinvocation to avoid repeated work on a hot path. - It may be safer to skip the validation or fail fast with a clearer configuration error if
druidClient.getSchema()isnullor blank, rather than throwing a NOT_FOUND error tied to the query. - To keep error handling consistent with other connectors, consider using a schema-specific error code or existing helper (e.g., a
SchemaNotFoundutility) and align the error message format with other schema-not-found messages in Presto.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider normalizing and caching the configured schema name once (e.g., in the constructor) instead of calling `normalizeIdentifier` on `druidClient.getSchema()` for every `getTableHandle` invocation to avoid repeated work on a hot path.
- It may be safer to skip the validation or fail fast with a clearer configuration error if `druidClient.getSchema()` is `null` or blank, rather than throwing a NOT_FOUND error tied to the query.
- To keep error handling consistent with other connectors, consider using a schema-specific error code or existing helper (e.g., a `SchemaNotFound` utility) and align the error message format with other schema-not-found messages in Presto.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
hantangwangd
left a comment
There was a problem hiding this comment.
@agrawalreetika thanks for the fix, looks good to me. Could you update the PR title? This doesn't look like an Iceberg change.
BryanCutler
left a comment
There was a problem hiding this comment.
LGTM, could you add a test?
I’m sorry — I didn’t see your comment before the merge. Currently, there are no actual integration tests available for Druid. I had opened an issue earlier to highlight this gap: #26143 |
Description
Add validation for schema names in Druid
Motivation and Context
For Druid connector in Presto, schema name is provided via druid catalog config - https://github.com/prestodb/presto/blob/master/presto-druid/src/main/java/com/facebook/presto/druid/DruidConfig.java#L92
Apart 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 Druid
Here in Druid catalog,

druid.schema-name=wikipediaBefore -
Below query should fail, since schema name is configured as
wikipedianotwikipedia1After -

Test Plan
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.