dongjoon-hyun commented on PR #58153: URL: https://github.com/apache/spark/pull/58153#issuecomment-5915478925
Thank you for the update, @anuragmantri. I took one more pass over `d2aa8674fdc` and have some additional comments. I continue the numbering from [my previous comment](https://github.com/apache/spark/pull/58153#issuecomment-5881105596) so that the points can be referred to unambiguously. None of these is a regression of existing behavior, and they come from reading the code rather than from running it. 7. **`SHOW CREATE TABLE` silently drops every write clause when a transform name needs quoting.** `transformName=identifier` accepts a quoted name and the parser strips the backticks (`parsers.scala:310`), so ``CREATE TABLE cat.t (id INT, p INT) USING foo PARTITIONED BY (p) DISTRIBUTED BY PARTITION ORDERED BY `z-order`(id)`` is a valid statement. `isSpellable` rejects the name with its regex (`ShowCreateTableExec.scala:139`), and because the `forall(isSpellable)` guard at `:151` wraps the whole block, `SHOW CREATE TABLE` prints neither `ORDERED BY` nor `DISTRIBUTED BY PARTITION`. Replaying the output creates a table without the declared layout, with no error. The earlier discussion treated a name like `+` as having no spelling, but `` `+`(id, 1) `` does parse as an `applyTransform`. Could we render the name with `quoteIfNeeded(t.name)` in `toSQL` (`WriteDistributionAndOrdering.scala:67`) and drop the name check? The regex is the same as `QuotingUtils.validIdentPattern`, and column references are already quoted this way. The `+(id, 1)` fixture would then round-trip, so the "unspellable" test needs a key that really has no spelling, e.g. a transform whose argument is another transform. 8. **`DISTRIBUTED BY PARTITION` on a schemaless `CREATE`/`REPLACE TABLE` gives advice that cannot be followed.** The parser decides "no partitioning" from the absence of `PARTITIONED BY` / `CLUSTERED BY` in the statement (`AstBuilder.scala:6518`, `:6619`). For `CREATE TABLE cat.t USING foo LOCATION '/x' DISTRIBUTED BY PARTITION`, the error says "Please add PARTITIONED BY or CLUSTERED BY ... INTO ... BUCKETS, or drop the clause and use only ORDERED BY." However, adding `PARTITIONED BY (p)` then fails in `PreprocessTableCreation` with `_LEGACY_ERROR_TEMP_1165` ("It is not allowed to specify partitioning when the table schema is not defined", `rules.scala:330-332`), and using only `ORDERED BY` fails with `SPECIFY_WRITE_ORDERING_IS_NOT_ALLOWED`. So the hash mode cannot be declared at all when the schema and partitioning come from the catalog, while `UNORDERED` is accepted there. The docs mention the schemaless restriction for `ORDERED BY` (`sql-ref-syntax-ddl-create-table-datasource.m d:127`) but not for `DISTRIBUTED BY PARTITION` (`:114`). Could we at least fix the message and add the sentence to the docs? Alternatively, the check could move next to the schemaless check in `PreprocessTableCreation` and be skipped when the schema is empty, leaving that case to the catalog. 9. **Built-in transform names are matched case-sensitively, so the pinned limitation is wider than the test shows.** `visitTransform` dispatches on the raw text (`applyCtx.identifier.getText match { case "bucket" ... case "days" ... }`, `AstBuilder.scala:5869`), so `DAYS(ts)` or `BUCKET(4, id)` becomes an `ApplyTransform` rather than a `DaysTransform` / `BucketTransform`. In the default case-insensitive session, `ORDERED BY days(TS)` and `ORDERED BY bucket(4, ID)` succeed, but `ORDERED BY DAYS(TS)` and `ORDERED BY BUCKET(4, ID)` fail with `WRITE_ORDERING_WITH_UNKNOWN_COLUMN`, and `BUCKET(4, id)` hands the catalog a generic transform named `BUCKET`. The dispatch is pre-existing and `PARTITIONED BY` behaves the same, so this is not a regression, but it is another way to hit the limitation pinned by "a case-insensitive session does not normalize an ApplyTransform's references". Could you add it to that test? As a possible follow-up, `RewritableTransform` is a sealed trait in the same file and its only user is `PreprocessTableCreation`, so making `ApplyTransform` a `RewritableTransform` would let both `ORDERED BY` and `PARTITIONED BY` normalize these references and remove the limitation. 10. **The capability Javadoc names only `createTable`, but the request can be dropped per method.** `TableCatalog.createTable(Identifier, TableInfo)` and `StagingTableCatalog.stageCreate` / `stageReplace` / `stageCreateOrReplace(Identifier, TableInfo)` all forward only columns, partitions and properties by default. A staging catalog that advertises the capability and overrides the three `stage*` overloads still loses the request silently for `CREATE TABLE cat.t (id INT) USING x ORDERED BY id`, because a `CREATE TABLE` with a column list is not staged and goes to `createTable` (the fixture comment at `CreateTableWriteOrderSuite.scala:706` describes this trap). Could the Javadoc of `SUPPORTS_CREATE_TABLE_WITH_WRITE_DISTRIBUTION_AND_ORDERING` and of `TableInfo#writeDistributionMode()` list the four `TableInfo` overloads and say that a catalog advertising the capability must override every one it can be reached through? This pairs with point 3. 11. **`PARTITIONED BY` and `ORDERED BY` now render the same transform differently in one output.** The literal fix went into a renderer private to the ordering (`WriteDistributionAndOrdering.toSQL`), while `showTablePartitioning` still uses `t.describe()` (`ShowCreateTableExec.scala:112`), and so does the `Part N` row in `DescribeTableExec.scala:161`. For `PARTITIONED BY (truncate(s, 10L)) ORDERED BY truncate(s, 10L)`, `SHOW CREATE TABLE` prints `PARTITIONED BY (truncate(s, 10))` next to `ORDERED BY (truncate(s, 10L) ASC NULLS FIRST)`, and replaying it turns the partition literal into an `INT`. The partition half is pre-existing, so I'm fine with a follow-up, but could the comment at `WriteDistributionAndOrdering.scala:62` say that `PARTITIONED BY` still goes through `describe`? Sharing one renderer would be the real fix once points 1 and 2 are settled. 12. **Question on the shape of a sort key in the new API.** `ORDERED BY id` reaches the catalog as `sort(identity(id), ...)` because `visitWriteOrderField` wraps whatever `visitTransform` returns (`AstBuilder.scala:6368`). Other producers of v2 `SortOrder` in Spark use a bare `NamedReference` for a column (e.g. `DataSourceStrategy.translateSortOrders`), which is also the usual form for `requiredOrdering()`, and `V2ExpressionSQLBuilder` does not accept a `Transform` inside a `SortOrder`. Spark's own write path resolves both forms, so nothing is broken. However, `TableInfo#writeOrdering()` and `Table#writeOrdering()` become public API in 4.4.0, so could we either unwrap the identity transform or document the shape in the Javadoc (each key is a `Transform`, and a plain column comes as `identity`)? It would also be good to state there that the array is never null, and whether checking that a key's type is orderable is left to the catalog (e.g. `ORDERED BY m` on a `MAP` column is accep ted today). 13. **Question on the plan and exec signatures.** This is a follow-up to my earlier point on source compatibility. I said the break was acceptable, but there is a precedent going the other way: SPARK-43529 added a default parameter to the same four plans, and its follow-up (`7e94f2a5433`) moved it into `UnresolvedTableSpec` to "Restore the signatures of class `CreateTable`, `CreateTableAsSelect`, `ReplaceTable` and `ReplaceTableAsSelect`". Constraints and the default collation also travel on `TableSpec`, which already reaches every consumer here, so carrying the pair there would leave the 22 positional patterns (`ApplyDefaultCollation`, `ResolveCatalogs` and `TypeCoercionBase` change only for arity) and the 7 exec signatures untouched. For example, Apache Paimon's `PaimonCreateTableAsSelectStrategy` matches `CreateTableAsSelect` and constructs `CreateTableAsSelectExec` positionally, so it stops compiling with this PR. Did you consider this? I'm fine with a follow-up if it is too m uch for this PR. 14. **Tests.** - `CreateTableWriteOrderSuite.scala:550` asserts with `ddl.contains(expected)`, and `ORDERED BY (id DESC NULLS LAST)` is also a substring of `LOCALLY ORDERED BY (id DESC NULLS LAST)`, while the replay test at `:532` compares only `ordering()`. So emitting `LOCALLY ORDERED BY` for a `range` table would not fail any test. Each clause is printed on its own line, so `ddl.split("\n").contains(expected)` would pin the mode. Comparing `writeDistributionMode()` in the replay test and adding a `PARTITIONED BY (c) ORDERED BY (id)` case would help too. - "the new keywords stay usable as identifiers" toggles only `spark.sql.ansi.enabled`, but the parser switches to `ansiNonReserved` only when `spark.sql.ansi.enforceReservedKeywords` is also true (`SQLConf.scala:9779`), so both iterations take the `nonReserved` path. Could you set `ENFORCE_RESERVED_KEYWORDS` in the ANSI iteration? Minor: - `rules.scala:362-365`: `findNestedField` already returns the normalized path, so the body of `normalizeResolvableReferences` can be `schema.findNestedField(fieldNames, resolver = resolver).map { case (path, field) => FieldReference(path :+ field.name) }.getOrElse(ref)` instead of three schema traversals through two lookup functions with different rules (`DescribeTableExec.scala:117-119` uses the same idiom). `normalizeReferences` at `:343` has a single caller and could stay inline as before. - `ShowCreateTableExec.scala:131-134`: the scaladoc describes the `transform` rule, not `transformArgument`. Also, `select` fails to parse only when `spark.sql.ansi.enforceReservedKeywords` is true, not under ANSI in general. - `CreateTableWriteOrderSuite.scala:706`: a `REPLACE TABLE` with a column list is staged (the test at `:381` asserts `stageReplace`); only `CREATE TABLE` is not. - Related to point 1: a connector-reported typed `NULL` literal renders as `CAST(NULL AS INT)` (`literals.scala:654`), so an allow-list of literal types in `isSpellable` may be simpler than special-casing `FLOAT` and non-finite values. - The hash-without-partitioning check is duplicated verbatim in `visitCreateTable` and `visitReplaceTable`. It could live in `writeSpecsFrom`. Pre-existing in the shared `visitTransform` and now reachable from `ORDERED BY` (fine as follow-ups): - `bucket(4294967300, id)` is silently truncated to `bucket(4, id)` by `.toInt` (`AstBuilder.scala:5877`), while a `DECIMAL` or `TINYINT` count is rejected. - `ORDERED BY IDENTIFIER('id')` is not resolved to `id` because `visitQualifiedName` uses `_.getText` (`AstBuilder.scala:5806`) instead of `getIdentifierParts`. `PARTITIONED BY` has the same gap. -- 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]
