Repository navigation
fix(core): record the legacy precombine key for tables below version 9 - #20346
wombatu-kun wants to merge 4 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 also writes hoodie.table.precombine.field when a table below version 9 is created or re-created, so 0.x and 1.0.x readers can find the ordering field; one edge case with multi-field ordering is flagged inline. Please take a look at any inline comments, and this should be ready for a Hudi committer or PMC member to take it from here.
| String orderingFields = hoodieConfig.getProps().getProperty(ORDERING_FIELDS.key()); | ||
| if (orderingFields != null && tableVersion.lesserThan(HoodieTableVersion.NINE)) { | ||
| // readers of tables below version 9 only know the deprecated precombine key, which the 9 to 8 downgrade writes too | ||
| hoodieConfig.setValue(PRECOMBINE_FIELD, orderingFields); |
There was a problem hiding this comment.
🤖 It looks like a v6/v8 table created with several ordering fields (e.g. orderingFields = 'ts,seq') would now record hoodie.table.precombine.field=ts,seq, which 0.x and 1.0.x read as a single field name. On 1.0.2 the file group reader then throws Field: ts,seq does not exist in the table schema for MOR slices with log files, where today it only logs a warning and reads. Would it make sense to write the legacy key only when there is exactly one ordering field?
…a single ordering field
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #20346 +/- ##
============================================
+ Coverage 80.73% 80.75% +0.01%
- Complexity 35169 35320 +151
============================================
Files 2553 2562 +9
Lines 143577 144146 +569
Branches 17486 17620 +134
============================================
+ Hits 115924 116401 +477
- Misses 19696 19772 +76
- Partials 7957 7973 +16
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! This records hoodie.table.precombine.field alongside hoodie.table.ordering.fields for tables below version 9 that have a single ordering field, both at creation and when a table is re-created from its own properties (rename, truncate), so 0.x and 1.0.x readers find the ordering field again. No issues flagged from this automated pass — a Hudi committer or PMC member can take it from here for a final review.
cc @yihua
| if (orderingFields.size() == 1 && tableVersion.lesserThan(HoodieTableVersion.NINE)) { | ||
| // readers of tables below version 9 only know the deprecated precombine key, which the 9 to 8 downgrade writes too, | ||
| // and take its value as one field name | ||
| hoodieConfig.setValue(PRECOMBINE_FIELD, orderingFields.get(0)); |
There was a problem hiding this comment.
major: NineToEightDowngradeHandler.getOrderingFieldsBelowVersionNine in #20348 resolves this to DebeziumConstants.ADDED_SEQ_COL_NAME when the legacy payload is MySqlDebeziumAvroPayload, so a v8 table created here and an identical one downgraded to v8 end up with different precombine keys. LEGACY_PAYLOAD_CLASS_NAME is reconciled before this point, so the same check fits here.
There was a problem hiding this comment.
Done ecbffc4: below version 9 a MySqlDebeziumAvroPayload table now gets hoodie.table.precombine.field=_event_seq, the same as after the downgrade. The payload is detected through hoodie.compaction.payload.class, since hoodie.table.legacy.payload.class is not set below version 9.
There was a problem hiding this comment.
Reverted in c27bc85 after danny0405's comment: a table declared with _event_seq still gets the same precombine key as a downgraded one, and one declared with ts now keeps ts. With _event_seq next to hoodie.table.ordering.fields=ts, a 0.x or 1.0.x Spark writer would dedup on _event_seq while 1.x dedups on ts, or fail with Config conflict when passed ts.
…nder the legacy precombine key
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! This PR records the ordering field under the legacy hoodie.table.precombine.field key when a table below version 9 is created or re-created, using a single ordering field as-is and _event_seq for MySqlDebeziumAvroPayload, the same value the 9 to 8 downgrade writes. 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. The code is clean; I have one small suggestion to pull the new legacy-precombine block out of create() into a named helper.
cc @yihua
| hoodieConfig.setValue(POPULATE_META_FIELDS, | ||
| Boolean.toString(metaFieldsMode.toLegacyPopulateMetaFields())); | ||
| } | ||
| List<String> orderingFields = StringUtils.split(hoodieConfig.getProps().getProperty(ORDERING_FIELDS.key()), ","); |
There was a problem hiding this comment.
🤖 nit: would it make sense to move this block into a small helper like setLegacyPrecombineFieldIfNeeded(hoodieConfig, tableVersion), so create() reads as a list of steps and the rule for versions below 9 lives in one named place?
There was a problem hiding this comment.
With c27bc85 the block is back to one if, the size of the POPULATE_META_FIELDS step above it, so it stays inline.
| // and take its value as one field name | ||
| if (MySqlDebeziumAvroPayload.class.getName().equals(hoodieConfig.getString(PAYLOAD_CLASS_NAME))) { | ||
| // below version 9 this payload orders on its sequence field, the value the downgrade writes for it | ||
| hoodieConfig.setValue(PRECOMBINE_FIELD, DebeziumConstants.ADDED_SEQ_COL_NAME); |
There was a problem hiding this comment.
So the user just not declare the ordering field? should we just follow the user config since it has no default value now. and why couple payload class with ordeging field, do we have this validation for the bindings already?
There was a problem hiding this comment.
Done c27bc85: the legacy key now repeats the declared ordering field whatever the payload, and stays unset when none is declared. There is no such validation: this payload's ordering fields are only rewritten at version 9 creation, by the 8 to 9 and 9 to 8 handlers, and by StreamSync on version 9 tables.
…he legacy precombine key whatever the payload
shangxinli
left a comment
shangxinli
left a comment
There was a problem hiding this comment.
Checked the revert in c27bc85 - the 9 to 8 downgrade drops hoodie.table.ordering.fields when it writes _event_seq, so a created table that keeps both keys is not the same case and repeating the declared ordering field is the right call here. My comment is settled.
This branch has not been deployed
5 participants
Describe the issue this Pull Request addresses
Closes #20345
Tables below version 9 recorded their ordering fields only as
hoodie.table.ordering.fields, which Hudi 0.x (version 6) and 1.0.x (version 8) do not read, andALTER TABLE RENAMEdroppedhoodie.table.precombine.fieldfrom tables that had it.Summary and Changelog
HoodieTableConfig.createalso records the ordering field of a table below version 9 underhoodie.table.precombine.field, the key its readers know and the one the 9 to 8 downgrade writes. Those readers take that key as one field name, so it is written only when a single ordering field is declared, with that field, whatever the payload. Table creation and every re-creation from the table's own properties go through it.TestHoodieTableMetaClientpins both keys per table version, for a new table, for one ordered on two fields, for aMySqlDebeziumAvroPayloadtable, and for one re-created from properties that carry only the legacy key. A Spark SQL test does the same through CREATE TABLE and ALTER TABLE RENAME at versions 6 and 8.Impact
Tables created or re-created below version 9 with a single ordering field carry
hoodie.table.precombine.fieldnext tohoodie.table.ordering.fields, with the same value, so 0.x and 1.0.x find their ordering field again and order records on the same field as 1.x. Tables ordered on several fields keep only the new key, as before, since 1.0.2 would otherwise fail MOR merges withField: ts,seq does not exist in the table schemainstead of warning. 1.x readshoodie.table.ordering.fieldsfirst, so its behavior is unchanged, and the 8 to 9 upgrade still removes the legacy key. Tables created before this fix keep only the new key until they are re-created. #20276 pins the exact table configs of versions 6 and 8, so whichever of the two lands second adds the legacy key there.Risk Level
low
The change adds one key below version 9, only next to a single declared ordering field and with the same value, and leaves every value 1.x reads as it was. Verified: the single-field tests fail on master with
precombine key at version 6 ==> expected: <ts> but was: <null>, and Hudi 0.15.0 and 1.0.2 resolvegetPreCombineField()to the ordering field on single-field tables written with this change (null without it).Documentation Update
none
Contributor's checklist