You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hudi.apache.org by "wombatu-kun (via GitHub)" <gi...@apache.org> on 2026/10/16 05:49:53 UTC
[PR] fix(spark): resolve nested ordering fields for Spark deletes written to delete blocks [hudi]
wombatu-kun opened a new pull request, #20368:
URL: https://github.com/apache/hudi/pull/20368
### Describe the issue this Pull Request addresses
Closes #20367
With the Spark record type, a delete written into an inline delete block lost the value of a nested ordering field. As a result, a stale delete removed a newer row.
### Summary and Changelog
- `HoodieSparkRecord.getOrderingValueAsJava` now resolves an ordering field by its nested path, the way `doGetOrderingValue` already does, instead of checking only the top-level fields. A field that does not exist still gets the default ordering value.
- `TestMORDataSource` writes a stale delete and a newer delete against a nested `bigint`, `string` and `decimal` ordering field at versions 8 and 9, and checks that only the newer one applies. The Avro record type serves as the reference.
### Impact
Spark-record-type writes to MOR tables at versions 8 and 9 now keep the value of a nested ordering field in their delete blocks, so event-time ordering applies to deletes as it does to updates. Version 9 is the default of 1.2.x.
Top-level ordering fields resolve as before. `writeDelete` of the inline log handle is the only caller. Version 10 native logs and the Avro record type already resolved the field by path. With #20341, deletes that win precombine take this path too.
### Risk Level
low
The lookup now resolves the field by its path, the same lookup `getColumnValueAsJava` already used to read the value.
Verified:
- On master, the Spark rows of the new test fail with `expected: <List(id1)> but was: <WrappedArray()>`.
- With the fix, the new test and the other 115 tests of `TestMORDataSource` pass.
### Documentation Update
none
### Contributor's checklist
- [x] Read through [contributor's guide](https://hudi.apache.org/contribute/how-to-contribute)
- [x] Enough context is provided in the sections above
- [x] Adequate tests were added if applicable
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(spark): resolve nested ordering fields for Spark deletes written to delete blocks [hudi]
Posted by "danny0405 (via GitHub)" <gi...@apache.org>.
danny0405 commented on code in PR #20368:
URL: https://github.com/apache/hudi/pull/20368#discussion_r4321315468
##########
hudi-client/hudi-spark-client/src/main/java/org/apache/hudi/common/model/HoodieSparkRecord.java:
##########
@@ -392,10 +392,12 @@ public Comparable<?> getOrderingValueAsJava(HoodieSchema recordSchema, Propertie
StructType structType = HoodieInternalRowUtils.getCachedSchema(recordSchema);
return OrderingValues.create(orderingFields, field -> {
// Ordering fields can be nested, so resolve them by path as doGetOrderingValue does.
- if (HoodieInternalRowUtils.getCachedPosList(structType, field).isEmpty()) {
+ scala.Option<NestedFieldPath> cachedNestedFieldPath =
+ HoodieInternalRowUtils.getCachedPosList(structType, field);
+ if (cachedNestedFieldPath.isEmpty()) {
return OrderingValues.getDefault();
}
- return (Comparable<?>) getColumnValueAsJava(recordSchema, field, props);
+ return (Comparable<?>) getFieldValueFromInternalRowAsJava(data, cachedNestedFieldPath.get());
Review Comment:
guess we might need to add a new getFieldValueFromInternalRowInternal that pass around the StructType and HoodieUnsafeRowUtils.NestedFieldPath directly so that we can eliminate the repetitive calls completely.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(spark): resolve nested ordering fields for Spark deletes written to delete blocks [hudi]
Posted by "danny0405 (via GitHub)" <gi...@apache.org>.
danny0405 merged PR #20368:
URL: https://github.com/apache/hudi/pull/20368
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(spark): resolve nested ordering fields for Spark deletes written to delete blocks [hudi]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #20368:
URL: https://github.com/apache/hudi/pull/20368#issuecomment-6209794613
## [Codecov](https://app.codecov.io/gh/apache/hudi/pull/20368?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `0%` with `2 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 78.11%. Comparing base ([`c167f7b`](https://app.codecov.io/gh/apache/hudi/commit/c167f7b392441cfb86b1dd64bb6c2708878e80d6?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`77e024c`](https://app.codecov.io/gh/apache/hudi/commit/77e024c6c74944124b5d3ec1d332ef18a8ce5da4?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/hudi/pull/20368?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...rg/apache/hudi/common/model/HoodieSparkRecord.java](https://app.codecov.io/gh/apache/hudi/pull/20368?src=pr&el=tree&filepath=hudi-client%2Fhudi-spark-client%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fhudi%2Fcommon%2Fmodel%2FHoodieSparkRecord.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aHVkaS1jbGllbnQvaHVkaS1zcGFyay1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29tbW9uL21vZGVsL0hvb2RpZVNwYXJrUmVjb3JkLmphdmE=) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/hudi/pull/20368?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #20368 +/- ##
============================================
- Coverage 80.73% 78.11% -2.63%
+ Complexity 35201 34017 -1184
============================================
Files 2553 2553
Lines 143629 143635 +6
Branches 17504 17505 +1
============================================
- Hits 115964 112195 -3769
- Misses 19703 23261 +3558
- Partials 7962 8179 +217
```
| [Components](https://app.codecov.io/gh/apache/hudi/pull/20368/components?src=pr&el=components&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hudi-common](https://app.codecov.io/gh/apache/hudi/pull/20368/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `82.49% <ø> (-1.67%)` | :arrow_down: |
| [hudi-client](https://app.codecov.io/gh/apache/hudi/pull/20368/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `79.60% <0.00%> (-4.36%)` | :arrow_down: |
| [hudi-flink](https://app.codecov.io/gh/apache/hudi/pull/20368/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `86.08% <ø> (+<0.01%)` | :arrow_up: |
| [hudi-spark-datasource](https://app.codecov.io/gh/apache/hudi/pull/20368/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `67.19% <ø> (-7.00%)` | :arrow_down: |
| [hudi-utilities](https://app.codecov.io/gh/apache/hudi/pull/20368/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `78.19% <ø> (ø)` | |
| [hudi-cli](https://app.codecov.io/gh/apache/hudi/pull/20368/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `70.80% <ø> (ø)` | |
| [hudi-hadoop](https://app.codecov.io/gh/apache/hudi/pull/20368/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `69.06% <ø> (-2.13%)` | :arrow_down: |
| [hudi-sync](https://app.codecov.io/gh/apache/hudi/pull/20368/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `76.78% <ø> (ø)` | |
| [hudi-io](https://app.codecov.io/gh/apache/hudi/pull/20368/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `81.43% <ø> (-0.05%)` | :arrow_down: |
| [hudi-timeline-service](https://app.codecov.io/gh/apache/hudi/pull/20368/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `78.53% <ø> (-4.88%)` | :arrow_down: |
| [hudi-cloud](https://app.codecov.io/gh/apache/hudi/pull/20368/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `80.81% <ø> (-0.19%)` | :arrow_down: |
| [hudi-kafka-connect](https://app.codecov.io/gh/apache/hudi/pull/20368/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.90% <ø> (ø)` | |
| [Flag](https://app.codecov.io/gh/apache/hudi/pull/20368/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [flink-integration-tests](https://app.codecov.io/gh/apache/hudi/pull/20368/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `49.65% <ø> (+<0.01%)` | :arrow_up: |
| [hadoop-mr-java-client](https://app.codecov.io/gh/apache/hudi/pull/20368/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `43.94% <ø> (+<0.01%)` | :arrow_up: |
| [integration-tests](https://app.codecov.io/gh/apache/hudi/pull/20368/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `13.44% <0.00%> (-0.01%)` | :arrow_down: |
| [spark-client-hadoop-common](https://app.codecov.io/gh/apache/hudi/pull/20368/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `38.67% <0.00%> (-0.01%)` | :arrow_down: |
| [spark-java-tests](https://app.codecov.io/gh/apache/hudi/pull/20368/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `32.16% <0.00%> (-20.63%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Files with missing lines](https://app.codecov.io/gh/apache/hudi/pull/20368?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...rg/apache/hudi/common/model/HoodieSparkRecord.java](https://app.codecov.io/gh/apache/hudi/pull/20368?src=pr&el=tree&filepath=hudi-client%2Fhudi-spark-client%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fhudi%2Fcommon%2Fmodel%2FHoodieSparkRecord.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aHVkaS1jbGllbnQvaHVkaS1zcGFyay1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29tbW9uL21vZGVsL0hvb2RpZVNwYXJrUmVjb3JkLmphdmE=) | `50.72% <0.00%> (-4.62%)` | :arrow_down: |
... and [375 files with indirect coverage changes](https://app.codecov.io/gh/apache/hudi/pull/20368/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(spark): resolve nested ordering fields for Spark deletes written to delete blocks [hudi]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #20368:
URL: https://github.com/apache/hudi/pull/20368#discussion_r4325471107
##########
hudi-client/hudi-spark-client/src/main/java/org/apache/hudi/common/model/HoodieSparkRecord.java:
##########
@@ -392,10 +392,12 @@ public Comparable<?> getOrderingValueAsJava(HoodieSchema recordSchema, Propertie
StructType structType = HoodieInternalRowUtils.getCachedSchema(recordSchema);
return OrderingValues.create(orderingFields, field -> {
// Ordering fields can be nested, so resolve them by path as doGetOrderingValue does.
- if (HoodieInternalRowUtils.getCachedPosList(structType, field).isEmpty()) {
+ scala.Option<NestedFieldPath> cachedNestedFieldPath =
+ HoodieInternalRowUtils.getCachedPosList(structType, field);
+ if (cachedNestedFieldPath.isEmpty()) {
return OrderingValues.getDefault();
}
- return (Comparable<?>) getColumnValueAsJava(recordSchema, field, props);
+ return (Comparable<?>) getFieldValueFromInternalRowAsJava(data, cachedNestedFieldPath.get());
Review Comment:
Done 648342b7c02e. Per row, `getOrderingValueAsJava` resolves the `StructType` once and each field's `NestedFieldPath` once, and reads the value through the new `getFieldValueFromInternalRowInternal(InternalRow, NestedFieldPath, boolean)`, which the name-based lookup now shares; it takes no `StructType`, since the path already carries each field's type.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(spark): resolve nested ordering fields for Spark deletes written to delete blocks [hudi]
Posted by "hudi-agent (via GitHub)" <gi...@apache.org>.
hudi-agent commented on code in PR #20368:
URL: https://github.com/apache/hudi/pull/20368#discussion_r4291389816
##########
hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/hudi/functional/TestMORDataSource.scala:
##########
@@ -2594,6 +2594,37 @@ class TestMORDataSource extends HoodieSparkClientTestBase with SparkDatasetMixin
def testNestedFieldPartition(): Unit = {
TestCOWDataSource.runNestedFieldPartitionTest(spark, basePath, storage, "MOR")
}
+
+ @ParameterizedTest
Review Comment:
🤖 nit: could you rename the test to say what it verifies, e.g. `testDeleteWithNestedOrderingFieldRespectsEventTimeOrdering`? Also the `"050"` vs `"100"` strings only work as intended after the cast, which is easy to miss.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag quality.</i></sub>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(spark): resolve nested ordering fields for Spark deletes written to delete blocks [hudi]
Posted by "danny0405 (via GitHub)" <gi...@apache.org>.
danny0405 commented on code in PR #20368:
URL: https://github.com/apache/hudi/pull/20368#discussion_r4310036688
##########
hudi-client/hudi-spark-client/src/main/java/org/apache/hudi/common/model/HoodieSparkRecord.java:
##########
@@ -389,8 +389,10 @@ public Comparable<?> getOrderingValueAsJava(HoodieSchema recordSchema, Propertie
if (orderingFields == null) {
return OrderingValues.getDefault();
} else {
+ StructType structType = HoodieInternalRowUtils.getCachedSchema(recordSchema);
return OrderingValues.create(orderingFields, field -> {
- if (recordSchema.getField(field).isEmpty()) {
+ // Ordering fields can be nested, so resolve them by path as doGetOrderingValue does.
+ if (HoodieInternalRowUtils.getCachedPosList(structType, field).isEmpty()) {
Review Comment:
the `HoodieInternalRowUtils.getCachedSchema` and `HoodieInternalRowUtils.getCachedPosList` could be called again once go into `getColumnValueAsJava`, since this is a per-row function call, could we try to eliminate the redundant calls.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(spark): resolve nested ordering fields for Spark deletes written to delete blocks [hudi]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #20368:
URL: https://github.com/apache/hudi/pull/20368#discussion_r4314124160
##########
hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/hudi/functional/TestMORDataSource.scala:
##########
@@ -2594,6 +2594,37 @@ class TestMORDataSource extends HoodieSparkClientTestBase with SparkDatasetMixin
def testNestedFieldPartition(): Unit = {
TestCOWDataSource.runNestedFieldPartitionTest(spark, basePath, storage, "MOR")
}
+
+ @ParameterizedTest
Review Comment:
Done b5b24b74103d. The leading zero makes `"050"` sort before `"100"` as a string too, and a comment now says so.
##########
hudi-client/hudi-spark-client/src/main/java/org/apache/hudi/common/model/HoodieSparkRecord.java:
##########
@@ -389,8 +389,10 @@ public Comparable<?> getOrderingValueAsJava(HoodieSchema recordSchema, Propertie
if (orderingFields == null) {
return OrderingValues.getDefault();
} else {
+ StructType structType = HoodieInternalRowUtils.getCachedSchema(recordSchema);
return OrderingValues.create(orderingFields, field -> {
- if (recordSchema.getField(field).isEmpty()) {
+ // Ordering fields can be nested, so resolve them by path as doGetOrderingValue does.
+ if (HoodieInternalRowUtils.getCachedPosList(structType, field).isEmpty()) {
Review Comment:
Done b5b24b74103d
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org