voonhous commented on code in PR #20193:
URL: https://github.com/apache/hudi/pull/20193#discussion_r4184858694
##########
hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/spark/sql/hudi/dml/schema/TestVariantShreddingMixedLayouts.scala:
##########
@@ -2321,6 +2319,31 @@ class TestVariantShreddingMixedLayouts extends
HoodieSparkSqlTestBase with Varia
}
}
+ test("Variant read does not mutate the session vectorized reader
configuration") {
+ assume(HoodieSparkUtils.gteqSpark4_1, SPARK_4_1_GATE)
+
+ withVariantTable(
+ "vectorized reader session conf",
+ "cow",
+ recordTypes = Seq(HoodieRecordType.SPARK)) { (tableName, _, _) =>
+
+ spark.sql(
+ s"""insert into $tableName values (1, parse_json('{"key":"v1"}'),
1000)""")
+
+ withSQLConf("spark.sql.parquet.enableVectorizedReader" -> "true") {
+ assert(spark.conf.get("spark.sql.parquet.enableVectorizedReader") ==
"true")
Review Comment:
**nit:** This assert runs directly inside `withSQLConf(... -> "true")`, so
it can't fail; only the post-read assert catches the bug. Feel free to ignore,
but could we drop it?
```suggestion
```
##########
hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/spark/sql/hudi/dml/schema/TestVariantShreddingMixedLayouts.scala:
##########
@@ -942,11 +942,9 @@ class TestVariantShreddingMixedLayouts extends
HoodieSparkSqlTestBase with Varia
checkNestedExceptionContains(
() => spark.sql(s"select id, cast(v as string), note from
$tableName").collect())(
"pushVariantIntoScan")
- // The guard's empty-projection carve-out: count(*) reads no column
data, and the query
- // schema it would be checked against is the UNPRUNED table schema,
variant included, so
- // the guard must not run at all. Drop the requiredSchema.nonEmpty
gate at any of its five
- // sites and this count fails on the shredded file.
- checkAnswer(s"select count(*) from $tableName")(Seq(1))
+ // count(*) is deliberately not pinned: it fails on any schema-on-read
variant table once
+ // an internal schema is committed (#20139). Previously this stayed
green only because #20140
+ // leaked the query-local vectorization fallback into the Spark
session.
Review Comment:
**major:** Deleting this pin leaves the `requiredSchema.nonEmpty`
empty-projection carve-out (`ParquetSchemaEvolutionUtils.scala:90`) with no
test on the file-group-reader path; the old comment said this count is what
fails if that gate is dropped. The guard runs on the row-based path too, and
#20139 notes the count passes with `enableVectorizedReader=false`. Could we
keep the pin on the row-based reader, as below? The comment at L1045-1047 also
still says this pin is "green only through #20140"; could that be updated in
the same pass?
```suggestion
// Empty-projection carve-out: count(*) reads no column data, so the
guard must not run.
// Pinned on the row-based reader only; the vectorized arm fails on
#20139.
withSQLConf("spark.sql.parquet.enableVectorizedReader" -> "false") {
checkAnswer(s"select count(*) from $tableName")(Seq(1))
}
```
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/spark/sql/execution/datasources/parquet/HoodieFileGroupReaderBasedFileFormat.scala:
##########
@@ -346,8 +346,8 @@ class HoodieFileGroupReaderBasedFileFormat(tablePath:
String,
val dataStructTypeWithMandatoryPartitionFields =
StructType(dataStructType.fields ++ partitionSchema.fields.filter(f =>
mandatoryFields.contains(f.name) && !isNestedPartitionField(f.name)))
val dataSchema = HoodieSchemaUtils.pruneDataSchema(schema,
HoodieSchemaConversionUtils.convertStructTypeToHoodieSchema(dataStructTypeWithMandatoryPartitionFields,
sanitizedTableName), exclusionFields)
-
spark.sessionState.conf.setConfString("spark.sql.parquet.enableVectorizedReader",
supportVectorizedRead.toString)
-
+ // Keep vectorization query-local; mutating the session configuration here
leaks this
+ // reader's fallback into subsequent queries.
Review Comment:
**nit:** This comment describes the deleted line rather than the code that
remains. Feel free to ignore, but would this wording work?
```suggestion
// The vectorization decision reaches the reader only through
buildBaseFileReader's
// supportVectorizedRead argument; never write it to the session conf.
```
##########
hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/spark/sql/hudi/dml/schema/TestVariantShreddingMixedLayouts.scala:
##########
@@ -2321,6 +2319,31 @@ class TestVariantShreddingMixedLayouts extends
HoodieSparkSqlTestBase with Varia
}
}
+ test("Variant read does not mutate the session vectorized reader
configuration") {
Review Comment:
**minor:** This test only runs on Spark 4.1+, but the leak happens on every
version: incremental and bootstrap reads set `supportVectorizedRead=false` at
`HoodieFileGroupReaderBasedFileFormat.scala:227`, and PR CI runs only spark3.5
and spark4.2. Not blocking, but could we add a twin in
`TestIncrementalReadWithFileGroupReader` that does a COW incremental read under
`enableVectorizedReader=true` and asserts the conf afterwards? A plain MOR
snapshot won't trigger it, since it keeps `supportVectorizedRead=true`.
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/spark/sql/execution/datasources/parquet/HoodieFileGroupReaderBasedFileFormat.scala:
##########
@@ -346,8 +346,8 @@ class HoodieFileGroupReaderBasedFileFormat(tablePath:
String,
val dataStructTypeWithMandatoryPartitionFields =
StructType(dataStructType.fields ++ partitionSchema.fields.filter(f =>
mandatoryFields.contains(f.name) && !isNestedPartitionField(f.name)))
val dataSchema = HoodieSchemaUtils.pruneDataSchema(schema,
HoodieSchemaConversionUtils.convertStructTypeToHoodieSchema(dataStructTypeWithMandatoryPartitionFields,
sanitizedTableName), exclusionFields)
-
spark.sessionState.conf.setConfString("spark.sql.parquet.enableVectorizedReader",
supportVectorizedRead.toString)
-
+ // Keep vectorization query-local; mutating the session configuration here
leaks this
Review Comment:
**minor:** `HoodieBaseRelation.imbueConfigs` (L397-400) has the same leak:
it writes `spark.sql.parquet.filterPushdown=true` and
`recordLevelFilter.enabled=true` into the session, overriding a user's `false`.
It runs on every incremental relation and on MDT reads. Not blocking, but would
it be worth dropping those two `setConfString` calls in this PR too, since it
is the same fix?
--
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]
