Skip to content

Fix nested scalar hashing#8804

Merged
robert3005 merged 2 commits into
developfrom
agent/fix-scalar-nested-hashing
Jul 17, 2026
Merged

Fix nested scalar hashing#8804
robert3005 merged 2 commits into
developfrom
agent/fix-scalar-nested-hashing

Conversation

@connortsui20

@connortsui20 connortsui20 commented Jul 16, 2026

Copy link
Copy Markdown
Member

Fixes #8744

Scalar equality ignores dtype nullability recursively, but hashing only removed top-level nullability. The typed scalar views had the same mismatch, so equal nested values could produce different hashes.

  • adds recursive nullability-insensitive dtype hashing matching DType::eq_ignore_nullability
  • uses it for Scalar, ListScalar, StructScalar, and ExtScalar
  • covers nested list, struct, Union dtype, and extension storage nullability regressions

This is kind of ugly but I don't think there is a better way to do this

@codecov

codecov Bot commented Jul 16, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.65217% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.82%. Comparing base (17f843a) to head (76745ac).
⚠️ Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
vortex-array/src/dtype/dtype_impl.rs 87.50% 6 Missing ⚠️

☔ View full report in Codecov by Harness.
📢 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.

@github-actions

github-actions Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Polar Signals Profiling Results

Latest Run

Status Commit Job Attempt Link
🟢 Done 277b386 1 Explore Profiling Data
Previous Runs (1)
Status Commit Job Attempt Link
🟢 Done 76745ac 1 Explore Profiling Data

Powered by Polar Signals Cloud

@github-actions

github-actions Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Benchmarks: Vortex queries

Verdict: No clear signal (low confidence)
Attributed Vortex impact: -5.8%
Engines: DataFusion No clear signal (-7.8%, medium confidence) · DuckDB No clear signal (-3.8%, medium confidence)
Vortex (geomean): 0.925x ➖
Parquet (geomean): 0.953x ➖
Shifts: Parquet (control) -4.7% · Median polish -6.5%

How to read Verdict and Engines
  • Verdict: Overall PR-level signal after subtracting baseline drift estimated from Parquet control rows. It can be Likely improvement, Likely regression, or No clear signal.
  • Engines: Per-engine attribution. DataFusion is compared against DataFusion/Parquet controls; DuckDB is compared against DuckDB/Parquet controls. This answers whether each engine improved or regressed independently.
  • Confidence: Based on directional consistency, share of rows above the noise floor, and control-run noise.

datafusion / vortex-file-compressed (0.869x ✅, 2↑ 0↓)
name PR 277b386 (ns) base 928738e (ns) ratio (PR/base)
vortex_q00/datafusion:vortex-file-compressed 🚀 9447509 10725993 0.88
vortex_q01/datafusion:vortex-file-compressed 🚀 6300238 7350345 0.86
datafusion / parquet (0.943x ➖, 0↑ 0↓)
name PR 277b386 (ns) base 928738e (ns) ratio (PR/base)
vortex_q00/datafusion:parquet 19917422 21355066 0.93
vortex_q01/datafusion:parquet 4556845 4783376 0.95
duckdb / vortex-file-compressed (0.927x ➖, 1↑ 0↓)
name PR 277b386 (ns) base 928738e (ns) ratio (PR/base)
vortex_q00/duckdb:vortex-file-compressed 🚀 9998247 11407955 0.88
vortex_q01/duckdb:vortex-file-compressed 6070606 6193463 0.98
duckdb / parquet (0.963x ➖, 0↑ 0↓)
name PR 277b386 (ns) base 928738e (ns) ratio (PR/base)
vortex_q00/duckdb:parquet 23461287 24228706 0.97
vortex_q01/duckdb:parquet 9394105 9799860 0.96

No file size changes detected.

@connortsui20
connortsui20 marked this pull request as ready for review July 16, 2026 20:31
@connortsui20
connortsui20 enabled auto-merge (squash) July 17, 2026 13:59
Comment thread vortex-array/src/dtype/dtype_impl.rs Outdated
@connortsui20
connortsui20 requested a review from lwwmanning July 17, 2026 14:41
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
@connortsui20
connortsui20 force-pushed the agent/fix-scalar-nested-hashing branch from f07de1b to 277b386 Compare July 17, 2026 14:45
@connortsui20
connortsui20 disabled auto-merge July 17, 2026 14:51
@robert3005
robert3005 merged commit cc6803a into develop Jul 17, 2026
74 checks passed
@robert3005
robert3005 deleted the agent/fix-scalar-nested-hashing branch July 17, 2026 14:58
@connortsui20

Copy link
Copy Markdown
Member Author

see #8826 for related discussion on these semantics

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.

Scalar hashing violates Eq for nested nullability

3 participants