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

Implement Validity for SparseArray, make scalar_at for bitpacked array respect patches #194

Merged
merged 13 commits into from
Apr 4, 2024

Conversation

jdcasale
Copy link
Contributor

@jdcasale jdcasale commented Apr 4, 2024

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

@jdcasale
Copy link
Contributor Author

jdcasale commented Apr 4, 2024

Need to make flatten respect the supplied fill type still

vortex-alp/src/compress.rs Outdated Show resolved Hide resolved
vortex-array/src/array/sparse/compute.rs Outdated Show resolved Hide resolved
@gatesn gatesn marked this pull request as draft April 4, 2024 11:33
@jdcasale jdcasale marked this pull request as ready for review April 4, 2024 14:39
gatesn
gatesn previously requested changes Apr 4, 2024
vortex-alp/src/compress.rs Outdated Show resolved Hide resolved
vortex-array/src/array/sparse/compress.rs Outdated Show resolved Hide resolved
vortex-array/src/array/sparse/compute.rs Outdated Show resolved Hide resolved
vortex-array/src/array/sparse/compute.rs Outdated Show resolved Hide resolved
vortex-array/src/array/sparse/compute.rs Outdated Show resolved Hide resolved
vortex-array/src/array/sparse/compute.rs Show resolved Hide resolved
vortex-array/src/array/sparse/serde.rs Outdated Show resolved Hide resolved
vortex-fastlanes/src/bitpacking/compress.rs Outdated Show resolved Hide resolved
vortex-fastlanes/src/bitpacking/compute.rs Outdated Show resolved Hide resolved
vortex-fastlanes/src/bitpacking/compute.rs Outdated Show resolved Hide resolved
Comment on lines 38 to 41
assert!(
all_fill_types_are_equal,
"Cannot concatenate SparseArrays with differing fill values"
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be an error not a panic, i.e. wrap it in an if statement and do vortex_bail!

@jdcasale jdcasale enabled auto-merge (squash) April 4, 2024 17:28
@jdcasale jdcasale dismissed gatesn’s stale review April 4, 2024 17:29

Consensus reached

@jdcasale jdcasale merged commit 02d8180 into develop Apr 4, 2024
2 checks passed
@jdcasale jdcasale deleted the jc/fix_scalar_at branch April 4, 2024 17:29
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.

4 participants