Repository navigation
test: migrate regex extraction and position coverage to SQL fixtures - #6882
Conversation
Part of apache#6627. Preserve Java regex semantics and dispatcher routing while replacing 18 Scala tests, including multi-batch extraction coverage.
sunchao
left a comment
There was a problem hiding this comment.
Reviewed the full diff from cf6673808dc92d0be7ed331431fe9a2e97067a7d to 816499f434f6f1e733e59c7eb5adef6c391ed4bd: one commit, five files. The PR is non-draft. The snapshot and live discussion contain no reviews, comments, or review threads.
Routed skills: review-comet-pr and review-comet-expression-pr.
Summary
- Prior state and problem: Extraction and position coverage was split between SQL fixtures and 18 Scala tests. This PR consolidates those cases into the existing SQL test framework.
- Design approach: Extend the three default regex fixtures, add a dedicated 5,000-row extraction fixture, and remove the corresponding Scala tests. Production code is unchanged.
- Correctness: Checked the removed tests against their replacements. All 60 distinct subject/pattern/group combinations from the removed non-batch tests remain covered, including the separate three-row all-NULL table. Spark source confirms extraction group semantics, NULL propagation, empty-string/empty-array no-match results, and
regexp_instrreturning the whole match’s 1-based position regardless of the tested third-argument values. No introduced P1/P2 correctness issue was identified. - Compatibility analysis: Compared implementations and relevant tests for Spark
3.4.3,3.5.9,4.0.4,4.1.3, and4.2.0. Their differences do not invalidate these plain-string cases. Compared all touched files withbranch-1.1. This PR intentionally reorganizes and strengthens coverage without changing released runtime behavior. - Key design decisions: Raw SQL literals preserve regex backslashes.
expect_dispatch(...)checks both Spark-equivalent answers and JVM dispatch. Including subjects and explicit row IDs prevents result comparison from overlooking incorrect associations between inputs and outputs. - Implementation sketch: Three expanded Parquet-backed fixtures cover extraction and position cases. The new fixture uses
range(0, 5000, 1, 1)andspark.comet.batchSize=64. Its generated inputs preserve 715 NULL and 4,285 non-NULL subjects. Existing fixture discovery registers it automatically. - Performance: There is no production-path overhead. Consolidation removes repeated Scala table setup, while the batch test remains bounded at 5,000 rows. CI records that fixture completing successfully in approximately 234 ms. No comparative performance improvement is claimed.
- Design: Reusing the established SQL harness is straightforward and keeps result and dispatch assertions together. The separate batch fixture isolates its configuration, which the harness restores afterward.
- Abstraction & complexity: No new abstraction or framework machinery is introduced. Removing repetitive Scala setup simplifies maintenance while retaining specialized combined-expression, error, stress, and collation tests.
- Behavioral changes worth calling out: The migrated coverage explicitly requires JVM dispatch. The batch fixture now specifies a small batch size and retains row identity. Native opt-in fixtures remain unchanged, including their two pre-existing ignored lookahead queries.
- Suggested improvements: No additional change meeting the P1/P2 reporting bar was identified.
Exact-head CI: 26 checks succeeded and 15 were skipped, with no failures. Required Checks, Linux native build/tests, default Spark 4.1 execution suites, and compatibility lint checks passed. The expressions log confirms all four affected fixtures passed. The remaining CometRegExpJvmSuite had 55 passes and one unchanged Spark-4.0-only cancellation. Spark SQL profile jobs, macOS, and Iceberg jobs were skipped.
Validation limits: Local validation consisted of a mechanical migration-coverage check and git diff --check, both successful. I did not rebuild or rerun JVM/native tests locally. Runtime evidence comes from exact-head CI. Other supported Spark profiles were checked against source but were not executed for this review, and I did not independently measure Arrow batch counts.
No introduced P1/P2 issues found within this review.
Which issue does this PR close?
Part of #6627. Other regex categories remain separate migrations. This branch is independent of #6863, which migrates Java RLIKE tests.
Rationale for this change
Move regexp_extract, regexp_extract_all and regexp_instr coverage into SQL fixtures while preserving the original Java regex semantics and input cases. The fixtures explicitly assert JVM codegen dispatch and compare the results with Spark.
What changes are included in this PR?
How are these changes tested?
Spark 4.1.3 / Scala 2.13.17 / JDK 21, root-reactor build with native JNI whose source and checksum match current main:
No migrated query is skipped. The unchanged opt-in fixtures retain their two pre-existing ignored lookahead queries.
Run the remaining supported Spark profiles with
run-all-spark-profilesbefore queueing; they have not yet been verified on this branch.