wombatu-kun commented on code in PR #19691:
URL: https://github.com/apache/hudi/pull/19691#discussion_r3840254752
##########
hudi-spark-datasource/hudi-spark/src/test/java/org/apache/hudi/TestSparkSqlHudiPackageStructure.java:
##########
@@ -89,6 +98,90 @@ public void
testSparkSqlHudiScalaTestClassesInAllowedPackagesOnly() {
"Expected to find at least one Scala test class in " + BASE_PACKAGE);
}
+ /**
+ * Every Scala test class under {@link #BASE_PACKAGE} must be named by at
least one Azure
+ * wildcardSuites entry, otherwise it silently never runs on Azure.
+ *
+ * <p>The Azure jobs deliberately name leaf packages ({@code dml.others},
{@code dml.insert},
+ * {@code dml.schema}) rather than the recursive {@code dml} parent, because
ScalaTest's
+ * {@code -w} is a plain prefix match with no exclusion primitive: pointing
one job at
+ * {@code ...hudi.dml} would re-run the whole {@code dml.insert} set that
already has its own
+ * job. That split is what makes a newly added {@code dml.*} package start
out dark, so this
+ * test is the guard for it - the other, non-recursive {@code
testSparkSqlHudi...} check above
+ * lets {@code dml.*} through because it treats {@code dml} as one allowed
package.
+ */
+ @Test
+ public void testScalaTestPackagesAreCoveredByAzureWildcardSuites() {
Review Comment:
This guard is in neither the description's Changelog nor its Impact section,
which still reads "None - test-only, plus one Azure CI wildcard addition" even
though it now fails hudi-spark UT for anyone adding a Scala package under
`org.apache.spark.sql.hudi`. Add it to both.
##########
hudi-spark-datasource/hudi-spark/src/test/java/org/apache/hudi/TestSparkSqlHudiPackageStructure.java:
##########
@@ -89,6 +98,90 @@ public void
testSparkSqlHudiScalaTestClassesInAllowedPackagesOnly() {
"Expected to find at least one Scala test class in " + BASE_PACKAGE);
}
+ /**
+ * Every Scala test class under {@link #BASE_PACKAGE} must be named by at
least one Azure
+ * wildcardSuites entry, otherwise it silently never runs on Azure.
+ *
+ * <p>The Azure jobs deliberately name leaf packages ({@code dml.others},
{@code dml.insert},
+ * {@code dml.schema}) rather than the recursive {@code dml} parent, because
ScalaTest's
+ * {@code -w} is a plain prefix match with no exclusion primitive: pointing
one job at
+ * {@code ...hudi.dml} would re-run the whole {@code dml.insert} set that
already has its own
+ * job. That split is what makes a newly added {@code dml.*} package start
out dark, so this
+ * test is the guard for it - the other, non-recursive {@code
testSparkSqlHudi...} check above
+ * lets {@code dml.*} through because it treats {@code dml} as one allowed
package.
+ */
+ @Test
+ public void testScalaTestPackagesAreCoveredByAzureWildcardSuites() {
+ Set<String> azurePrefixes = readAzureWildcardSuitePrefixes();
+ assertFalse(azurePrefixes.isEmpty(),
+ "Expected to parse at least one wildcardSuites entry from " +
AZURE_PIPELINE_FILE);
+
+ List<String> uncovered = findScalaTestClasses().stream()
+ .filter(className -> azurePrefixes.stream()
+ .noneMatch(prefix -> className.equals(prefix) ||
className.startsWith(prefix + ".")))
Review Comment:
This proves a package is named by some wildcardSuites entry, not that a job
runs it: these jobs also pass `-pl $(JOB3456_MODULES)`, and nothing ties a
parsed `*WildcardSuites` parameter back to a step that expands it. Worth
restricting this check to the modules those jobs build and resolving the
`$(VAR)` tokens back to the parameter they name - follow-up, not a blocker.
##########
hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/spark/sql/hudi/dml/schema/TestVariantDataType.scala:
##########
@@ -977,6 +978,107 @@ 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 - necessary, but only sufficient off a
Hive catalog:
+ // DefaultSource discards the user schema outright when
isUsingHiveCatalog, so under HMS the
+ // struct DDL below is never seen and the read throws with the conf off
too. This test runs
+ // on the in-memory catalog, so it pins the non-Hive half only.
+ 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
+
+ // HoodieBaseHadoopFsRelationFactory (reached here via
+ // HoodieCopyOnWriteSnapshotHadoopFsRelationFactory, not
HoodieBaseRelation) swallows an
+ // internal-schema load failure into None and then falls back to
getTableSchema, which
+ // throws the same exception - so neither auto-resolve leg can, on its
own, prove the
+ // InternalSchema path ran. This assert only pins that the fixture still
carries a loadable
+ // internal schema; the conf-off/conf-on catalog-DDL pair below is what
discriminates,
+ // because with a DDL present the None fallback resolves to the user
schema and succeeds.
+ val schemaResolver = new TableSchemaResolver(createMetaClient(spark,
tablePath))
+ assert(schemaResolver.getTableInternalSchemaFromCommitMetadata.isPresent,
Review Comment:
The description's parenthetical - that this assert stops the schema-on-read
legs passing via the commit-metadata fallback - says the opposite of the
comment above it, which has the relation swallowing its own load failure into
`None` and falling back to `getTableSchema` anyway. Drop it from the
description.
--
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]
