Skip to content

test: move bit_count boundary coverage to SQL fixtures - #6655

Queued
rich7420 wants to merge 3 commits into
apache:mainfrom
rich7420:test/6634-bit-count-boundaries
Queued

rich7420 wants to merge 3 commits into
apache:mainfrom
rich7420:test/6634-bit-count-boundaries

Conversation

@rich7420

@rich7420 rich7420 commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Part of #6634.

Rationale for this change

SQL fixtures omit TINYINT, SMALLINT and BOOLEAN bit_count coverage. Move the Scala boundary test into the SQL framework and retain its original cases while expanding coverage.

What changes are included in this PR?

Add bitwise/bit_count_types.sql and remove the corresponding Scala min/max test. Preserve the original rows, four integer types, boolean literals and both Parquet dictionary settings. Add a boolean column, NULLs, -1/0/1 and BIGINT values around 2^32.

Use interpreted Spark reference execution to avoid Spark 3.4.3's boolean bit_count codegen defect. Plain queries still assert Comet operators. Remaining hash and bitwise migrations are separate work.

How are these changes tested?

  • Current main-integrated head: both dictionary-setting variants passed on Spark 4.1.3 / JDK 21 with Comet operator assertions and a freshly built matching native library.
  • Root-reactor compilation, Spotless and Scalastyle passed. Historical verification before this main integration passed the final fixture on all five supported Spark profiles.
  • Current-head CI is running without reported failures. Native artifact creation and the exec job's artifact download succeeded. Additional Spark-profile runtime, Spark SQL and Iceberg suites have not run for this head.

@github-actions github-actions Bot added enhancement New feature or request test Testing related labels Oct 5, 2026
@rich7420
rich7420 marked this pull request as ready for review October 5, 2026 12:08

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Summary

  • Prior state and problem: Boundary coverage lived in a Scala test, while SQL fixtures lacked small-integer and Boolean coverage.
  • Design approach: Move the test into a typed Parquet fixture using the existing SQL runner and both dictionary settings.
  • Correctness / compatibility analysis: The original rows and integer types are preserved. Spark sources for 3.4.3, 3.5.9, 4.0.4, 4.1.3 and 4.2.0 confirm the expected integer, Boolean and null semantics. The documented Spark 3.4.3 Boolean codegen defect is present in its source.
  • Key design decisions: Interpreted Spark execution avoids that defect. Plain query records retain Comet operator assertions, and the runner disables constant folding for Boolean literals.
  • Implementation sketch: One automatically discovered fixture supplies two setup statements and two queries per dictionary variant, replacing one Scala test without adding infrastructure or abstractions.
  • Behavioral changes worth calling out: Coverage expands to Boolean columns, nulls, -1/0/1 and values around 2^32. Comparison with branch-1.1 confirms the same test-only migration. No production behavior or runtime overhead changes.
  • Suggested improvements: None at the requested severity threshold. No introduced P1/P2 issues found within this review.

Reviewed the entire two-file diff from a4e72fd9d4e3bb8f459fc7950a98536724daf859 to ac005262628a5012f00a234f465f6f9110804cb5. The PR is not a draft. Snapshot and live discussion checks contained no reviews, issue comments, inline comments or review threads. Routed skills: review-comet-pr and review-comet-expression-pr.

Validation: Both Spark 3.5.9 dictionary variants passed a local reference probe covering exact typed input rows, both queries, unfolded Boolean literals and Parquet encodings. The exact-head SqlFileTestParser probe and git diff --check passed.

Exact-head CI: Preflight, CodeQL, lint and supporting checks passed. Linux native build, Rust tests, Java lint and compatibility checks remain in progress, with no reported failures. macOS, Spark SQL and Iceberg suites were skipped. Validation limits: No exact-head Comet execution was run locally because this checkout has no built native library. Other Spark profiles were checked against source, not executed locally.

@andygrove andygrove left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @rich7420

@andygrove
andygrove enabled auto-merge October 8, 2026 15:37
@andygrove
andygrove added this pull request to the merge queue Oct 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request test Testing related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants