spark-test commented on code in PR #59734:
URL: https://github.com/apache/spark/pull/59734#discussion_r4355413135
##########
sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveExternalCatalog.scala:
##########
@@ -270,6 +271,14 @@ private[spark] class HiveExternalCatalog(conf: SparkConf,
hadoopConf: Configurat
hiveTable.withNewStorage(locationUri = tableLocation),
ignoreIfExists)
} else {
+ // Hive incompatible partition column types are stored with a
placeholder type in Hive
+ // metastore (see `HiveClientImpl.toHiveTable`), which only data source
tables can restore.
+ schemaWithNoCollation
+ .filter(f => tableDefinition.partitionColumnNames.contains(f.name))
+ .find(f => !isHiveCompatibleDataType(f.dataType))
+ .foreach { f =>
+ throw
QueryCompilationErrors.dataTypeUnsupportedByDataSourceError("Hive", f)
Review Comment:
**Non-blocking (P2):** This check runs too late for `saveAsTable`
overwrites. Take an existing table `t` and
`df.write.format("hive").mode("overwrite").partitionBy("pt").saveAsTable("t")`,
where `pt` is `TIMESTAMP_NTZ`, `TIME`, a nanosecond timestamp, or an ANSI
interval. `SaveAsV1TableCommand` plans a `CreateHiveTableAsSelectCommand`, and
the partition column passes analysis: `PreprocessTableCreation` only applies
`canPartitionOn` to partition columns, and `DDLUtils.checkTableColumns` sees
only the data columns. `CreateHiveTableAsSelectCommand.run` then drops `t`
(line 72) and only afterwards calls `catalog.createTable` (line 81, outside the
write's try/catch), which reaches this `throw`. The user gets
`UNSUPPORTED_DATA_TYPE_FOR_DATASOURCE`, but `t`, and for a managed table its
data, is already gone.
That contradicts the SPARK-60387 overwrite contract in
`SaveAsV1TableCommand` ("drops the table as late as it can, so that a write
that fails its checks keeps the table"), which `MetastoreDataSourcesSuite` pins
for the Hive format. This rejection depends only on the query schema and the
partition column names, so it can run before the drop. The merge target also
lost `t` on this input (it replaced it with a broken table), so this isn't a
new silent data loss. Still, the new failure now lands after the destructive
step instead of before it. Only the overwrite of an existing table is affected;
the creation forms the new test covers all target tables that don't exist yet,
and they are rejected before anything is created.
**Recommended change:** Move the Hive serde partition-type check into a
shared validator on the HiveExternalCatalog companion object and keep calling
it from HiveExternalCatalog.createTable. In CreateHiveTableAsSelectCommand.run,
compute the new table's schema before the existing-table drop and run the same
validator on that schema and the table's partition columns before dropping
anything. Add Hive DDL test coverage that overwrites an existing table through
saveAsTable with an incompatible partition column.
**Why this works:** The validator applies the same collation stripping,
exact partition-name selection, and Hive compatibility predicate that decide
whether toHiveTable writes a placeholder partition type. Because the CTAS
command now runs it before catalog.dropTable, a rejected overwrite throws
before any catalog or data mutation. This restores the saveAsTable overwrite
guarantee that a write which fails its checks keeps the existing table.
createTable continues to reject the same definitions for producers that perform
no prior drop.
**Scope:** Share the Hive serde partition-type validation and run it before
the Hive CTAS overwrite drops the existing table.
**Compatibility:** Hive serde tables with Hive compatible partition types,
and data source tables with any of these partition types, keep being created
and overwritten exactly as before.
**Risks:** Computing the CTAS table schema before the drop must yield
exactly the schema later passed to catalog.createTable; reuse one computed
value for both rather than deriving it twice.
**Constraints:** Keep the rejected type domain identical to the
HiveClientImpl.toHiveTable placeholder condition, using the same collation
stripping, exact partition-name match, and compatibility predicate, so no
partition type is rejected on one path and accepted on another.
**Exclusions:**
- Existing tables already stored with placeholder partition column types
stay unchanged, as the PR declares; this repair only moves the rejection
earlier for new definitions.
**Success:** Overwriting an existing table through
DataFrameWriter.format("hive").mode("overwrite").partitionBy(...).saveAsTable
with a TIMESTAMP_NTZ, ANSI interval, nanosecond timestamp, or TIME partition
column fails with UNSUPPORTED_DATA_TYPE_FOR_DATASOURCE, and the existing table
still exists with its original schema and rows. Every Hive serde creation form
that the PR already rejects still fails with the same error and leaves no table
behind. Data source tables with these partition types, and Hive serde tables
with Hive compatible partition types, are still created and overwritten as
before.
**Verification:**
- **Regression:** Overwriting an existing Hive table through saveAsTable
with each Hive incompatible partition type fails with
UNSUPPORTED_DATA_TYPE_FOR_DATASOURCE and leaves the original table and its rows
readable.
- **Behavior:** The existing creation coverage (STORED AS, USING HIVE, CTAS,
saveAsTable into a new table, CREATE TABLE LIKE with STORED AS, and TIMESTAMP
under the TIMESTAMP_NTZ session default) still reports the same error
parameters with no table left behind.
- **Compatibility:** A Hive CTAS or saveAsTable overwrite with Hive
compatible partition types still replaces the table, and data source tables
with these partition types are still accepted.
--
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]
