Raviston6296 commented on PR #60116:
URL: https://github.com/apache/spark/pull/60116#issuecomment-6652802062
Thanks for the fix, the change looks right to me. I checked that the Hive
metastore keeps the table default collation, as the `collation` table property
in `HiveClientImpl.toHiveTable`, and reads it back into
`CatalogTable.collation`. So copying the field also works for the Hive catalog,
not just the in-memory one.
A few suggestions:
1. The new tests are in `InMemoryCatalogedDDLSuite`, so they only run
against the in-memory catalog. `HiveDDLSuite` also extends `DDLSuite`, and Hive
persists the collation through a separate path (a table property). Putting the
tests in `DDLSuite` would cover both catalogs for no extra code.
2. The `CreateTableLikeCommand` scaladoc lists the attributes copied from
the source ("storage(...), schema, provider, partitionColumnNames, bucketSpec
by default"). Could you add the default collation there?
3. `getTempViewOrPermanentTableMetadata` can also return a view, so `CREATE
TABLE t LIKE v` now copies a view's `DEFAULT COLLATION` into the table's
default collation. That seems reasonable, since the view's string columns
already carry it, but is it intended? If so, a test with a view created with
`DEFAULT COLLATION UTF8_LCASE` as the source would make it explicit.
4. Not for this PR: the V2 path (`CreateTableLikeExec`, used when the source
or target is in a non-session catalog) leaves source properties out of
`tableInfo`, and `collation` is one of `TABLE_RESERVED_PROPERTIES`, so it can't
be set through `TBLPROPERTIES` either. So the default collation is only kept if
the connector copies `PROP_COLLATION` from `sourceTable.properties()` itself.
Since the collation is a Spark-level property rather than connector-specific
state, should `CreateTableLikeExec` pass it in `tableInfo`, as it does for
columns and constraints? That could be a follow-up.
5. Nit: `withSQLConf(SQLConf.USE_V1_SOURCE_LIST.key -> "parquet")` looks
unnecessary, since `parquet` is already in the default V1 source list.
--
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]
