dongjoon-hyun commented on PR #58153:
URL: https://github.com/apache/spark/pull/58153#issuecomment-5881105596
Thank you for addressing the previous comments, @anuragmantri. I confirmed
that my four points are resolved in `d2aa8674fdc`. I have a few more comments,
mostly on the last round of changes.
1. **`SHOW CREATE TABLE` can still emit DDL that does not parse for a
`FLOAT` literal.** `toSQL` now renders literals through Catalyst `Literal.sql`
(`WriteDistributionAndOrdering.scala:65`), but `Literal.sql` renders a
`FloatType` value as `CAST('1.5' AS FLOAT)`, and `transformArgument` only
accepts `qualifiedName | constant`. So `ORDERED BY f(id, 1.5F)` is accepted
(`isSpellable` only checks `isInstanceOf[Literal[_]]` at
`ShowCreateTableExec.scala:140`), but `SHOW CREATE TABLE` prints `ORDERED BY
(f(id, CAST('1.5' AS FLOAT)) ASC NULLS FIRST)`, which cannot be replayed. The
same applies to non-finite `DOUBLE` values reported by a connector (`CAST('NaN'
AS DOUBLE)`). Could you render `FloatType` as `<v>F` (`FLOAT_LITERAL`), reject
non-finite float/double literals in `isSpellable`, and add `1.5F` to the
round-trip test at `CreateTableWriteOrderSuite.scala:525`?
2. **`DESCRIBE TABLE EXTENDED` / `SHOW CREATE TABLE` can throw for a
connector-reported literal.** `CatalystLiteral(l.value, l.dataType)` requires
the value to be in the internal representation already, so a `LiteralValue`
holding a `java.lang.String` (which `Expressions.literal("x")` produces) fails
with `IllegalArgumentException: requirement failed: Literal must have a
corresponding value to string ...`. The connector would be violating the
`Literal` Javadoc, but `DESCRIBE` is the place where we promise to report the
declared layout as-is. `CatalystLiteral.create(l.value, l.dataType)` accepts
both representations.
3. **The `CLUSTER BY` contract is documented only in the SQL reference.**
The answer to @szehon-ho's question (Spark passes both declarations without
reconciling them, and the catalog interprets or rejects the combination) is
meant for connector authors, so could we also state it in the Javadoc of
`TableCatalogCapability.SUPPORTS_CREATE_TABLE_WITH_WRITE_DISTRIBUTION_AND_ORDERING`
(`TableCatalogCapability.java:98`) or of `TableInfo#writeDistributionMode()`?
4. **Question on `DelegatingTable`.** `DelegatingTable` now reports the
declared layout (`DelegatingTable.java:89-96`), but Spark reads and writes a
`DelegatingTable` through the v1 path (`RelationResolution.scala:467`), which
never looks at it. If such a catalog advertises the capability, `DESCRIBE` /
`SHOW CREATE TABLE` show an `ORDERED BY` that Spark's own writes never apply.
Is this intended? If so, could we mention it in the capability Javadoc?
5. **Question on the public API shape.** `Table#writeDistributionMode()` and
the new `TableInfo` methods become public API in 4.4.0 and are hard to change
afterwards. Did you consider a small Java enum (like `SortDirection` /
`NullOrdering`) instead of `String` plus the `TableInfo.DISTRIBUTION_MODE_*`
constants? It would make the closed set explicit, and `SHOW CREATE TABLE` would
no longer need the unknown-mode (`zigzag`) case. If we keep `String`, please
add `@since 4.4.0` to the three constants (`TableInfo.java:40,46,52`). cc
@aokolnychyi
6. **Test suggestion.** The main motivation is that the first load of a
CTAS/RTAS can already follow the declared layout. Since
`CreateTableAsSelectExec` writes through `AppendData` on the newly created
table, a single test would pin this: a test catalog that builds
`InMemoryTable(..., distribution, ordering)` from the `TableInfo`, plus
`withQueryExecutionsCaptured`, to check that the inner write plans the required
shuffle/sort.
Minor:
- `QueryParsingErrors.distributedByPartitionWithoutPartitioning` ->
`...Error`, like the other methods there.
- `SparkSqlParser.scala:1695` adds a new use of `_LEGACY_ERROR_TEMP_0035`.
It matches the surrounding checks, but `invalidStatement`
(`INVALID_STATEMENT_OR_CLAUSE`), as used for `CREATE TEMPORARY TABLE` at
`:614`, would avoid a new legacy-condition usage.
- The operation string `... DISTRIBUTED BY/ORDERED BY`
(`WriteDistributionAndOrdering.scala:53`) is shown even when the statement only
has `UNORDERED`.
- `docs/sql-ref-syntax-ddl-create-table-datasource.md:154-156`: Spark
rejects the statement, not the catalog.
- PR description: the suite now has 33 tests, and `V2CreateTablePlan` gained
two abstract members (`writeOrdering`, `withWriteOrdering`) that implementors
in extensions also need to add.
The current CI failures look unrelated: `OracleIntegrationSuite`
(`ORA-12516`, cannot connect to the database) and the K8s `DepsTestsSuite`
"SPARK-33748: Launcher python client respecting PYSPARK_PYTHON" timeout. Could
you re-trigger CI?
--
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]
