FrankChen021 commented on code in PR #19830: URL: https://github.com/apache/druid/pull/19830#discussion_r3940729543
########## sql/src/main/codegen/includes/ddl.ftl: ########## @@ -0,0 +1,267 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +// Druid catalog DDL. These statements write catalog metadata only; they do not create or delete data. +// +// CREATE TABLE is reached through the standard Calcite SqlCreate() production, which has already consumed +// CREATE [OR REPLACE], so this production must not consume <EOF>: the enclosing SqlStmtList() handles statement +// separators. ALTER TABLE is a top-level statement production instead, because Calcite's stock SqlAlter() mandates +// a SYSTEM or SESSION scope that does not apply here. + +SqlCreate DruidSqlCreateTable(Span s, boolean replace) : +{ + boolean ifNotExists = false; + final SqlIdentifier id; + final List<SqlNode> columns = new ArrayList<SqlNode>(); + final List<SqlNode> projections = new ArrayList<SqlNode>(); + Span elementSpan = null; + SqlGranularityLiteral partitionedBy = null; + SqlNodeList clusteredBy = null; + boolean sealed = false; +} +{ + <TABLE> + [ <IF> <NOT> <EXISTS> { ifNotExists = true; } ] + id = CompoundTableIdentifier() + [ + // SEALED binds to the column list rather than trailing the statement: it declares that the list is the table's + // whole schema, so it is only accepted when there is a list for it to describe. + [ <SEALED> { sealed = true; } ] + <LPAREN> { elementSpan = span(); } + AddDruidTableElement(columns, projections) + ( + <COMMA> AddDruidTableElement(columns, projections) + )* + <RPAREN> + ] + [ + <PARTITIONED> <BY> + partitionedBy = PartitionGranularity() + ] + [ + clusteredBy = ClusteredBy() + ] + { + final SqlParserPos elementPos = elementSpan == null ? s.pos() : elementSpan.end(this); + return new DruidSqlCreateTable( + s.end(this), + replace, + ifNotExists, + id, + new SqlNodeList(columns, elementPos), + new SqlNodeList(projections, elementPos), + partitionedBy, + clusteredBy, + sealed + ); + } +} + +// A table element is either a column declaration or a projection definition. A column may legitimately be named +// "projection" (the keyword is non-reserved) and may have a bare-identifier type, so two tokens are not enough to +// tell the two apart: a projection definition is distinguished by its third token, which is always '(' or AS. +void AddDruidTableElement(List<SqlNode> columns, List<SqlNode> projections) : +{ + final DruidSqlColumnDeclaration column; + final SqlProjectionSpec projection; +} +{ + LOOKAHEAD(3) Review Comment: [P2] Avoid misclassifying projection-named complex columns `CREATE TABLE t (projection TYPE('COMPLEX<json>'))` is a valid column declaration: `PROJECTION` is intentionally non-reserved and `TYPE('...')` is the supported Druid-native type escape hatch. With `LOOKAHEAD(3)`, the token sequence `PROJECTION TYPE (` selects `DruidProjectionDefinition` (TYPE is also a non-reserved Calcite keyword), which then expects `SELECT` and rejects the declaration. This makes valid schema syntax depend on the column name; distinguish the complete projection production or special-case `TYPE` after a `projection` identifier. ########## extensions-core/druid-catalog/src/main/java/org/apache/druid/catalog/storage/sql/SQLCatalogManager.java: ########## @@ -344,88 +357,31 @@ public TableMetadata withHandle(Handle handle) throws NotFoundException " properties = :properties,\n" + " updateTime = :updateTime\n" + "WHERE schemaName = :schemaName\n" + - " AND name = :name\n"; + " AND name = :name\n" + + " AND updateTime = :oldVersion"; Review Comment: [P2] Keep CAS edits restricted to active rows The new compare-and-set update for `updateProperties` (and the analogous `UPDATE_COLUMNS_STMT`) checks only schema/name/updateTime; it omits the `state = 'A'` predicate used by `REPLACE_SPEC_STMT`. `markDeleting` changes state to `D` without a version predicate and assigns `nextVersion(0)`, so if deletion and an edit occur in the same millisecond, the deleting row can retain the version the edit read and the edit still rewrites the DELETING row and emits an update event. `TableMetadata.forUpdate` reports that event as ACTIVE, violating the existing "can't update when deleting" contract and letting caches observe metadata after deletion. Add `AND state = 'A'` to both incremental update statements or make deletion participate in the same version guard. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
