nsivabalan commented on PR #19205:
URL: https://github.com/apache/hudi/pull/19205#issuecomment-5016555160
Rebased onto latest master (2026-07-19) and folded the CI-fix tweaks into
their natural parent commits. Summary of what changed since last CI:
**Test fixes bundled into existing commits:**
- `TestHoodieTableConfig.testDefinedTableConfigs` — bumped expected size
from 45 → 46 to account for the new `hoodie.meta.fields.mode` config. Folded
into the feat commit.
- `TestHoodieSparkSqlWriter.testBulkInsertForPopulateMetaFields` /
`testDisableAndEnableMetaFields` (plus the `WithTestFormat` mirror) — updated
the null-vs-empty-string assertion. With the nullable meta-column stub fix, the
columns now materialize as `null` instead of `""`; the assertion was checking
`Row.mkString(",") != ""` which returned `"null"` for a null cell. Switched to
`!entry.isNullAt(0) && entry.getString(0).nonEmpty`, which matches the same
pattern used elsewhere in the file. Folded into the row-writer stub-nullable
fix commit.
- `TestHoodieStreamerMetaFieldsMode` — the parquet glob was `*/*.parquet`
but `HoodieTestDataGenerator`'s default partitions are `YYYY/MM/DD` (three
levels). Updated glob to `*/*/*/*.parquet`. Folded into the refactor commit
that introduced the test.
**Ignored flake:**
- `TestIncrementalQueries.testIncrementalQueryWithMultiCommitsInSameFile[3]`
failed once ("Got an invalid instant") and passed on retry (`Run 2: PASS`).
Looks like a timestamp-generation edge case unrelated to this PR.
**Self-review discussion points — flagging for reviewer input rather than
force-committing:**
1. **Selective-mode column-population is repeated in three writer paths** —
`HoodieRowCreateHandle`, `HoodieSparkParquetWriter`, `HoodieAvroParquetWriter`
each open-code `if (mode.isCommitTimePopulated()) { ... } if
(mode.isFileNamePopulated()) { ... }`. It's three lines of trivial logic per
site, but if the enum grows another selective column, all three drift. Options:
(a) leave as-is (perf-hot path, no shared context), (b) add a
`SelectiveMetaFieldsWriter#applyTo(record, commitTime, fileName)` helper on the
enum, (c) push the logic into a `MetaFieldPopulator` strategy. I'm inclined to
(a) with a `// mirror in HoodieAvroParquetWriter and HoodieSparkParquetWriter`
comment on the first site; happy to switch to (b) if you prefer. WDYT?
2. **Error message wording in `HoodieTableMetaClient.TableBuilder` (lines
1554–1564)** — the current message uses "Set populate.meta.fields=false or use
MetaFieldsMode.ALL", which reads like a runtime toggle. Since the mode is
immutable-per-table, would you prefer wording that emphasizes the config
conflict (e.g., "META_FIELDS_MODE=… conflicts with populate.meta.fields=true;
choose one, not both")?
3. **`populateMetaFields=false + no explicit mode` → NONE (backward-compat
path)** — covered indirectly by `noneModePersistsAndLeavesAllColumnsNull` in
`TestMetaFieldsMode`, but there's no test explicitly asserting the on-disk
`hoodie.properties` has *no* `hoodie.meta.fields.mode` key set in that case.
Worth adding a targeted assertion or is the indirect coverage enough?
Not addressing the interaction with RLI / secondary index in this PR — that
lives in the MoR follow-up (#19206 / phase 3) where the same fail-fast check
applies to both CoW and MoR.
--
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]
