You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "jbewing (via GitHub)" <gi...@apache.org> on 2025/11/25 17:01:20 UTC
[PR] Spark (4.0, 3.5): Set data file `sort_order_id` in manifest for writes from Spark [iceberg]
jbewing opened a new pull request, #14683:
URL: https://github.com/apache/iceberg/pull/14683
This PR updates all writes from Spark 4.0 & 3.5 to set the `sort_order_id` of data files when applicable per the [Iceberg Table Spec](https://iceberg.apache.org/spec/#partition-evolution:~:text=A%20data%20or%20delete%20file%20is%20associated%20with%20a%20sort%20order%20by%20the%20sort%20order%27s%20id%20within%20a%20manifest). I've opened this PR to:
1. Gauge interest in this and validate my approach
2. If the approach is good, make a decision on whether to backport these changes to Spark 3.4. This patch doesn't apply super cleanly to 3.4 as it stands right now, so I don't want to invest effort into it unless others would find value from it.
3. As a follow up to those PRs, implement the ability to then report this ordering information when applicable to Spark to better inform the query planner of potential optimizations. This is rather useful when used in conjunction with SPJ and I have this running on an Iceberg fork with success
This is a successor to my initial PR https://github.com/apache/iceberg/pull/13636 which has since been closed for being stale (but I've revived it here and ported the changes forwards to spark 4.0). I've re-opened this PR as of late there has been some increased interest in this:
- A community member has expressed interest in reviving my older PR https://github.com/apache/iceberg/pull/13636#issuecomment-3505090324
- Other community members have reported this same bug on the issue list https://github.com/apache/iceberg/issues/14531
So it appears that there is value to these changes being upstreamed instead of confined to a fork.
### Testing
I've added tests for newer added utility functions and updated existing tests that write data files and compact data files in a sorted manner to verify that we're setting the `sort_order_id` entry in the manifests to the correct value. Additionally, I've used this patch on an internal fork and verified that it correctly sets this field during compaction and normal writes.
Issue: https://github.com/apache/iceberg/issues/13634
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark (4.0, 3.5): Set data file `sort_order_id` in manifest for writes from Spark [iceberg]
Posted by "jbewing (via GitHub)" <gi...@apache.org>.
jbewing commented on PR #14683:
URL: https://github.com/apache/iceberg/pull/14683#issuecomment-3752825752
Sorry to keep y'all waiting—I've been a bit busy recently. Given that I'd been waiting for a review for 6 months, what's 6 days really 😅 .
But @anuragmantri nailed the core piece of this PR. In the Iceberg Spark write path, typically a Spark based sort order was passed through and surfaced to the Spark APIs. This Spark-based sort order is typically derived from both the Iceberg table sort order, the write distribution requirements, and partitioning requirements. This makes it harder to go from the Spark ordering back to the Iceberg ordering (in fact, sometimes it's impossible).
So what I did here is that essentially we pass through both orderings: the ordering that Iceberg intends in addition to the Spark ordering (see SparkWriteUtil). The Spark ordering is passed through as it was and we use the Iceberg ordering to set manifest entries at write time. That's the idea, there's a bit more plumbing involved in some places e.g. rewrite data files jobs can set custom non-table sort orders and regular writes can also set non-table sort orders.
> Also I would recommend just doing the changes in 1 Spark version for review purposes, we can backport to anything applicable afterwards.
Yeah I did that 6 months ago and you're welcome to review that PR: https://github.com/apache/iceberg/pull/13636 or collapse one of the 3.5 or 4.0 directories during a review of this PR (they are mirrors of each other after all as I just applied the patch for 3.5 directly to 4.0). Or I can split it up more if you'd like, it's just time on my end and [I’ve split up PRs for easier review in the past, but have occasionally found those PRs don’t get feedback. If there’s a preference for how to structure these, just let me know!](https://apache-iceberg.slack.com/archives/C03LG1D563F/p1765466132290829).
In any case, rant aside. I'm willing to do the work if someone is actually interested in reviewing & giving me feedback here so that this can get integrated into master.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark (4.0, 3.5): Set data file `sort_order_id` in manifest for writes from Spark [iceberg]
Posted by "anuragmantri (via GitHub)" <gi...@apache.org>.
anuragmantri commented on PR #14683:
URL: https://github.com/apache/iceberg/pull/14683#issuecomment-3752309756
Since the original author has not responded yet, let me explain this. (@jbewing please correct me)
> It looks like the distribution and ordering code is being redone
The distribution/ordering code is not being redone. The changes differentiate between:
- Spark ordering (for write requirements/distribution) - unchanged behavior
- Iceberg sort order (for setting sort_order_id in manifest files) - new
The new `SparkAndIcebergOrdering` class couples these together. During writes/rewrites, it validates that the Spark sort order matches a table sort order ID before writing it to manifests.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark (4.0, 3.5): Set data file `sort_order_id` in manifest for writes from Spark [iceberg]
Posted by "jbewing (via GitHub)" <gi...@apache.org>.
jbewing commented on PR #14683:
URL: https://github.com/apache/iceberg/pull/14683#issuecomment-3757782724
> I can understand the frustration, but I also understand that the OSS community is volunteering time to review the changes (most of the time outside of their work; I'm sure it is the same for you). Also, some of us just came back from the holiday season. I've also found it useful to reach out on the [dev list](https://iceberg.apache.org/community/#mailing-lists) for reviews. We could also join the [community sync meetings](https://iceberg.apache.org/community/#apache-iceberg-community-calendar) to seek reviews if you have time.
Yeah I totally get it. I've just had a slew of _interesting_ experiences in this project, so I want to be certain that if I'm putting in the work that someone else is in it with me 😄 .
> I'll discuss this PR and my https://github.com/apache/iceberg/pull/14948 that reports this ordering to Spark to the Iceberg Spark community sync next week to gather feedback. I'm happy to help with reviews, but I'm not a committer. We will need a committer to take a look as well.
Sweet yeah as I allude to in the PR description, I also have a patch that does this on a private fork. It works quite well in a narrower set of circumstances—you need to ensure that partitions don't get _too_ skewed or large. But in the set of circumstances that things are roughly even, the performance improvement is drastic. I'd be happy to take a look at your implementation and give feedback/lessons learned from ours.
> I am willing to help as well :) I just introduced a new child to my family so I have been gone for a few of those months but I will do my best to stay on top of any new developments. Feel free to ping me on slack as well
Sounds good! Totally understand—life happens. I just wanted to make sure before putting a bunch of effort in that it would be matched this time. I look forwards to working together!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark (4.0, 3.5): Set data file `sort_order_id` in manifest for writes from Spark [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on PR #14683:
URL: https://github.com/apache/iceberg/pull/14683#issuecomment-3756973188
I am willing to help as well :) I just introduced a new child to my family so I have been gone for a few of those months but I will do my best to stay on top of any new developments. Feel free to ping me on slack as well
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark (4.0, 3.5): Set data file `sort_order_id` in manifest for writes from Spark [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on PR #14683:
URL: https://github.com/apache/iceberg/pull/14683#issuecomment-3781260428
One single PR against 4.1 would be ideal IMHO. Unless you have multiple independent bits you want to get in one at a time.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark (4.0, 3.5): Set data file `sort_order_id` in manifest for writes from Spark [iceberg]
Posted by "jbewing (via GitHub)" <gi...@apache.org>.
jbewing commented on PR #14683:
URL: https://github.com/apache/iceberg/pull/14683#issuecomment-3797710089
Hey @anuragmantri & @RussellSpitzer , will do. I thought I was going to be able to get to it this weekend but wasn't able to find the time, but should have some time in the next couple days. Just wanted to give an update to let you know that I had seen this and am getting to it (just not as quickly as I would've liked to)
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark (4.0, 3.5): Set data file `sort_order_id` in manifest for writes from Spark [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #14683:
URL: https://github.com/apache/iceberg/pull/14683#issuecomment-3691844814
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark (4.0, 3.5): Set data file `sort_order_id` in manifest for writes from Spark [iceberg]
Posted by "jbewing (via GitHub)" <gi...@apache.org>.
jbewing commented on code in PR #14683:
URL: https://github.com/apache/iceberg/pull/14683#discussion_r2560768927
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/SparkWriteRequirements.java:
##########
@@ -48,12 +62,19 @@ public SortOrder[] ordering() {
return ordering;
}
+ public org.apache.iceberg.SortOrder icebergOrdering() {
+ return icebergOrdering;
+ }
Review Comment:
I'm not in love with this approach, but I went through a couple of iterations of this and plumbing the intended Iceberg SortOrder along through with the Spark SortOrder ended up being the cleanest in my opinion (open to any suggestions of course). I initially had an approach which would attempt to re-guess the Iceberg SortOrder from a Spark SortOrder. It worked pretty well, but was pretty naive in my opinion—the algorithm was to essentially reverse the Iceberg -> Spark SortOrder transformation as best as possible and then see if things matched up. It gets complicated when you start to consider Spark Orderings for stuff like say a positionDeltaUpdateMergeOrdering.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark (4.0, 3.5): Set data file `sort_order_id` in manifest for writes from Spark [iceberg]
Posted by "jbewing (via GitHub)" <gi...@apache.org>.
jbewing commented on PR #14683:
URL: https://github.com/apache/iceberg/pull/14683#issuecomment-3757969246
Alright I've rebased to the latest master. I can split out the changes if you'd like @RussellSpitzer. It seems like 4.1 is state of the art now, so would you like a PR that:
- Is opened against 4.1
- Backport all clean branches in a follow up (I plan on taking it back to 4.0 & 3.5 as I recall the 3.4 backport being ugly)
Or would you like it if we kept the current PR and ported forward to 4.1 as it seems like there hasn't been an official iceberg release for Spark 4.1. I'm assuming both are fine as I think the 4.1 setup must be more recent so stuff will probably forward/backport super cleanly.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark (4.0, 3.5): Set data file `sort_order_id` in manifest for writes from Spark [iceberg]
Posted by "jbewing (via GitHub)" <gi...@apache.org>.
jbewing closed pull request #14683: Spark (4.0, 3.5): Set data file `sort_order_id` in manifest for writes from Spark
URL: https://github.com/apache/iceberg/pull/14683
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark (4.0, 3.5): Set data file `sort_order_id` in manifest for writes from Spark [iceberg]
Posted by "anuragmantri (via GitHub)" <gi...@apache.org>.
anuragmantri commented on PR #14683:
URL: https://github.com/apache/iceberg/pull/14683#issuecomment-3781220228
Hi @jbewing - We discussed this PR [in the community](https://docs.google.com/document/d/19nno1RoPznbbxKOZZddZNHHafa7XULjbN6RPExdr2n4/edit?tab=t.0), and I would like to help move these forward with the help of other reviewers.
It seems like something went wrong with the rebase. Could you please rebase your change and make a single PR with just 4.1? I will do the same with my PR.
Community prefers that way as it's easier to review. We can have a subsequent PR that back ports to all older branches (in one PR). Usually that will not take much time in reviews.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark (4.0, 3.5): Set data file `sort_order_id` in manifest for writes from Spark [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on PR #14683:
URL: https://github.com/apache/iceberg/pull/14683#issuecomment-3720278848
I'm very interested in this, but could you explain the changes in the PR in a summary? It looks like the distribution and ordering code is being redone?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark (4.0, 3.5): Set data file `sort_order_id` in manifest for writes from Spark [iceberg]
Posted by "jbewing (via GitHub)" <gi...@apache.org>.
jbewing commented on PR #14683:
URL: https://github.com/apache/iceberg/pull/14683#issuecomment-3803016583
@RussellSpitzer & @anuragmantri, I've opened up https://github.com/apache/iceberg/pull/15150. We can move conversation there as I'm closing this PR in favor of that one.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spark (4.0, 3.5): Set data file `sort_order_id` in manifest for writes from Spark [iceberg]
Posted by "anuragmantri (via GitHub)" <gi...@apache.org>.
anuragmantri commented on PR #14683:
URL: https://github.com/apache/iceberg/pull/14683#issuecomment-3756191864
I can understand the frustration, but I also understand that the OSS community is volunteering time to review the changes (most of the time outside of their work; I'm sure it is the same for you). Also, some of us just came back from the holiday season. I've also found it useful to reach out on the [dev list](https://iceberg.apache.org/community/#mailing-lists) for reviews. We could also join the [community sync meetings](https://iceberg.apache.org/community/#apache-iceberg-community-calendar) to seek reviews if you have time.
I'll discuss this PR and my [PR](https://github.com/apache/iceberg/pull/14948) that reports this ordering to Spark to the Iceberg Spark community sync next week to gather feedback. I'm happy to help with reviews, but I'm not a committer. We will need a committer to take a look as well.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org