wombatu-kun commented on code in PR #19691:
URL: https://github.com/apache/hudi/pull/19691#discussion_r3834940539
##########
hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/spark/sql/hudi/dml/schema/TestVariantDataType.scala:
##########
@@ -977,6 +978,78 @@ class TestVariantDataType extends HoodieSparkSqlTestBase {
}
}
+ test("Test Spark 3.x schema-on-read reads of a variant table with a
committed internal schema") {
+ // #18021: hoodie.schema.on.read.enable resolves the schema through the
InternalSchema round
+ // trip, whose sentinel detection restores the VARIANT logical type - the
exact input
+ // HoodieSparkSchemaConverters rejects on Spark 3.x. Verified 2026-08-20:
every leg fails
+ // LOUDLY with the same actionable error as the plain auto-resolve path;
there is no silent
+ // wrong data and no obscure secondary failure. Notably that includes the
documented
+ // struct-DDL compat mode, which works on this same table with the conf
off (pinned below)
+ // but breaks once it is on, because internal-schema resolution overrides
the user's DDL.
+ // Real support is #18285; until then Spark 3.x compat-mode readers must
keep
Review Comment:
`DefaultSource` drops the user schema entirely when the session runs on a
Hive catalog, so keeping the conf off is necessary but not sufficient - under
HMS this read throws either way. Is scoping this advice to the non-Hive catalog
worth a clause here, or is HMS out of scope for the pin?
##########
azure-pipelines-20230430.yml:
##########
@@ -80,6 +80,7 @@ parameters:
- 'org.apache.spark.sql.hudi.command'
- 'org.apache.spark.sql.hudi.common'
- 'org.apache.spark.sql.hudi.ddl'
+ - 'org.apache.spark.sql.hudi.dml.schema'
Review Comment:
The Azure sets name `dml.others`, `dml.insert` and now `dml.schema` as
leaves while the GHA DML filter names the recursive
`org.apache.spark.sql.hudi.dml`, so the next package added under `dml` starts
out dark on Azure again. Follow-up rather than a blocker, but is the leaf split
deliberate for load balancing, or worth making the Azure DML job recursive
minus `dml.insert`, which the additional Spark and SQL job already runs?
##########
hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/spark/sql/hudi/dml/schema/TestVariantDataType.scala:
##########
@@ -977,6 +978,78 @@ class TestVariantDataType extends HoodieSparkSqlTestBase {
}
}
+ test("Test Spark 3.x schema-on-read reads of a variant table with a
committed internal schema") {
+ // #18021: hoodie.schema.on.read.enable resolves the schema through the
InternalSchema round
+ // trip, whose sentinel detection restores the VARIANT logical type - the
exact input
+ // HoodieSparkSchemaConverters rejects on Spark 3.x. Verified 2026-08-20:
every leg fails
+ // LOUDLY with the same actionable error as the plain auto-resolve path;
there is no silent
+ // wrong data and no obscure secondary failure. Notably that includes the
documented
+ // struct-DDL compat mode, which works on this same table with the conf
off (pinned below)
+ // but breaks once it is on, because internal-schema resolution overrides
the user's DDL.
+ // Real support is #18285; until then Spark 3.x compat-mode readers must
keep
+ // hoodie.schema.on.read.enable off.
+ assume(HoodieSparkUtils.isSpark3, "This test verifies Spark 3.x behavior
with schema-on-read")
+
+ withTempDir { tmpDir =>
+
HoodieTestUtils.extractZipToDirectory("variant_backward_compat/variant_schema_on_read_cow.zip",
tmpDir.toPath, getClass)
+ val tablePath =
tmpDir.toPath.resolve("variant_schema_on_read_cow").toString
+
+ // The schema-on-read legs assert the same exception as the plain leg,
so they would also
+ // pass if the internal schema silently failed to load and
HoodieBaseRelation fell back to
Review Comment:
When the internal-schema load fails into `None` the auto-resolve leg falls
through to `getTableSchema` and throws the same exception, so this assert does
not stop that leg passing without the InternalSchema path - only the
conf-off/conf-on catalog-DDL pair discriminates. The COW snapshot read also
resolves through `HoodieCopyOnWriteSnapshotHadoopFsRelationFactory` rather than
`HoodieBaseRelation`, so worth narrowing both claims here.
##########
hudi-common/src/test/resources/variant_backward_compat/README.md:
##########
@@ -39,3 +39,38 @@ The test runs on these four arguments:
COW tables generated are the same for both AVRO/SPARK. But for MOR, the log
files metadata are
different. Hence, we only need to generate test files for either 1/2, 3 and 4,
hence, 3 test
resource files.
+
+# variant_schema_on_read_cow.zip
+
+A Spark 4.1-written COW variant table carrying a COMMITTED INTERNAL SCHEMA:
one insert, then a
+schema-on-read DDL, then a second insert. Used by the Spark 3.x schema-on-read
rejection test
+(#18021). Generated by running these exact statements in a Spark 4.1 SQL
session:
+
+```sql
+create table variant_schema_on_read_cow (
+ id int,
+ v variant,
+ ts long
+) using hudi
+location '/path/to/table/parent/variant_schema_on_read_cow'
+tblproperties (
+ primaryKey = 'id',
+ preCombineField = 'ts'
+);
+
+insert into variant_schema_on_read_cow select 1, parse_json('{"key": "v1"}'),
1000;
+
+set hoodie.schema.on.read.enable=true;
+alter table variant_schema_on_read_cow add columns (note string);
+insert into variant_schema_on_read_cow select 2, parse_json('{"key": "v2"}'),
1000, 'n2';
+```
+
+and zipping the table directory:
+
+```shell
+cd /path/to/table/parent/
+zip -r variant_schema_on_read_cow.zip variant_schema_on_read_cow
+```
+
+The rejection test asserts the internal schema is present, so after
regenerating, confirm
+`.hoodie/.schema/` in the zip contains completed schemacommit files.
Review Comment:
The test asserts `getTableInternalSchemaFromCommitMetadata`, which reads
`latest_schema` from the newest schema-capable commit's extra metadata rather
than from `.hoodie/.schema/`, so a regenerated fixture can pass this check and
still fail the assert. Point the check at the last commit's extra metadata
instead.
##########
hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/spark/sql/hudi/dml/schema/TestVariantDataType.scala:
##########
@@ -977,6 +978,78 @@ class TestVariantDataType extends HoodieSparkSqlTestBase {
}
}
+ test("Test Spark 3.x schema-on-read reads of a variant table with a
committed internal schema") {
+ // #18021: hoodie.schema.on.read.enable resolves the schema through the
InternalSchema round
+ // trip, whose sentinel detection restores the VARIANT logical type - the
exact input
+ // HoodieSparkSchemaConverters rejects on Spark 3.x. Verified 2026-08-20:
every leg fails
+ // LOUDLY with the same actionable error as the plain auto-resolve path;
there is no silent
+ // wrong data and no obscure secondary failure. Notably that includes the
documented
+ // struct-DDL compat mode, which works on this same table with the conf
off (pinned below)
+ // but breaks once it is on, because internal-schema resolution overrides
the user's DDL.
+ // Real support is #18285; until then Spark 3.x compat-mode readers must
keep
+ // hoodie.schema.on.read.enable off.
+ assume(HoodieSparkUtils.isSpark3, "This test verifies Spark 3.x behavior
with schema-on-read")
+
+ withTempDir { tmpDir =>
+
HoodieTestUtils.extractZipToDirectory("variant_backward_compat/variant_schema_on_read_cow.zip",
tmpDir.toPath, getClass)
+ val tablePath =
tmpDir.toPath.resolve("variant_schema_on_read_cow").toString
+
+ // The schema-on-read legs assert the same exception as the plain leg,
so they would also
+ // pass if the internal schema silently failed to load and
HoodieBaseRelation fell back to
+ // the commit-metadata schema. Pin that the fixture carries a loadable
internal schema, so
+ // those legs cannot pass without exercising the InternalSchema path.
+ val schemaResolver = new TableSchemaResolver(createMetaClient(spark,
tablePath))
+ assert(schemaResolver.getTableInternalSchemaFromCommitMetadata.isPresent,
+ "fixture must carry a committed internal schema; regenerate it per the
README")
+
+ def assertVariantRejected(leg: String)(f: => Unit): Unit = {
+ val ex = intercept[HoodieSchemaException](f)
+ assert(ex.getCause.getMessage.contains("VARIANT type is only supported
in Spark 4.0+"),
+ s"[$leg] expected the actionable variant rejection, got:
${ex.getCause}")
+ }
+
+ assertVariantRejected("auto-resolve, plain") {
+ spark.read.format("hudi").load(tablePath).collect()
+ }
+ assertVariantRejected("auto-resolve, schema-on-read") {
+ spark.read.format("hudi").option("hoodie.schema.on.read.enable",
"true").load(tablePath).collect()
+ }
+
+ val tableName = generateTableName
+ spark.sql(
+ s"""
+ |create table $tableName (
+ | id int,
+ | v struct<value: binary, metadata: binary>,
+ | ts long,
+ | note string
+ |) using hudi
+ |location '$tablePath'
+ |tblproperties (
+ | primaryKey = 'id',
+ | preCombineField = 'ts'
+ |)
+ """.stripMargin)
+ try {
+ // With the conf off the compat struct DDL works even though the table
carries an
+ // internal schema - pinning that the conf, not the committed internal
schema itself,
+ // is what breaks compat mode.
+ val rows = spark.sql(s"select id, v, note from $tableName order by
id").collect()
+ assert(rows.map(r => (r.getInt(0), r.isNullAt(1),
r.getString(2))).toSeq
Review Comment:
This leg checks only `isNullAt` on the variant column, so it would pass on
swapped or structurally wrong `value`/`metadata` bytes even though the header
claims no silent wrong data. `verifyVariantBackwardCompatibility` pins the
exact variant bytes for the other fixtures - worth the same here.
--
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]
