nsivabalan commented on code in PR #19205:
URL: https://github.com/apache/hudi/pull/19205#discussion_r3767554795
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieWriteClient.java:
##########
@@ -1548,9 +1549,40 @@ public void
validateAgainstTableProperties(HoodieTableConfig tableConfig, Hoodie
// mismatch of table versions.
CommonClientUtils.validateTableVersion(tableConfig, writeConfig);
- // Once meta fields are disabled, it cant be re-enabled for a given table.
- if (!tableConfig.populateMetaFields() && writeConfig.populateMetaFields())
{
- throw new HoodieException(HoodieTableConfig.POPULATE_META_FIELDS.key() +
" already disabled for the table. Can't be re-enabled back");
+ // Meta-field population is physical, so a writer must not claim columns
the table does not
+ // have. Compare the full enum rather than the legacy booleans: those
collapse every selective
+ // mode to false, so a writer claiming COMMIT_TIME_ONLY against a NONE
table would slip through
+ // and advertise commit times that were never written.
+ //
+ // Two distinct cases, because writers routinely omit meta-field settings
entirely:
+ //
+ // - Widening is always rejected. Enabling a column now would leave
earlier commits without it,
+ // and readers cannot tell the two apart.
+ // - Any disagreement is rejected when the writer *explicitly* sets
hoodie.meta.fields.mode.
+ // That covers narrowing too, e.g. an explicit NONE against a
COMMIT_TIME_ONLY table, which
+ // would write null commit times while the table still advertises
COMMIT_TIME_ONLY and make
+ // incremental queries silently miss those rows.
+ //
+ // A writer that never mentions the mode is left alone: resolving to NONE
against an ALL table
+ // is long-standing behavior for callers that build a write config without
restating the table's
+ // settings, and writing fewer meta columns cannot make a reader believe
in absent data.
+ MetaFieldsMode tableMetaFieldsMode = tableConfig.getMetaFieldsMode();
+ MetaFieldsMode writeMetaFieldsMode = writeConfig.getMetaFieldsMode();
+ boolean writerStatedMode =
writeConfig.contains(HoodieTableConfig.META_FIELDS_MODE)
+ &&
!StringUtils.isNullOrEmpty(writeConfig.getString(HoodieTableConfig.META_FIELDS_MODE));
+ if (writeMetaFieldsMode.isWiderThan(tableMetaFieldsMode)) {
+ throw new HoodieException(String.format(
+ "%s cannot be widened for an existing table: table is %s but the
writer requests %s. Meta "
+ + "columns are physical, so enabling one now would leave earlier
commits without it. "
+ + "Set %s=%s on the writer, or recreate the table to change it.",
+ HoodieTableConfig.META_FIELDS_MODE.key(), tableMetaFieldsMode,
writeMetaFieldsMode,
+ HoodieTableConfig.META_FIELDS_MODE.key(), tableMetaFieldsMode));
+ } else if (writerStatedMode && writeMetaFieldsMode != tableMetaFieldsMode)
{
Review Comment:
You were right, and it is gone from the condition entirely.
Your reasoning held under the old two-tier check, and the rule has since
collapsed further: the condition is now just `writeMetaFieldsMode !=
tableMetaFieldsMode`, with no `writerStatedMode` term at all.
The flag survives only to phrase the error message — a writer that stated
nothing is told it "does not state one and so resolves to ALL", while one that
stated a different mode is told it "requests NONE". Same outcome either way;
the advice differs, because the fix differs. It is named
`writerStatedMetaFields` now and commented as message-only so nobody reads it
as part of the rule.
One correction to your premise, worth noting because it is why the flag
could not just be deleted outright: under the current rule `writeMetaFieldsMode
!= tableMetaFieldsMode` does **not** imply the writer stated something. A
writer that states nothing resolves to the `ALL` default, which differs from
every non-`ALL` table — so the unstated case reaches the throw too. That is
deliberate (see the reply on the four-point thread) and is what
`validateAgainstTablePropertiesRejectsADefaultWriterAgainstANoneTable` pins.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieWriteClient.java:
##########
@@ -1548,9 +1549,40 @@ public void
validateAgainstTableProperties(HoodieTableConfig tableConfig, Hoodie
// mismatch of table versions.
CommonClientUtils.validateTableVersion(tableConfig, writeConfig);
- // Once meta fields are disabled, it cant be re-enabled for a given table.
- if (!tableConfig.populateMetaFields() && writeConfig.populateMetaFields())
{
- throw new HoodieException(HoodieTableConfig.POPULATE_META_FIELDS.key() +
" already disabled for the table. Can't be re-enabled back");
+ // Meta-field population is physical, so a writer must not claim columns
the table does not
+ // have. Compare the full enum rather than the legacy booleans: those
collapse every selective
+ // mode to false, so a writer claiming COMMIT_TIME_ONLY against a NONE
table would slip through
+ // and advertise commit times that were never written.
+ //
+ // Two distinct cases, because writers routinely omit meta-field settings
entirely:
+ //
+ // - Widening is always rejected. Enabling a column now would leave
earlier commits without it,
+ // and readers cannot tell the two apart.
+ // - Any disagreement is rejected when the writer *explicitly* sets
hoodie.meta.fields.mode.
+ // That covers narrowing too, e.g. an explicit NONE against a
COMMIT_TIME_ONLY table, which
+ // would write null commit times while the table still advertises
COMMIT_TIME_ONLY and make
+ // incremental queries silently miss those rows.
+ //
+ // A writer that never mentions the mode is left alone: resolving to NONE
against an ALL table
+ // is long-standing behavior for callers that build a write config without
restating the table's
+ // settings, and writing fewer meta columns cannot make a reader believe
in absent data.
+ MetaFieldsMode tableMetaFieldsMode = tableConfig.getMetaFieldsMode();
+ MetaFieldsMode writeMetaFieldsMode = writeConfig.getMetaFieldsMode();
+ boolean writerStatedMode =
writeConfig.contains(HoodieTableConfig.META_FIELDS_MODE)
+ &&
!StringUtils.isNullOrEmpty(writeConfig.getString(HoodieTableConfig.META_FIELDS_MODE));
+ if (writeMetaFieldsMode.isWiderThan(tableMetaFieldsMode)) {
Review Comment:
No — narrowing is not allowed either, and you are right that the code did
not read that way. It does now.
Both directions are rejected, for different reasons:
- **Widening** would leave earlier commits missing a column later ones have,
and a reader cannot distinguish the two.
- **Narrowing** would leave rows the table still advertises as populated,
which incremental queries then silently skip. That is the StreamSync-restart
data loss @cshuo reported.
The old code had a separate `isWiderThan` branch that made widening look
like the only rejected direction. There is now a single equality check, and
`isWiderThan` is used only to pick which half of the explanation goes in the
message.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/io/HoodieWriteMergeHandle.java:
##########
@@ -413,7 +418,16 @@ protected void writeToFile(HoodieKey key, HoodieRecord<T>
record, HoodieSchema s
if (shouldPreserveRecordMetadata) {
// NOTE: `FILENAME_METADATA_FIELD` has to be rewritten to correctly
point to the
// file holding this record even in cases when overall metadata is
preserved
- HoodieRecord populatedRecord = record.updateMetaField(schema,
HoodieRecord.FILENAME_META_FIELD_ORD, newFilePath.getName());
+ //
+ // The rewrite is gated on the mode: hoodie.meta.fields.mode is the
single authority on which
+ // meta columns hold values, and this path would otherwise populate
_hoodie_file_name on a
+ // COMMIT_TIME_ONLY / NONE table. The value written when the mode opts
out is an explicit null
+ // rather than a skipped update, because the record being preserved here
came from the previous
+ // base file — under a narrowed mode it can still carry a file name
written while the table was
+ // on ALL, and leaving that in place would carry a stale value forward.
+ String fileNameToWrite =
Review Comment:
Pushing back on this one — skipping the update would carry a stale file name
forward.
The record on this path is `oldRecord`, read from the *previous* base file
(`HoodieWriteMergeHandle:408`, the `copyOldRecord` branch). If the table was on
`ALL` when that file was written and has since been narrowed, the record
already carries a `_hoodie_file_name` — and it names a file this record no
longer lives in. Skipping the update leaves that value in place, so a
`COMMIT_TIME_ONLY` table ends up with rows pointing at replaced files.
Writing an explicit null is what clears it. Note that routing the null
through `MetadataValues` would not work either — `updateMetadataValuesInternal`
skips null entries:
```java
// HoodieAvroIndexedRecord:383
if (value != null) {
avroRecord.put(HoodieMetadataField.values()[pos].getFieldName(), value);
}
```
which is why the code calls `updateMetaField(..., FILENAME_META_FIELD_ORD,
null)` directly rather than `setFileName(null)`.
Happy to be shown wrong if you think the narrowed-after-ALL case is
unreachable, but the mode is one-way and narrowing is explicitly supported
through hudi-cli, so a table that was `ALL` and is now `COMMIT_TIME_ONLY` is a
state we are committing to support.
--
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]
