voonhous commented on PR #19687: URL: https://github.com/apache/hudi/pull/19687#issuecomment-5355016580
Pushed d8ffd05a107c for a second review round. Short version: the guards this PR added had bypasses, and a few of the new tests could not fail for the reason they exist. This commit closes both. **Production** | Area | Gap | Fix | |---|---|---| | Schema-on-read guard | Only in `ParquetSchemaEvolutionUtils`; the four `*LegacyHoodieParquetFileFormat` copies of the same merge block were unguarded, and `count(*)` on a shredded table threw (empty projection = unpruned query schema) | Guard hoisted to a shared helper called from all five sites, recursive into nested types, skipped for empty projections | | Hive guard | Top-level columns only (the row writer shreds nested variants too), and bypassed whenever `shouldUseFilegroupReader` is false: fgr disabled, schema-on-read enabled, bootstrap splits | Detection via the recursive `toShreddedReadSchema`; the legacy `MapredParquetInputFormat` fall-through gets its own fail-fast, gated so non-variant tables never pay a footer read | | Sort-column validation | Bypassed by consistent-bucket clustering, plain `bulk_insert` sort columns and the java client; case-sensitive, so `sort.columns=V` against column `v` slipped through; MAP missing, and "has no ordering" was wrong for BLOB/VECTOR on the row path | One `SortUtils.validateSortableColumns` (case-insensitive, VARIANT/MAP/BLOB/VECTOR, message "Sorting by column ... is not supported") in the partitioner constructors, with the early strategy/procedure checks kept for a cleaner error | | bot.yml | The adapter suites are JUnit classes: `wildcardSuites` cannot discover them and the java UT lanes already run them, so the hunk was a no-op and its "never ran in CI" claim wrong | Reverted | **Tests** | Change | Why | |---|---| | Nested fail-fast legs (Hive + schema-on-read) and a vectorized-reader sweep on the nested row-writer test | Nested paths are exactly what a top-level-only guard misses; #18605 history for the vectorized leg | | BLOB/VECTOR/MAP rejection pinned in `TestClusteringProcedure`, no Spark 4.1 gate | The old pin sat behind `gteqSpark4_1`, leaving the validation untested on the 3.5 lane | | Incremental, RO and time-travel legs assert values; the default `pushVariantIntoScan` leg asserts a variant-related failure | Count-only asserts and a bare `intercept[Throwable]` stay green even when `v` reads back all-null | | Flink pin asserts "Shredded Variant is not supported in Flink" | Any unrelated error mentioning the column used to green it | | CDC layout-flip folded into `TestVariantDataType`'s CDC test; ordering-modes test folded into the three-layout compaction test (its delete-block leg survives); unshredded clustering twin retired | Near-duplicates; each unique leg was kept | | Dead `Inferred` scaffolding pruned; `createVariantTable` and a chain-walking `checkNestedExceptionContains(runnable)` shared | Unused members; the reflective inference seam itself stays for #18961 | Deliberately left as follow-ups: the flink 1.18-2.0 copies of `ParquetSplitReaderUtil` carry no shredded guard (only 2.1 does, and only 2.1 is in CI), and the Trino reader context has the same silent-null exposure the Hive guard closes. PR description updated to match (including that the spark4.0 lanes are `[CI-TRIM]`-commented, so the 4.0 leg is local-run coverage only). -- 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]
