Skip to content

test: cover lpad and rpad routing configurations - #5895

Open
rich7420 wants to merge 2 commits into
apache:mainfrom
rich7420:test/4616-padding-routing
Open

rich7420 wants to merge 2 commits into
apache:mainfrom
rich7420:test/4616-padding-routing

Conversation

@rich7420

@rich7420 rich7420 commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Part of #4616.

Rationale for this change

The padding SQL tests compare results without distinguishing native execution from JVM codegen dispatch. Separate Scala tests duplicate the routing coverage.

What changes are included in this PR?

Add expression-specific routing assertions to the four existing lpad/rpad SQL fixtures and remove the duplicate Scala routing tests. Preserve the original NULL, empty-padding and literal/column cases with the dispatcher enabled and disabled.

How are these changes tested?

All four SQL fixtures pass locally on Spark 3.5.9 and 4.1.3. The five retained padding tests also pass on Spark 4.1.3. A native-to-dispatch regression passes the old SQL fixtures but fails the new native assertions for both expressions.

@github-actions github-actions Bot added enhancement New feature or request test Testing related labels Sep 13, 2026
@andygrove

Copy link
Copy Markdown
Member

Thanks @rich7420. Maybe we could just delete these tests and re-implement in the Comet SQL suite?

@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.

Correctness

Summary

Reviewed 81fa972a498695fbe05015f5eabc3222dd576845 against f69c4c81b9429e327ea95658530ae4ed4ed19635. I found no actionable correctness issue. This changes one test file and leaves production routing and padding semantics unchanged.

The existing checks exercised unsupported shapes only with the dispatcher enabled and relied on global activity counters. The revised tests cover both dispatcher settings and name lpad or rpad explicitly. The shared assertion requires the expression to appear in its expected implementation set and be absent from the other. With dispatch disabled, the tests compare Spark results and require the expression-specific disabled-dispatcher reason.

The cases match the current serde contract: a column string with a literal pad stays native, while literal-string and nonliteral-pad shapes use dispatch or Spark fallback. Removing the allowIncompatible loop is consistent with these shapes being Unsupported, whose routing does not consult that setting. Disabling constant folding keeps the all-literal case meaningful.

I compared the tests with the maintained Spark 3.5 and 4.0 implementations and padding tests. They preserve Spark's null-intolerant behavior, default space padding, truncation, and literal/column argument handling. Existing tests retain negative/zero lengths, Unicode, null lengths, and binary coverage.

Validation

Current CI is green. I verified that the Spark 4.1 and Spark 4.0 expression jobs each passed all four changed routing tests. Their checkout was 2c4768268cb92c2286c3d51c3afe3413d8166f6e, whose parents are the reviewed base and head and whose tree equals the head. The four tests cover 20 query/configuration combinations.

The Spark 4.1 shard reports 1,386 passed with no failures or cancellations. Spark 4.0 reports 1,383 passed, no failures, and three cancellations outside these four tests. I did not run a local Spark build or independently repeat the author's injected-regression probes. Maintained Spark 3.4/4.1 source was unavailable, so CI execution is distinct from the canonical source comparison.

Performance

There is no production execution cost in this test-only change. Replacing global counter checks with per-plan expression assertions makes the routing checks independent of unrelated dispatcher activity. The matrix grows from 16 to 20 query/configuration combinations across tiny fixtures. No performance claim or microbenchmark is needed for this scope.

Design

Reusing checkSparkAnswerAndImpl and the existing fallback helper keeps result comparison and routing checks together. The existing SQL-suite suggestion is feasible: the SQL runner already supports expect_native, expect_dispatch, and expect_fallback through the same helpers. Moving the fixtures there can preserve these assertions. I have no additional design finding beyond that discussion.

Abstraction & complexity

The two small loops share coverage between left and right padding and between dispatcher settings. They add no helper abstraction or production indirection. Removing the expression-class-name mapping and global stats handling makes each test's expected route easier to read.

@rich7420

Copy link
Copy Markdown
Contributor Author

Thanks @rich7420. Maybe we could just delete these tests and re-implement in the Comet SQL suite?

sure, I'll try to do that

@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.

Re-reviewed c1615090. The SQL migration addresses the earlier suggestion: all 20 query/configuration combinations from the removed Scala routing tests are preserved, including native execution with dispatch disabled. The fixtures use the same expression-specific implementation and fallback assertions, keep the previous input cases, and disable constant folding. I found no new or remaining P1/P2 findings in this update.

I verified all four SQL fixtures and the five retained padding tests passing in the current CI logs for Spark 3.5, 4.0, and 4.1. CI executed merge 44ac055f, which includes newer base changes. All five reviewed files match that merge, and the relevant assertion helpers are equivalent. This is CI execution evidence. I did not run Spark/JNI locally or independently repeat the author’s mutation probe.

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