[Json] Use partition and take in RunEndEncoded decoder#9658
[Json] Use partition and take in RunEndEncoded decoder#9658Jefffrey merged 5 commits intoapache:mainfrom
partition and take in RunEndEncoded decoder#9658Conversation
568cceb to
ffd5849
Compare
|
Local benchmark results |
alamb
left a comment
There was a problem hiding this comment.
Looks good to me -- thank you @liamzwbao
| arrow-buffer = { workspace = true } | ||
| arrow-cast = { workspace = true } | ||
| arrow-data = { workspace = true } | ||
| arrow-ord = { workspace = true } |
There was a problem hiding this comment.
These are fairly non trivial crates (ord and select) so it is sad to see the dependencies being added here
That being said, I think it is becoming clear that anything involving REE benefits from those two
Maybe we could split them up or osmething into new crates with the "core" parts (specifically partition and take) 🤔
arrow-take and arrow-partition maybe 🤔
I'll file a ticket to consider this
There was a problem hiding this comment.
There was a problem hiding this comment.
Worth noting that arrow-cast already brings in ord & select in it's dependencies, so I don't think this actually affects the build too much (compared to previous state)
There was a problem hiding this comment.
Worth noting that
arrow-castalready brings in ord & select in it's dependencies, so I don't think this actually affects the build too much (compared to previous state)
yeah, adding REE to cast required those new dependencies. We have some ideas of how to avoid it but they aren't great
| let values_data = mutable.freeze(); | ||
| let run_ends_data = | ||
| PrimitiveArray::<R>::new(ScalarBuffer::from(run_ends), None).into_data(); | ||
| let indices = UInt32Array::from_iter_values(indices.into_iter().map(|i| i as u32)); |
There was a problem hiding this comment.
In theory the old code could also handle usize indices not ust u32 but I think in practice it won't matter
There was a problem hiding this comment.
Yes, also indices are bounded by the tape pos (which is &[u32]), so they should never exceed u32::MAX IIUC
Jefffrey
left a comment
There was a problem hiding this comment.
This behaviour of efficiently generating a REE from a flat array seems generic enough to be worth splitting into a separate function (that maybe other users might need), but could be a followup 🤔
It also exists in the cast kernels -- so refactoring that out (or reusing the cast) would be good |
Which issue does this PR close?
Rationale for this change
What changes are included in this PR?
Optimize RunEndEncoded decoder to use
partitionandtake, substantially improving performance (over 2x speedup).Are these changes tested?
Covered by existing tests
Are there any user-facing changes?
No