test(spark): cover the legacy parquet read path with file-group reader disabled#19133
test(spark): cover the legacy parquet read path with file-group reader disabled#19133yihua wants to merge 2 commits into
Conversation
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! This PR adds TestLegacyParquetReadPath, a COW functional suite that exercises the previously-uncovered legacy Spark read path (BaseFileOnlyRelation, IncrementalRelationV1/V2, and the per-version legacy parquet file format) by constructing the legacy relations directly with the file-group reader disabled and comparing results row-by-row against the file-group-reader path plus independently computed expected values. I traced the expected counts for the snapshot, filter-pushdown, and incremental cases and they hold up, and the per-branch coverage (vectorized/non-vectorized, relation vs. file-format conversion, V1 vs. V2) looks well-targeted. No correctness issues found. A few style/readability suggestions in the inline comments. Please take a look, and this should be ready for a Hudi committer or PMC member to take it from here. One small maintainability nit below — the magic positional index in collectSorted; everything else looks clean and well-commented.
cc @yihua
| private def collectSorted(df: DataFrame): Seq[Row] = | ||
| df.select(comparedCols.map(col): _*).collect().toSeq.sortBy(_.getString(2).toInt) | ||
|
|
||
| private def assertSameRows(expected: DataFrame, actual: DataFrame): Unit = { |
There was a problem hiding this comment.
🤖 nit: the magic index 2 silently couples this sort to the current position of "id" in comparedCols — could you use _.getAs[String]("id").toInt instead so it stays correct if comparedCols is ever reordered?
There was a problem hiding this comment.
DonecollectSorted now sorts by getAs[String]("id") instead of the positional index.
| spark = null | ||
| } | ||
|
|
||
| private def makeRows(ids: Seq[Int], ts: Long, valueFn: Int => Long): Seq[(String, Long, Long, String)] = |
There was a problem hiding this comment.
The row shape is entirely scalar (id, ts, value, partition), so no case here exercises the legacy parquet format's vectorized nested-column read - historically the most fragile branch of these per-version copies (the nested-column vectorized-read bug fixed in HUDI-7190 / #10265). A regression re-introduced there would still ship silently, which is exactly the blind spot this suite is meant to close. Consider adding a struct (or array) column to the schema so the vectorized nested path is covered too; the existing cases only hit flat columns.
There was a problem hiding this comment.
+1, this is the most valuable case to add. The whole point of the suite is to catch a legacy per-version parquet-read regression, and the vectorized nested-column path (HUDI-7190 / #10265) is where those have historically lived. Adding a struct (and ideally an array) column to makeRows/the schema, exercised under both the vectorized and the non-vectorized branches you already toggle, would close that gap. The flat-column cases here wouldn't have caught #10265.
There was a problem hiding this comment.
Addressed! Added a nested struct and a tags array to the shared row shape, so every snapshot case now drives the vectorized nested-column read, and testCowSnapshotReadWithoutVectorizedReader covers the row-based branch. The nested/array values are derived from value/ts, so upserted rows are compared with changed nested content too, not just inserts.
… columns Address review feedback on apache#19133: - Add a nested struct and an array column so the legacy parquet reader is driven through its complex-type branch (the vectorized nested-column path, historically fragile per HUDI-7190/apache#10265), not just flat scalar columns. - Sort compared rows by the named 'id' column instead of a positional index.
|
@wombatu-kun Addressed your comment here, can you please do another round of review? Thank you! |
|
|
||
| /** Row shape written by these tests. A nested struct and an array are included so the legacy | ||
| * parquet read path is exercised on complex types -- the historically fragile vectorized | ||
| * nested-column branch (e.g. HUDI-7190), not just flat scalar columns. */ |
There was a problem hiding this comment.
On the spark3.3 profile these columns switch vectorization off rather than on. None of the SparkXXLegacyHoodieParquetFileFormat shims override supportBatch any more (HUDI-7190 removed the AtomicType-only override), so they inherit Spark's, which gates struct/array support on spark.sql.parquet.enableNestedColumnVectorizedReader. That config defaults to false in Spark 3.3 and true only from 3.4 on, so with nested/tags in the schema supportBatch returns false on 3.3, enableVectorizedReader is false, and the read falls back to parquet-mr. The vectorized nested-column branch that HUDI-7190 fixed for spark33 stays uncovered, testCowSnapshotReadEqualsFileGroupReader and testCowSnapshotReadWithoutVectorizedReader collapse onto the same row-based path there, and the flat-scalar schema that preceded this commit did vectorize on 3.3 - so this is a net coverage loss on that leg. The suite does run there: test-spark-java-tests-part1 in bot.yml runs -Punit-tests over hudi-spark on spark3.3.
Set spark.sql.parquet.enableNestedColumnVectorizedReader=true for the vectorized cases so the branch is exercised on every profile, and consider asserting that the format reports supportBatch true there, so a change of Spark's default cannot silently drop the coverage again.
5 participants
Describe the issue this Pull Request addresses
The legacy (pre-file-group-reader) Spark read path has zero automated coverage: the per-Spark-version
SparkXXLegacyHoodieParquetFileFormatcopies (~245 lines each across the six version modules) andBaseFileOnlyRelation,IncrementalRelationV1,IncrementalRelationV2,IncrementalRelationUtilinhudi-spark-commonall report 0% on Codecov. These classes still run in production:DefaultSource#resolveBaseFileOnlyRelationusesBaseFileOnlyRelationfor metadata-table reads, andHoodieStreamSourceV1/V2pickIncrementalRelationV1/V2whenhoodie.file.group.reader.enabledis false. A regression there currently ships silently.Note on routing: the batch
DefaultSource.createRelationpath no longer consultshoodie.file.group.reader.enabledfor normal snapshot/incremental reads; the legacy relations are reachable via the streaming sources and metadata-table reads. The tests therefore construct the legacy relations directly with the flag disabled in their options, matching the streaming-source invocation.Summary and Changelog
Add
TestLegacyParquetReadPath(hudi-spark functional suite, COW, <=30 rows per table) asserting sorted row-level equality between the legacy path and the file-group-reader path on the same table, plus independently computed expected values so both paths cannot be equally wrong:BaseFileOnlyRelation.buildScanand itstoHadoopFsRelationconversion (asserting the relation executes the legacy Hudi parquet file format).spark.sql.parquet.enableVectorizedReader=false.hoodie.datasource.read.extract.partition.values.from.path.IncrementalRelationV1(instant-time start) andIncrementalRelationV2(completion-time start,OPEN_CLOSED), compared against the file-group-reader incremental result.Since the suite lives in the shared
hudi-sparkmodule functional package, each CI matrix leg exercises its own Spark version's legacy file format copy.Impact
Test-only; no production code change. Closes the coverage blind spot on the legacy read path (six version-module file format copies plus the legacy relations in
hudi-spark-common).Risk Level
low
Documentation Update
None.
Contributor's checklist