nsivabalan commented on PR #19205: URL: https://github.com/apache/hudi/pull/19205#issuecomment-5216102901
Pushed 5 commits addressing @danny0405's and @voonhous's feedback, plus the two `hudi-agent` findings. PR description rewritten — it was describing an earlier design (comma-separated tokens, a `parseMetaFieldsMode` helper that no longer exists), and it now documents the config interplay explicitly, which was the main thing asked for. ### The headline change: the mode is a table property `hoodie.meta.fields.mode` now changes **only** at table creation, through hudi-cli (#19206), or by an upgrade — never as a side effect of a write. One rule, in `BaseHoodieWriteClient#validateAgainstTableProperties`, for every engine: | Writer states | Behavior | |---|---| | neither meta-field property | **inherits** the table's mode; no comparison | | either, matching | accepted | | either, wider | rejected | | either, narrower or sibling | rejected | Inheritance is what makes the strict rule safe: table services and a restarted `HoodieStreamer` build write configs without restating meta-field settings, so they inherit rather than being rejected. This let me delete `StreamSync`'s own back-fill (`4a7fa0c`) — it was redundant and, worse, keyed only on the mode key, so an explicitly-passed legacy boolean would have silently inherited instead of conflicting. The metadata table needs no exemption: `HoodieMetadataWriteUtils` builds a fresh config with `populate.meta.fields=false` and its own table resolves to `NONE`, so the two match. `TestSparkRDDMetadataWriteClient` passes, and there is now a test asserting it with MDT enabled. ### Three read-path bugs, found by finally writing the incremental test @voonhous was right that this was the highest-value missing test, and right that the read path was load-bearing and untested. Writing it surfaced two live bugs plus one gap: 1. **The MoR guard was rejecting every selective CoW table.** `HoodieCopyOnWriteIncrementalHadoopFsRelationFactory` builds a `MergeOnReadIncrementalRelation` to back its file index, so the guard fired on the entire feature. Scoped to `MERGE_ON_READ`; `TestHoodieSparkMergeOnReadTableIncrementalRead` still passes. 2. **The CoW commit-time guard was on an unreachable path.** `IncrementalRelationV1/V2` back only the streaming source, so a datasource read of a `FILE_NAME_ONLY`/`NONE` table returned **zero rows silently** — the exact "no new data" ambiguity the guard exists to prevent. Added to the factory the datasource actually uses. 3. `_hoodie_file_name` was stamped on records copied forward during upserts (the `hudi-agent` finding on `HoodieAvroParquetWriter`). Fixed in `HoodieWriteMergeHandle` **and** `BaseCreateHandle` — the second site nothing had flagged, and which the new upsert test caught immediately after the first fix looked complete. ### Behavior changes needing release notes Three, up from one. Called out in the description under an explicit sign-off heading: 1. Row-writer meta-column stubs are now SQL `NULL` instead of `""`, including on **existing** `populate.meta.fields=false` tables. Required for the columns to be Parquet `OPTIONAL`. 2. **`hoodie.populate.meta.fields=false` against an `ALL` table now throws** where it previously narrowed the write silently (HUDI-2161 / `d5026e9a2485`). This is the one change that can fail a working pipeline. Only when *explicitly set* — a writer that says nothing inherits, which is why table services are unaffected. 3. Downgrading a table on a selective mode now **fails** rather than degrading to `NONE` with a warning. That removes the one-way lossy path @voonhous flagged: the unrecoverable state is now unreachable rather than documented. `ALL`/`NONE` round-trip unchanged. Risk level raised low → medium on account of #2. ### One pre-existing bug found, filing separately `count()` and column-projected reads return **0 rows** on a CoW incremental query while `collect()` returns the correct rows. I initially thought this was mine; it is not. It reproduces on unmodified `master` at table version 9 on a **default `ALL` table**: ``` MASTER_PROBE tableVersion=NINE collect=2 count=0 selectCol1=0 ``` Cause looks to be `HoodieFileGroupReaderBasedFileFormat` routing column-less incremental scans to `readBaseFile`, which pushes a `_hoodie_commit_time` predicate into Parquet without projecting that column (`isCount` is forced false for incremental at `:253`, and the guard at `:304` then falls through). I have not fixed it here — it is on the shared read path for every Spark query and deserves its own review — but it is a silent-wrong-answer bug affecting all users today. The tests in this PR read whole rows to avoid it. ### Two notes for reviewers - `HoodieTableConfig:352` and `HoodieWriterUtils.scala:380` direct users to hudi-cli, but `set-meta-fields-mode` lives in #19206, not this tree. Messages now cite recreation/upgrade as the in-tree paths. Worth deciding whether #19206 lands first. - `HoodieParquetBinaryCopyBase:131` still masks `_hoodie_file_name` unconditionally (@voonhous's summary item 1). The row-writer clustering path is now tested, but threading the mode into the binary copier needs a constructor change through `HoodieParquetFileBinaryCopier`; flagging as a follow-up rather than widening this PR further. Binary-copy clustering is not exercised by the PR's tests either way. Test counts: `TestMetaFieldsModeE2E` 27/27, `TestBaseHoodieWriteClient` 110/110, `TestHoodieTableConfig` + resolution 107/107, upgrade/downgrade 84/84. checkstyle and scalastyle clean across all six touched modules. -- 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]
