fix(hadoop-mr): ignore blank Hive projection ids and report both lists on mismatch - #19463
fix(hadoop-mr): ignore blank Hive projection ids and report both lists on mismatch#19463rangareddy wants to merge 1 commit 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 improves the diagnostics of the "Error ordering fields for storage read" failure in HoodieRealtimeRecordReaderUtils#orderFields by reporting the de-duplicated name count that was actually compared (rather than the raw count, which could print two equal numbers for a real mismatch) and including the Hive projection lists, plus adds unit and reader-path test coverage. The message change is consistent with the comparison logic and the tests reproduce the reported failure shape. No issues flagged from this automated pass — a Hudi committer or PMC member can take it from here for a final review.
cc @yihua
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19463 +/- ##
============================================
- Coverage 77.03% 76.97% -0.07%
- Complexity 33863 33868 +5
============================================
Files 2575 2575
Lines 143379 143398 +19
Branches 17574 17579 +5
============================================
- Hits 110451 110378 -73
- Misses 24666 24740 +74
- Partials 8262 8280 +18
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
voonhous
left a comment
voonhous
left a comment
There was a problem hiding this comment.
Reviewed the change against the history of orderFields and against issue #14673 itself.
The core change is correct and worth having: 3a05edab01f7 (2019-11-03) introduced the LinkedHashSet de-duplication and switched the predicate to fieldNamesSet.size() while leaving the format argument as fieldNames.size() one line below. That was an oversight, never covered by a test, and this PR fixes it. No accidental revert, no resource leak in the new test (HoodieRealtimeRecordReader closes realReader in its catch), and dumping the column lists is consistent with existing precedent in this package.
Comments inline. The two that I think should change before merge are the root-cause claim in the reader test's javadoc, which #14673 does not support and which also lands in the commit message, and the fact that the documented HIVE-22438 shape still escapes as a bare NumberFormatException -- for a PR about diagnosability, that is the case most worth covering. The rest are test-efficacy points and nits, marked as such.
8e450d3 to
1305992
Compare
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Approve. One minor naming nit in the test file; production change is clean.
cc @yihua
…s on mismatch Relates to apache#14673 (HUDI-1286). orderFields maps Hive's hive.io.file.readcolumn.names and .ids onto an ordered projection list. Two problems, both reachable from the query in that issue. A blank id token reached Integer.parseInt and failed with a bare NumberFormatException carrying neither projection list. Blank tokens are exactly what HIVE-22438 produces: for SELECT COUNT(*) on Hive before 3.0.0 the read-column ids arrive empty and Hive combines them into e.g. ",2,0,3". HoodieRealtimeInputFormatUtils#cleanProjectionColumnIds already documents this but strips only one leading comma, so ",,2,0" still gets through. Filtering blank tokens also resolves the shape reported in apache#14673 outright: four names against the five tokens ",2,0,3,5" only disagreed because the blank was counted, and master fails that input with exactly the reported "#fieldNames: 4, #fieldPositions: 5". On a genuine mismatch the message reported the raw name count while comparing the de-duplicated one, so duplicate names produced two equal numbers: names "rider,driver,fare,fare" ids "0,1,2,3" -> Error ordering fields for storage read. #fieldNames: 4, #fieldPositions: 4 The counts are now named #distinctFieldNames and #distinctFieldPositions, so a de-duplicated count printed beside a raw list no longer reads as a contradiction, and both projection lists are included since they come from the engine and are the only way to tell which side is wrong. On the mechanism behind apache#14673: it is not confirmed. An earlier draft of this change asserted CombineHiveInputFormat accumulating projection entries across splits, which the issue does not support - the reported frame is HiveInputFormat.getRecordReader, the base class, and HoodieRealtimeInputFormatUtils#addProjectionField always sets both keys together, so Hudi cannot make them diverge that way. HIVE-22438 and HUDI-313 are the mechanism this repo already documents for that query. Eight tests for orderFields, which had none. Four fail on master: the two message assertions, the blank-token handling, and the apache#14673 shape. The other four characterise existing behaviour, including a guard for the partition-field filter that HUDI-5308 (apache#7355) removed - that parameter was implemented and then deliberately dropped, not never implemented.
1305992 to
8b0f075
Compare
rangareddy
changed the title
|
This will need a re-review since the content has changed. |
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! This PR makes orderFields drop blank Hive projection id tokens (the HIVE-22438 shape) and reports both de-duplicated counts plus the raw projection lists on a genuine mismatch. No new issues flagged from this automated pass — the earlier de-duplication and residual blank-token concerns were already raised and acknowledged. A Hudi committer or PMC member can take it from here for a final review. A couple of minor comments on the new production comment and a test method name.
cc @yihua
| List<String> fieldNames = fieldNameCsv.isEmpty() ? new ArrayList<>() : Arrays.stream(fieldNameCsv.split(",")).collect(Collectors.toList()); | ||
| Set<String> fieldNamesSet = new LinkedHashSet<>(fieldNames); | ||
| if (fieldNamesSet.size() != fieldOrders.length) { | ||
| // The counts are of the de-duplicated lists, which is what was compared, so they are named as such: |
There was a problem hiding this comment.
🤖 nit: the 4-line comment here re-explains reasoning that the renamed format placeholders (#distinctFieldNames, #distinctFieldPositions) already make self-evident — could you trim it to something like // use de-duplicated counts to match what was actually compared?
| * The shape reported in #14673 - four names against five id tokens, one of them blank - is what the | ||
| * HIVE-22438 combining produces. Dropping the blank leaves four real ids against four names, so it | ||
| * resolves rather than failing at all: the counts only ever disagreed because the blank was counted. | ||
| */ |
There was a problem hiding this comment.
🤖 nit: embedding the raw issue number in the method name (Issue14673) means the test is opaque without the Javadoc — could you rename it to something like testOrderFieldsResolvesWhenSingleBlankIdTokenCausedCountMismatch so the name is self-describing?
5 participants
Describe the issue this Pull Request addresses
Relates to #14673 (HUDI-1286), which reports a MOR
_rtquery failing with:orderFieldsmaps Hive'shive.io.file.readcolumn.namesandhive.io.file.readcolumn.idsonto an orderedprojection list. Two problems, both reachable from that query.
A blank id token failed to parse, with no diagnostics at all. Blank tokens are what HIVE-22438 produces:
for
SELECT COUNT(*)on Hive before 3.0.0 the read-column ids arrive empty and Hive combines them into e.g.",2,0,3".HoodieRealtimeInputFormatUtils#cleanProjectionColumnIdsalready documents this but strips onlyone leading comma, so
",,2,0"still gets through. A blank token then reachedInteger.parseIntand threw abare
NumberFormatExceptioncarrying neither projection list.On a genuine mismatch the message could print two equal numbers. The comparison uses the de-duplicated
name count while the message reported the raw one:
and it omitted the two lists, which come from the engine and are the only way to tell which side is wrong.
On the mechanism behind #14673 — it is not confirmed. An earlier revision of this PR asserted
CombineHiveInputFormataccumulating projection entries across splits. That is not supported by the issue:the reported frame is
HiveInputFormat.getRecordReader, the base class, andHoodieRealtimeInputFormatUtils#addProjectionFieldalways setsREAD_COLUMN_NAMESandREAD_COLUMN_IDStogether in the same
if, so Hudi cannot make them diverge that way. HIVE-22438 and HUDI-313 are themechanism this repo already documents for that query, and the blank-token shape reproduces the reported
numbers exactly — see Verification.
Summary and Changelog
This turned out to be a functional fix, not only a diagnostics one: for the reported input the counts
disagreed only because the blank was counted, so dropping it makes the projection resolve.
#distinctFieldNames/#distinctFieldPositions, so a de-duplicatedcount printed beside a raw list no longer reads as a contradiction, and both projection lists are included.
orderFields, which had none.Not changed here, and filed as #19506: names and ids are de-duplicated independently and then paired
positionally, which is unsound because Hive prepends ids while appending names. Filed so this message
improvement is not later mistaken for a fix for that.
Verification
mvn test checkstyle:check apache-rat:check -pl hudi-hadoop-mr -Dtest='TestHoodieRealtimeRecordReaderUtils,TestHoodieRealtimeRecordReader'→ 42 tests green, rat
Unapproved: 0.Four of the eight new tests fail against master's
orderFields, which is the evidence they guard this change:The second line is the equal-numbers pathology. The fourth is the shape reported in #14673, reproduced
exactly — master fails
("a,b,c,ts", ",2,0,3,5")with#fieldNames: 4, #fieldPositions: 5, which is why Inow believe HIVE-22438 is the actual mechanism. The other four tests characterise existing behaviour rather
than guarding this change, including
testOrderFieldsNoLongerFiltersPartitionFields.On that last one: the unused
partitioningFieldsparameter was implemented and then deliberately removedby
30d497a19844([HUDI-5308] Hive3 query returns null when the where clause has a partition field, #7355),which deleted the
.filter(fn -> !partitioningFields.contains(fn)). An earlier revision of this descriptioncalled it an intent that was never implemented, which was wrong and would have misled whoever picks up
HUDI-1286.
An earlier revision also added a reader-level test in
TestHoodieRealtimeRecordReader. It was removed duringreview: the names there come from
schema.getFields()and Avro forbids duplicate field names, so its countassertion could not fail whether or not the fix was present, and the message content it did check is covered
by the unit tests above without building a MOR table, a parquet base file and a log block.
Impact
SELECT COUNT(*)on a MOR_rttable against Hive before 3.0.0 stops failing on the blank id token. Anyremaining genuine mismatch now reports the counts that were compared plus both projection lists. No behaviour
change for a well-formed projection: the same inputs produce the same ordering.
Risk Level
low — one filter and one message in a single method, with eight tests where there were none, four of which
fail without the change.
Documentation Update
none
Contributor's checklist