-
Notifications
You must be signed in to change notification settings - Fork 32
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: CompressionTrees diverge from the actual array children #1430
Conversation
@@ -1,8 +1,8 @@ | |||
mod tokio_runtime; | |||
|
|||
use core::cell::LazyCell; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getting ready for non_std
Vortex?
} | ||
EitherOrBoth::Left(Some(child_tree)) => { | ||
vortex_panic!( | ||
"Child tree without child array!!\nroot tree: {}\nroot array: {}\nlocal tree: {path}\nlocal array: {}\nproblematic child_tree: {child_tree}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I chose to panic rather than making this fallible because this is definitely programmer error (and also, we explore ~all possible permutations of encoding trees in the unit test suite)
this is split off from #1068 |
@lwwmanning need to look deeper but the whole idea of compression tree was that you could return a different array than the compressor. The compression tree would send you back to that path |
we only did that in the FoR compressor for constant 0, and I'm not sure that's right (i.e., if it's constant 0, the Constant compressor or should win over the FoR compressor with Constant child in this new implementation, rather than saying that we should encode-like with the FoRCompressor) |
also, of the 5 things I changed, I'd argued all except the FoR one are clearly bugs |
If we got to for compressor then that means array isn’t constant. Either stats are missing or it’s not constant. Not constant case is likely sparse 0s |
Yep, and in that case we should do Frequency encoding, not FoR |
😱 |
Fixed the following misalignments:
like.child(0)
Also improved the CompressionTree display impl to use TreeDisplay, which is much nicer & more useful for debugging. And I removed the
panic_in_result_fn
lint, since we banpanic
, but assertions are in fact quite nice sometimes.