Skip to content
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

fix: replace BTreeMap with IndexMap to preserve insertion order #2150

Merged
merged 1 commit into from
Feb 1, 2024

Conversation

roeap
Copy link
Collaborator

@roeap roeap commented Jan 31, 2024

Description

When switching to using BTreeMap for representing partition values, I introduced a bug, thinking BTreeMap would preserve insertion order. While it is ordered, it is ordered based on keys.

This PR moves to using IndexMap, which actually preserves insertion order.

@github-actions github-actions bot added the binding/rust Issues for the Rust crate label Jan 31, 2024
@rtyler rtyler added this to the Rust v0.17 milestone Feb 1, 2024
@roeap roeap enabled auto-merge (squash) February 1, 2024 18:03
Copy link
Member

@rtyler rtyler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish there was a test to prove that the orderingness of IndexMap fixed a problem with BTreeMap, but oh well, let's roll 😄

@roeap roeap merged commit 5518261 into delta-io:main Feb 1, 2024
20 checks passed
@roeap roeap deleted the partition-values-order branch February 1, 2024 22:26
RobinLin666 pushed a commit to RobinLin666/delta-rs that referenced this pull request Feb 2, 2024
…a-io#2150)

# Description

When switching to using `BTreeMap` for representing partition values, I
introduced a bug, thinking `BTreeMap` would preserve insertion order.
While it is ordered, it is ordered based on keys.

This PR moves to using `IndexMap`, which actually preserves insertion
order.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
binding/rust Issues for the Rust crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants