Uh oh!
There was an error while loading. Please reload this page.
Use arrow IPC Stream format for spill files - #14868
Conversation
cc413d5 to
7ac9fdfComparexudong963
commented
Feb 25, 2025
Maybe it's better to add tests to cover your issue? |
davidhewitt
commented
Feb 25, 2025
Good point, I will add them probably tomorrow 👍 |
comphead
commented
Feb 25, 2025
2010YOUY01
left a comment
There was a problem hiding this comment.
Thank you for the nice work. I think it's ready to go after the regression test is added.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
7ac9fdf to
f904b63Comparedavidhewitt
commented
Feb 27, 2025
Rebased on main & test pushed 👍 |
| } | ||
| #[test] | ||
| fn test_batch_spill_and_read_dictionary_arrays() -> Result<()> { |
There was a problem hiding this comment.
I confirmed that this test fails on main with the error in #4658
davidhewitt
commented
Feb 27, 2025
#14078 looks like a similar problem but I suspect that the IPC Stream format is not much different in cost to the file format. At least with the test added here if the file format is changed again in the future, dictionary arrays will not regress. |
comphead
commented
Feb 27, 2025
Thanks @davidhewitt I think this PR is good, please check the clippy |
comphead
left a comment
There was a problem hiding this comment.
thanks @davidhewitt it is a really nice first contribution
davidhewitt
commented
Feb 27, 2025
Thanks! |
comphead
commented
Feb 28, 2025
Thanks @davidhewitt I let the PR some time for other approvers since this is a first contribution. I do not see any objections and planning to merge this PR, thanks again |
- remove test related to apache#14868 remove test
Which issue does this PR close?
Rationale for this change
The IPC Stream format allows for dictionary replacement, unlike the IPC File format. As per #4658 (comment) the File format does not offer advantages for the spill use case.
What changes are included in this PR?
Replaced the functionality in spilled sorts to write the IPC Stream format, instead of the IPC File format.
Are these changes tested?
Covered by existing tests of spill, I adapted these where necessary.
Are there any user-facing changes?
The code change is internal only; if for some reason users were inspecting the contents of spilled files without using the datafusion APIs to read them, they will find the format has changed.