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

REE infers length from ends array #147

Merged
merged 2 commits into from
Apr 3, 2024
Merged

REE infers length from ends array #147

merged 2 commits into from
Apr 3, 2024

Conversation

robert3005
Copy link
Member

@robert3005 robert3005 commented Mar 26, 2024

fixes #19

@robert3005
Copy link
Member Author

Will reopen this later, need to bring back some more scalar_at impls

@robert3005 robert3005 closed this Mar 26, 2024
@lwwmanning lwwmanning reopened this Apr 3, 2024
@lwwmanning lwwmanning enabled auto-merge (squash) April 3, 2024 19:56
@lwwmanning lwwmanning merged commit 1a5c2fe into develop Apr 3, 2024
2 checks passed
@lwwmanning lwwmanning deleted the rk/ree-length branch April 3, 2024 20:03
@lwwmanning
Copy link
Member

possible now that #160 has merged 🥳

@jdcasale
Copy link
Contributor

jdcasale commented Apr 4, 2024

This PR introduces a panic when running bench compress, investigating.

RUST_BACKTRACE=1 cargo run -r --color=always --bin compress --manifest-path /Users/jcasale/fulcrum/vortex/bench-vortex/Cargo.toml

assertion `left == right` failed
  left: 1024
 right: 0
stack backtrace:
   0: _rust_begin_unwind
   1: core::panicking::panic_fmt
   2: core::panicking::assert_failed_inner
   3: core::panicking::assert_failed
   4: vortex_ree::ree::REEArray::try_new
   5: vortex_ree::compress::<impl vortex::compress::EncodingCompression for vortex_ree::ree::REEEncoding>::compress
   6: vortex::compress::find_best_compression
   7: vortex::compress::sampled_compression
   8: vortex::compress::CompressCtx::compress_array
   9: <alloc::vec::Vec<T> as alloc::vec::spec_from_iter::SpecFromIter<T,I>>::from_iter
  10: core::iter::adapters::try_process
  11: vortex::compress::CompressCtx::compress_array
  12: vortex::compress::CompressCtx::compress
  13: core::ops::function::impls::<impl core::ops::function::FnOnce<A> for &mut F>::call_once
  14: <alloc::vec::Vec<T> as alloc::vec::spec_from_iter::SpecFromIter<T,I>>::from_iter
  15: bench_vortex::reader::compress_parquet_to_vortex
  16: compress::main
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

@robert3005
Copy link
Member Author

@jdcasale it's #193. In cases where it throws the ends array is bitpacked and it must be bitpacked to 0 bits with patches

robert3005 added a commit that referenced this pull request Apr 4, 2024
jdcasale added a commit that referenced this pull request Apr 4, 2024
…y respect patches (#194)

before:
Issue: #193

#147 uncovered that we did not
respect patches in scalar_at calculations, causing a panic when REE ends
arrays were bitpacked with patches.


After:
* We have a validity is implementation for BitPackedArray
* ScalarAtFn for BitPackedArray no longer ignores patches
* benches/compress no longer panics
* flatten respects fill_value

---------

Co-authored-by: Will Manning <[email protected]>
Co-authored-by: Robert Kruszewski <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

REEArray should infer length from values and not take it in constructor
3 participants