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: properly deserialize percent-encoded file paths of Remove actions, to make sure tombstone and file paths match #2035

Merged
merged 10 commits into from
Jan 16, 2024

Conversation

sigorbor
Copy link
Contributor

@sigorbor sigorbor commented Jan 4, 2024

Description

Percent-encoded file paths of Remove actions were not properly deserialized, and when compared to active file paths, the paths didn't match, which caused tombstones to be recognized as active files (be kept in the state)

Related Issue(s)

Documentation

…ns, to make sure tombstone and file paths match
@github-actions github-actions bot added binding/rust Issues for the Rust crate crate/core labels Jan 4, 2024
@ion-elgreco
Copy link
Collaborator

@sigorbor can you add a small test?

@sigorbor
Copy link
Contributor Author

sigorbor commented Jan 7, 2024

@sigorbor can you add a small test?

Sure, done

@sigorbor sigorbor closed this Jan 7, 2024
@sigorbor sigorbor reopened this Jan 7, 2024
@sigorbor
Copy link
Contributor Author

sigorbor commented Jan 7, 2024

@ion-elgreco @rtyler - please review and approve

@sigorbor
Copy link
Contributor Author

sigorbor commented Jan 7, 2024

@ion-elgreco Integration tests are failing on a clearly unrelated issue.

@sigorbor
Copy link
Contributor Author

sigorbor commented Jan 8, 2024

@ion-elgreco @rtyler guys, can we proceed to merging the PR? It is approved and all the checks passed

@ion-elgreco ion-elgreco enabled auto-merge (squash) January 8, 2024 07:37
@sigorbor
Copy link
Contributor Author

@ion-elgreco this PR is approved, can it please be merged? BTW, are there any plans to release a new delta-rs Rust version soon? Any ETA? Would be great if it can happen soon, and include the current PR - we have customers waiting for this fix...

@ion-elgreco
Copy link
Collaborator

ion-elgreco commented Jan 16, 2024

@sigorbor I don't know when the next rust release will be planned, we are waiting for the arrow log functionality to be added and some other things.

If customers need it now, you can compile it from source and share that in the mean time to unblock them : ).

@sigorbor
Copy link
Contributor Author

thanks @ion-elgreco

"Integration tests" fail on apparently unrelated issue.. Could you please merge this PR?

@ion-elgreco ion-elgreco merged commit ba1b563 into delta-io:main Jan 16, 2024
20 checks passed
@ion-elgreco
Copy link
Collaborator

@sigorbor merged it

@roeap
Copy link
Collaborator

roeap commented Jan 16, 2024

sorry for chiming in so late, I held off on this PR since the entire serde logic will be more or less gone once #2037 lands.

Is there a specific scenario that needs to be considered in tests?

natinimni pushed a commit to natinimni/delta-rs that referenced this pull request Jan 31, 2024
…s, to make sure tombstone and file paths match (delta-io#2035)

Percent-encoded file paths of Remove actions were not properly
deserialized, and when compared to active file paths, the paths didn't
match, which caused tombstones to be recognized as active files (be kept
in the state)

<!---
For example:

- closes delta-io#106
--->

<!---
Share links to useful documentation
--->

---------

Co-authored-by: Igor Borodin <igborodi@microsoft.com>
Co-authored-by: Ion Koutsouris <15728914+ion-elgreco@users.noreply.github.com>
Co-authored-by: R. Tyler Croy <rtyler@brokenco.de>
RobinLin666 pushed a commit to RobinLin666/delta-rs that referenced this pull request Feb 2, 2024
…s, to make sure tombstone and file paths match (delta-io#2035)

# Description
Percent-encoded file paths of Remove actions were not properly
deserialized, and when compared to active file paths, the paths didn't
match, which caused tombstones to be recognized as active files (be kept
in the state)

# Related Issue(s)
<!---
For example:

- closes delta-io#106
--->

# Documentation

<!---
Share links to useful documentation
--->

---------

Co-authored-by: Igor Borodin <igborodi@microsoft.com>
Co-authored-by: Ion Koutsouris <15728914+ion-elgreco@users.noreply.github.com>
Co-authored-by: R. Tyler Croy <rtyler@brokenco.de>
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 crate/core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants