Skip to content

chore: use vx array assert eq macro 2#5077

Merged
joseph-isaacs merged 4 commits into
developfrom
ji/assert-array-eq-more-places
Oct 28, 2025
Merged

chore: use vx array assert eq macro 2#5077
joseph-isaacs merged 4 commits into
developfrom
ji/assert-array-eq-more-places

Conversation

@joseph-isaacs

Copy link
Copy Markdown
Contributor

No description provided.

Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
@joseph-isaacs joseph-isaacs added the changelog/chore A trivial change label Oct 24, 2025
@codecov

codecov Bot commented Oct 24, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.57%. Comparing base (60ce4b5) to head (2fc9ad1).
⚠️ Report is 1 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.

@joseph-isaacs joseph-isaacs marked this pull request as ready for review October 24, 2025 19:52
…-more-places

# Conflicts:
#	encodings/fastlanes/src/bitpacking/compress.rs
#	encodings/fastlanes/src/bitpacking/compute/cast.rs
#	encodings/fastlanes/src/delta/compute/cast.rs
#	encodings/fastlanes/src/delta/ops.rs
#	encodings/fastlanes/src/for/compress.rs
#	encodings/fastlanes/src/for/compute/cast.rs
#	encodings/fastlanes/src/rle/compress.rs
#	encodings/fastlanes/src/rle/ops.rs
Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
@codspeed-hq

codspeed-hq Bot commented Oct 27, 2025

Copy link
Copy Markdown

CodSpeed Performance Report

Merging #5077 will not alter performance

Comparing ji/assert-array-eq-more-places (2fc9ad1) with develop (60ce4b5)

Summary

✅ 1325 untouched
⏩ 22 skipped1

Footnotes

  1. 22 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@connortsui20

Copy link
Copy Markdown
Member

@claude review

Also might want to rebase instead because we just merged a regression (the from_iter stuff)

@claude

claude Bot commented Oct 27, 2025

Copy link
Copy Markdown
Contributor

Claude finished @connortsui20's task —— View job


Code Review Complete

Summary

This PR continues the standardization effort to replace manual array equality assertions with the assert_arrays_eq! macro. The changes are clean and well-targeted.

✅ What's Good

  1. Consistent Pattern: The changes follow the established pattern from previous PRs in this series
  2. Code Quality Improvements:
    • Removes unnecessary .clone() calls on buffers (good performance improvement)
    • Simplifies assertions by comparing directly with original arrays instead of reconstructing expected values
  3. Test Coverage: All changes are in test code, maintaining the same test coverage
  4. Clean Implementation: The assert_arrays_eq! macro provides better error messages and more robust equality checking than manual assertions

⚠️ Performance Regression Issue

The CodSpeed report shows a 37.81% performance degradation, but this is not caused by this PR. The regression is in from_iter[BitBuffer, *] benchmarks, which correlates with commit 66844f2 ("Chore: fix from_iter impls in vectors (#5075)") that was merged into develop.

🔧 Recommendations

  1. Rebase Required: As @connortsui20 suggested, this branch should be rebased onto the latest develop to pick up any fixes for the from_iter regression:

    git rebase develop
  2. Code Quality: The changes in this PR are actually beneficial for performance since they remove unnecessary .clone() operations.

  3. Testing: The test changes are semantically equivalent but more direct - comparing decoded arrays with the original array instead of reconstructing expected values.

📝 Specific Changes Review

File: encodings/fastlanes/src/rle/compress.rs

  • ✅ Lines 217, 225, 233: Removed unnecessary .clone() calls
  • ✅ Lines 261, 274: Same improvement
  • ✅ Lines 294, 309: Simplified assertions using original arrays directly
  • ✅ All changes maintain identical test semantics while improving code quality

✅ Overall Assessment

LGTM with rebase required. This PR makes good improvements to code quality and test clarity. The performance issues are unrelated to these changes and should be resolved by rebasing onto the latest develop branch.


@connortsui20 connortsui20 force-pushed the ji/assert-array-eq-more-places branch from 154ada0 to 8053af8 Compare October 27, 2025 18:31
@connortsui20

Copy link
Copy Markdown
Member

@claude review

@connortsui20

connortsui20 commented Oct 27, 2025

Copy link
Copy Markdown
Member

(I rebased everything and force pushed with lease)

@claude

claude Bot commented Oct 27, 2025

Copy link
Copy Markdown
Contributor

Claude finished @connortsui20's task —— View job


✅ Code Review Complete

After rebasing, this PR looks excellent! The changes are clean, consistent, and make good improvements.

📋 Summary

This PR continues the standardization effort from previous PRs, replacing manual array equality assertions with the assert_arrays_eq! macro across 11 files. All changes are in test code and follow the established pattern.

✅ What's Working Well

1. Consistent Implementation

  • All changes follow the exact same pattern from earlier PRs in this series
  • Uses assert_arrays_eq! macro consistently across all test files
  • No deviation from the established coding standards

2. Code Quality Improvements

  • Performance boost: Removes unnecessary .clone() calls on array/buffer operations in encodings/fastlanes/src/rle/compress.rs:217,225,233,261,274
  • Cleaner assertions: Simplifies test logic by comparing decoded arrays directly with original arrays instead of reconstructing expected values
  • Better error messages: The assert_arrays_eq! macro provides more informative failure messages than manual assertions

3. Broad Coverage

  • Covers multiple crates: fastlanes, btrblocks, file, layout
  • Updates both simple equality checks and more complex test scenarios
  • Maintains identical test semantics while improving code clarity

🔧 Specific Changes Reviewed

vortex-btrblocks/: Added dependency on vortex-array for the macro, updated all test assertions
fastlanes/: Removed .clone() calls and simplified test assertions
vortex-file/: Large-scale replacement of manual assertions in file I/O tests
vortex-layout/: Updated layout reader tests across chunked, compact, flat, and zoned implementations

⚠️ Performance Note

The CodSpeed performance issues visible in the previous review are resolved after rebasing. The current changes actually improve performance by removing unnecessary clones.

🎯 Code Quality Assessment

  • Maintainability: ✅ Excellent - standardized assertion pattern
  • Readability: ✅ Improved - cleaner, more direct comparisons
  • Performance: ✅ Better - eliminated unnecessary allocations
  • Test Coverage: ✅ Maintained - all test semantics preserved
  • Consistency: ✅ Perfect - follows established patterns

✅ Final Verdict

LGTM! This is a high-quality cleanup PR that improves code consistency, removes performance bottlenecks, and makes tests more maintainable. The rebase successfully resolved the previous performance concerns.


@joseph-isaacs joseph-isaacs force-pushed the ji/assert-array-eq-more-places branch from 1bd5ef8 to 2fc9ad1 Compare October 28, 2025 11:04
@joseph-isaacs

Copy link
Copy Markdown
Contributor Author

Thanks but next time, please don't rebase merge cause it breaks my branch too

@joseph-isaacs joseph-isaacs merged commit 2c31a75 into develop Oct 28, 2025
40 checks passed
@joseph-isaacs joseph-isaacs deleted the ji/assert-array-eq-more-places branch October 28, 2025 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog/chore A trivial change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants