Skip to content

test: let Spark 4 test profiles use the Spark-default ANSI mode#4370

Merged
andygrove merged 1 commit into
apache:mainfrom
andygrove:default-ansi
May 20, 2026
Merged

test: let Spark 4 test profiles use the Spark-default ANSI mode#4370
andygrove merged 1 commit into
apache:mainfrom
andygrove:default-ansi

Conversation

@andygrove
Copy link
Copy Markdown
Member

Which issue does this PR close?

Closes #.

Builds on the idea behind #4368.

Rationale for this change

CometTestBase was pinning spark.sql.ansi.enabled to false, so the Spark 4.0/4.1/4.2 test profiles never actually exercised the ANSI behavior real Spark 4 users get out of the box. Tests that pass against Comet today may regress in production because we never run them with the same defaults.

What changes are included in this PR?

  • Stop setting ANSI_ENABLED in CometTestBase.sparkConf. Spark 3.x picks up its own default (off), Spark 4.x picks up its own default (on).
  • For suites whose tests predominantly exercise silent-overflow / wrap-around / null-on-bad-input semantics, override sparkConf at the suite level to disable ANSI (tests that target ANSI behavior already opt in via withSQLConf(ANSI_ENABLED -> "true")):
    • CometFuzzMathSuite
    • CometCastSuite
    • CometAggregateSuite
  • CometSqlFileTestSuite: prepend ANSI_ENABLED -> "false" to the conf list passed into withSQLConf. The _ansi.sql fixtures that include --CONFIG: spark.sql.ansi.enabled=true still win, because their values are applied later.
  • Per-test withSQLConf(SQLConf.ANSI_ENABLED.key -> "false") wraps for the long tail in CometExpressionSuite, CometArrayExpressionSuite, CometMathExpressionSuite, CometDateTimeUtilsSuite, CometExecSuite, and ParquetReadSuite.

How are these changes tested?

The existing tests are the test. Ran the affected suites under -Pspark-4.0 locally (892 tests across 10 suites, 0 failures):

Suite Tests Result
CometFuzzMathSuite 30 pass
CometCastSuite 165 pass
CometAggregateSuite 81 pass
CometExpressionSuite 127 pass
CometArrayExpressionSuite 39 pass
CometMathExpressionSuite 7 pass
CometDateTimeUtilsSuite 8 pass
CometExecSuite 124 pass
CometSqlFileTestSuite 258 pass
ParquetReadV1Suite 53 pass

CI will sweep Spark 4.0/4.1/4.2 across all the partitions.

CometTestBase pinned spark.sql.ansi.enabled to false, which meant the
Spark 4.0/4.1/4.2 test profiles never exercised the ANSI behavior real
Spark 4 users get out of the box.

Stop setting the conf in the base; tests now run with each Spark
version's own default (3.x off, 4.x on). Suites and individual tests
that genuinely assume non-ANSI semantics opt out explicitly:

- Suite-level overrides: CometFuzzMathSuite, CometCastSuite, and
  CometAggregateSuite (most tests cover silent overflow / wrap-around).
- CometSqlFileTestSuite runner prepends ANSI=false to the conf list so
  the .sql fixture's own --Config line still wins for the _ansi.sql
  files that opt in to ANSI.
- Per-test withSQLConf wraps for the long tail in CometExpressionSuite,
  CometArrayExpressionSuite, CometMathExpressionSuite,
  CometDateTimeUtilsSuite, CometExecSuite, and ParquetReadSuite.
Copy link
Copy Markdown
Contributor

@mbutrovich mbutrovich left a comment

Choose a reason for hiding this comment

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

LGTM, do we need a plan to enhance the ANSI-specific test paths, or do we think those are well exercised by the existing tests that came in with ANSI-specific PRs?

Thanks @andygrove!

@andygrove andygrove merged commit 9e5af5a into apache:main May 20, 2026
76 checks passed
@andygrove andygrove deleted the default-ansi branch May 20, 2026 13:51
@andygrove
Copy link
Copy Markdown
Member Author

LGTM, do we need a plan to enhance the ANSI-specific test paths, or do we think those are well exercised by the existing tests that came in with ANSI-specific PRs?

Thanks @andygrove!

I think we're good. We have explicit ANSI opt-in testing for all supported expressions that have different behavior for ANSI mode.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants