Repository navigation
Conversation
sunchao
left a comment
There was a problem hiding this comment.
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
queryrecords 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/1and values around2^32. Comparison withbranch-1.1confirms 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.
Which issue does this PR close?
Part of #6634.
Rationale for this change
SQL fixtures omit TINYINT, SMALLINT and BOOLEAN
bit_countcoverage. 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.sqland 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_countcodegen defect. Plain queries still assert Comet operators. Remaining hash and bitwise migrations are separate work.How are these changes tested?