Skip to content

[persist] Upgrade arrow to fix a minor source roundtripping issue #32655

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

bkirwi
Copy link
Contributor

@bkirwi bkirwi commented Jun 4, 2025

Upgrading to pick up this upstream arrow fix: apache/arrow-rs#6953

Motivation

https://github.com/MaterializeInc/database-issues/issues/9318

Tips for reviewer

There were a couple minor API changes to adapt to as well, due to the upgrade, which explains most of the diff. (The largest is that chrono added a new method to a trait, shadowing our own method - I've adapted the code to use the upstream version instead.)

I've committed the proptest failure, though note that it's still hard to reproduce even so... the bug only appears when two floats hash to the same bucket in a hash table, which is cot common. I could only repro the original bug after several hours of cargo stress; so while I'm pretty confident this is the fix, it's hard to prove with testing.

Checklist

  • This PR has adequate test coverage / QA involvement has been duly considered. (trigger-ci for additional test/nightly runs)
  • This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
  • If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
  • If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
  • If this PR includes major user-facing behavior changes, I have pinged the relevant PM to schedule a changelog post.

@bkirwi bkirwi force-pushed the source-fix-arrow branch from 5e9a5ad to 311f684 Compare June 4, 2025 17:23
@bkirwi bkirwi changed the title [persist] Upgrade arrow to fix a (minor) source roundtripping issue [persist] Upgrade arrow to fix a minor source roundtripping issue Jun 4, 2025
@bkirwi bkirwi force-pushed the source-fix-arrow branch from 311f684 to 9317dc9 Compare June 5, 2025 14:42
@bkirwi bkirwi marked this pull request as ready for review June 5, 2025 14:58
@bkirwi bkirwi requested review from a team as code owners June 5, 2025 14:58
@bkirwi bkirwi requested a review from aljoscha June 5, 2025 14:58
@bkirwi
Copy link
Contributor Author

bkirwi commented Jun 5, 2025

Nightly run: https://buildkite.com/materialize/nightly/builds/12214

The only failure seems to already exist on main.

bkirwi added 2 commits June 6, 2025 12:02
This is still hard to repro, since it depends on random state in the
parquet hasher... but hopefully it won't take a few hours next time.
@bkirwi bkirwi force-pushed the source-fix-arrow branch from 9317dc9 to 5017a8b Compare June 6, 2025 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant