feat: ddl statements + catalog - #19830
Conversation
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 2 |
| P2 | 3 |
| P3 | 0 |
| Total | 5 |
Reviewed 42 of 42 changed files.
This is an automated review by Codex GPT-5.6-Sol
| @Override | ||
| protected void execute(CatalogTableWriter writer) | ||
| { | ||
| writer.updateProperties(tableId, properties); |
There was a problem hiding this comment.
[P1] Revalidate the complete table after property edits
This uses the property-only edit endpoint, whose transaction loads and validates properties without columns, so DatasourceDefn.validate(ResolvedTable) and its cross-field checks never run. For example, after defining a DAY projection, SET PROPERTIES can change segmentGranularity to PT1H even though full validation rejects a projection coarser than its segments; it can likewise clear sealed while __base remains. The catalog then contains an invalid specification and subsequent ingestion fails. Load and validate the complete revised TableSpec inside the Coordinator transaction.
| @Override | ||
| protected void execute(CatalogTableWriter writer) | ||
| { | ||
| writer.updateColumns(tableId, Collections.singletonList(column)); |
There was a problem hiding this comment.
[P1] Enforce column operation predicates atomically
UpdateColumns appends a column when its name is absent, so ALTER TABLE t ALTER COLUMN typo SET DATA TYPE BIGINT silently adds typo instead of rejecting the nonexistent target. ADD COLUMN has the inverse predicate checked by a separate Broker read, allowing concurrent ADDs to both pass and the later merge to overwrite the first type. Add/alter existence semantics need dedicated checks inside the Coordinator's column-update transaction.
| engine, | ||
| sql, | ||
| query, | ||
| CONTEXT, |
There was a problem hiding this comment.
[P2] Preserve statement context when planning projections
The nested planner receives only the hard-coded CONTEXT and discards the enclosing statement context, even though SET clauses are explicitly supported before DDL. For example, SET sqlTimeZone = 'America/Los_Angeles' followed by a projection using TIME_FLOOR stores a UTC expression, so the equivalent query under the same context plans differently and cannot match the projection. Merge relevant outer PlannerContext values before applying the deterministic overrides.
| ) | ||
| ); | ||
| } | ||
| final VirtualColumn virtualColumn = planned.getVirtualColumn(selected.get(i)); |
There was a problem hiding this comment.
[P2] Handle direct aliases in base projections
ScanQuery deduplicates its column list, while outputNames retains every SELECT item. A valid declared layout [id, copy] with SELECT id, id AS copy therefore has two outputs but only one selected entry, and the second iteration throws IndexOutOfBoundsException; a direct alias also has no virtual column to materialize copy. Preserve the select-to-source mapping or reject this form with a user-facing validation error.
| } | ||
| name.unparse(writer, leftPrec, rightPrec); | ||
|
|
||
| final SqlWriter.Frame frame = writer.startList("(", ")"); |
There was a problem hiding this comment.
[P2] Do not unparse omitted columns as empty parentheses
The grammar permits CREATE TABLE tbl PARTITIONED BY DAY with no parenthesized element list, but unparse always emits CREATE TABLE tbl () PARTITIONED BY DAY. Empty parentheses cannot be parsed because AddDruidTableElement is mandatory once '(' is present, so a valid AST does not round-trip. Omit the frame when both lists are empty or teach the grammar to accept ().
|
cc @maytasm |
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 1 |
| P2 | 1 |
| P3 | 0 |
| Total | 2 |
Reviewed 44 of 44 changed files. Found two current-head correctness issues around projection type compatibility and atomic __base updates.
This is an automated review by Codex GPT-5.6-Luna(max)
| } | ||
|
|
||
| final String granularity = table.stringProperty(SEGMENT_GRANULARITY_PROPERTY); | ||
| DataSchema.validateProjections( |
There was a problem hiding this comment.
[P1] ALTER COLUMN can leave projections type-incompatible
Projection validation checks referenced-column presence but does not reconcile projection dimension and aggregator types with current column definitions. A table with x BIGINT and SUM(x) can accept ALTER COLUMN x SET DATA TYPE VARCHAR, leaving an incompatible projection that later fails during initialization and can break ingestion. Validate compatibility or reject incompatible column edits before commit.
|
|
||
| if (BASE_PROJECTION_NAME.equals(projectionName)) { | ||
| // The base table is a property of the table, not one of its projections, so it is set rather than appended. | ||
| if (existing.spec().properties().get(DatasourceDefn.BASE_TABLE_PROPERTY) != null) { |
There was a problem hiding this comment.
[P2] Base projection existence check is not atomic
The __base branch performs the IF NOT EXISTS or duplicate check from a prior table read, then calls updateProperties without repeating that predicate inside the Coordinator transaction. Concurrent ADD PROJECTION __base statements can both observe absence and commit, with the later update overwriting the earlier projection; the non-IF form can also succeed instead of reporting a duplicate. Move the check into the atomic update.
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 1 |
| P2 | 1 |
| P3 | 0 |
| Total | 2 |
Reviewed 44 of 44 changed files.
This is an automated review by Codex GPT-5.6-Luna(max)
| if (baseTable == null) { | ||
| throw CatalogException.badRequest("A base table layout is required"); | ||
| } | ||
| return catalog.tables().updateProperties(id, table -> { |
There was a problem hiding this comment.
[P1] Base-table edits are not actually serialized
updateProperties performs an unlocked read followed by an update of the properties blob, with no row lock or version predicate. Concurrent ADD [IF NOT EXISTS] PROJECTION __base calls can both observe no layout and let the latter overwrite the former; a concurrent column edit can also leave __base referencing removed or retyped columns. Use row locking or compare-and-set updates and validate against the committed version.
| requireSealed(Boolean.TRUE.equals(existing.spec().properties().get(DatasourceDefn.SEALED_PROPERTY))); | ||
| writer.setBaseTable( | ||
| tableId, | ||
| translateBaseTable(handlerContext, tableId.name(), columns, alterTable.getProjection()), |
There was a problem hiding this comment.
[P2] IF NOT EXISTS still plans an existing base layout
The handler translates and plans the supplied base-table query before the Coordinator evaluates ifNotExists. If __base already exists, ALTER TABLE ... ADD IF NOT EXISTS PROJECTION __base AS (...) should be a no-op, but an invalid or missing column in the supplied body can still make it fail. Short-circuit known existing layouts before translation while retaining the server-side race check.
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 2 |
| P2 | 0 |
| P3 | 0 |
| Total | 2 |
Reviewed 45 of 45 changed files.
This is an automated review by Codex GPT-5.6-Luna(max)
| .bind(COLUMNS_COL, JacksonUtils.toBytes(jsonMapper, revised.columns())) | ||
| .bind(blobColumn, JacksonUtils.toBytes(jsonMapper, blob.apply(revised))) | ||
| .bind(UPDATE_TIME_COL, updateTime) | ||
| .bind(OLD_VERSION_PARAM, existing.updateTime()) |
There was a problem hiding this comment.
[P1] CAS version can repeat
The new compare-and-set update uses System.currentTimeMillis() as the replacement version, while the predicate compares the row against existing.updateTime(). Two edits in the same millisecond can therefore reuse the same version: both transactions may pass the old-version predicate, overwrite one another, and publish indistinguishable cache versions. Generate a strictly increasing version atomically (or use an equivalent database-side mechanism) before relying on this CAS.
| { | ||
| String path = tablePath(TABLE_EDIT_PATH, tableId); | ||
| FutureUtils.getUnchecked( | ||
| serviceClient.asyncRequest( |
There was a problem hiding this comment.
[P1] Retries can replay catalog mutations
This new mutating POST goes through a client configured with StandardRetryPolicy.maxAttempts(6). If the Coordinator commits the DDL but the response is lost, a retryable channel error or 5xx can replay the non-idempotent edit (and the analogous create request), producing conflicts or duplicate effects while the SQL caller sees failure. Disable retries for writes or add request idempotency/deduplication.
There was a problem hiding this comment.
producing conflicts or duplicate effects
I don't think this can really happen with the compare and set stuff, so mainly the risk is a confusing error message? Re: idempotency, the SQL supports IF NOT EXISTS/IF EXISTS, so i don't think we want it to be implicitly idempotent.
Also this retrying isn't really new in this PR, createTable was already going through it prior to this PR.. maybe in a follow-up we could consider tightening up retry on write operations, since I think all it really gives us is a more accurate error message?
FrankChen021
left a comment
There was a problem hiding this comment.
I have reviewed the code for correctness, edge cases, concurrency, and integration risks; no new issues found.
Reviewed 45 of 45 changed files.
Validation: git diff --check on the current 45-file PR diff; tests and builds not run.
This is an automated review by Codex GPT-5.6-Luna(max)
capistrant
left a comment
There was a problem hiding this comment.
This is a very cool addition to Druid, thank you for all the work and iteration that has gone into the PR. I reviewed test coverage and docs extensively to understand the features. I support the overall design and impl decisions spec'd out. I appreciate the detailed doc and the coverage that spans UTs and Embedded tests to provide confidence in the fact that the feature does what is spec'd out in the PR description and docs.
The many rounds with @FrankChen021 seem to have also worked through a lot of potential gotchas and have contributed to confidence in bugs being smoked out in this process versus in live testing after merge.
I'm approving with some nits I found reading through all the test files and javadocs. I also took into consideration that this is an experimental feature defaulted to off that is part of an experimental extension. release notes should stress this rawness and solicit bug reports/feedback from any early adopters.
| @SuppressWarnings("unchecked") | ||
| private DatasourceProjectionMetadata projection(int index) | ||
| { | ||
| return ((List<DatasourceProjectionMetadata>) WRITER.calls.get(0).spec.properties().get("projections")).get(index); | ||
| } | ||
|
|
||
| private String projectionsJson() throws Exception | ||
| { | ||
| return queryFramework().queryJsonMapper() | ||
| .writeValueAsString(WRITER.calls.get(0).spec.properties().get("projections")); | ||
| } |
There was a problem hiding this comment.
nit: don't interleave these in the tests
| private List<DatasourceProjectionMetadata> projectionsOf(String tableName) | ||
| { | ||
| return TestHelper.JSON_MAPPER.convertValue( | ||
| client.readTable(TableId.datasource(tableName)) | ||
| .spec().properties().get(DatasourceDefn.PROJECTIONS_KEYS_PROPERTY), | ||
| new TypeReference<List<DatasourceProjectionMetadata>>() {} | ||
| ); | ||
| } |
There was a problem hiding this comment.
nit: another interleaved private that could be moved out of test area
| written into a defined column of the table is consistent with that columns definition, minimizing errors where unexpected | ||
| data is written into a particular column of the table. | ||
|
|
||
| ### SQL DDL |
There was a problem hiding this comment.
nice doc, ty for adding it up front. greatly helped set stage for review
| private String columnType(String tableName, String columnName) throws CatalogException | ||
| { | ||
| return catalog.tables().read(TableId.datasource(tableName)).spec().columns().stream() | ||
| .filter(c -> columnName.equals(c.name())) | ||
| .findFirst() | ||
| .orElseThrow(() -> new AssertionError("No column [" + columnName + "]")) | ||
| .dataType(); | ||
| } |
There was a problem hiding this comment.
nit: don't interleave with tests here. sorry to be repeating so many of these 😅
| } | ||
|
|
||
| /** | ||
| * The Druid type of a column, which for {@code __time} is always {@link ColumnType#LONG} whatever was declared. |
There was a problem hiding this comment.
The end reads confusing to me. are you meaning "regardless of what was declared" or something in that direction
Description
This PR adds DDL statement support to Druid so that
CREATEandALTERstatements can be used to manage the catalog contents. This functionality requires WRITE permissions to the datasource (same as the backing catalog APIs), and is gated behind a new runtime propertydruid.sql.planner.enableCatalogDdlwhich is false by default. This PR includes support for defining the logical schema (columns list in catalog), aggregate projections (projections property in catalog), base table projection (baseTable property in catalog), clustering (clusterKeysin catalog), time partitioning (segmentGranularityin catalog), and any other properties (sealed, targetSegmentRows) with generic property setting syntax.These operations are purely metadata operations, and so
DROP TABLEhas been omitted since in my mind it has the most room for confusion (a DROP TABLE only cleared metadata and didn't do anything else sounds kind of confusing to me). Dropping columns and projections from a table felt more easily explainable that these do not modify any existing data and instead stage the schema for subsequent ingestion (and future work should wire this catalog stuff better into compaction/reindexing so that we can also frame it such that compaction/reindexing will begin to eventually converge on the updated schema).These all go through the regular query path, and return 0 rows on successful operation. Some follow-up work is needed to improve web-console syntax highlighting and behaviors, and since there is no drop table statement it would still probably be nice to eventually add a catalog management ui, but the statements at least already work as-is through the current query interface.
Some examples:
create table:
alter table to add column:
alter table to add projection:
alter table set property:
alter table set 'base table' projection to create clustered segments:
create table with base table and projection definitions: