Skip to content

Conversation

@danking
Copy link
Contributor

@danking danking commented Sep 3, 2025

Fixes #4503

Pco mistakenly used the number of values rather than the number of valid values.

@danking danking requested review from a10y and connortsui20 September 3, 2025 19:08
Signed-off-by: Daniel King <dan@spiraldb.com>
@danking danking force-pushed the dk/test-pco-with-nullable branch from 757756d to fb837a5 Compare September 3, 2025 19:09
@danking danking marked this pull request as ready for review September 3, 2025 19:09
@danking danking enabled auto-merge (squash) September 3, 2025 19:09
Comment on lines +48 to +55
fn test_compress_decompress_big() {
let array = PrimitiveArray::from_option_iter([None, Some(1)]);
let compressed = PcoArray::from_primitive(&array, 3, 0).unwrap();
assert_eq!(compressed.scalar_at(0), Scalar::null_typed::<i32>());
assert_eq!(compressed.scalar_at(1), Scalar::from(Some(1)));
let decompressed = compressed.decompress();
assert_eq!(decompressed.scalar_at(0), Scalar::null_typed::<i32>());
assert_eq!(decompressed.scalar_at(1), Scalar::from(Some(1)));
Copy link
Contributor

Choose a reason for hiding this comment

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

Which one should panic? there are 4 different asserts here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's actually the decompression that panics. I'll add a message check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I found the fix.

Signed-off-by: Daniel King <dan@spiraldb.com>
@danking danking added the changelog/chore A trivial change label Sep 3, 2025
Fixes #4503.

Pco mistakenly used the number of values rather than the number of _valid_ values.

Signed-off-by: Daniel King <dan@spiraldb.com>
@danking danking changed the title test: add a should_panic test (for now) for #4503 fix: #4503 Sep 3, 2025
@danking danking added changelog/fix A bug fix and removed changelog/chore A trivial change labels Sep 3, 2025
@codecov
Copy link

codecov bot commented Sep 3, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.93%. Comparing base (d135f4a) to head (b6e7eed).
⚠️ Report is 7 commits behind head on develop.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@danking danking merged commit e202a68 into develop Sep 3, 2025
48 checks passed
@danking danking deleted the dk/test-pco-with-nullable branch September 3, 2025 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog/fix A bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: pco fails to round-trip the size two i32 array: [null, 1].

4 participants