Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SPARK-50380][SQL][TESTS][FOLLOWUP] Enable ANSI for conditional branches with error expression test #48943

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dongjoon-hyun
Copy link
Member

@dongjoon-hyun dongjoon-hyun commented Nov 23, 2024

What changes were proposed in this pull request?

This is a follow-up to recover non-ANSI CI.

Why are the changes needed?

The original PR broke non-ANSI CI because the test case assumes ANSI setting.

Does this PR introduce any user-facing change?

No, this is a test-only change.

How was this patch tested?

Manual tests.

BEFORE

$ SPARK_ANSI_SQL_MODE=false build/sbt "catalyst/testOnly *.ReorderAssociativeOperatorSuite -- -z SPARK-50380"
...
[info] *** 1 TEST FAILED ***
[error] Failed tests:
[error] 	org.apache.spark.sql.catalyst.optimizer.ReorderAssociativeOperatorSuite
[error] (catalyst / Test / testOnly) sbt.TestsFailedException: Tests unsuccessful
[error] Total time: 8 s, completed Nov 23, 2024, 11:50:45 AM

AFTER

$ SPARK_ANSI_SQL_MODE=false build/sbt "catalyst/testOnly *.ReorderAssociativeOperatorSuite -- -z SPARK-50380"
...
[info] ReorderAssociativeOperatorSuite:
[info] - SPARK-50380: conditional branches with error expression (508 milliseconds)
[info] Run completed in 1 second, 413 milliseconds.
[info] Total number of tests run: 1
[info] Suites: completed 1, aborted 0
[info] Tests: succeeded 1, failed 0, canceled 0, ignored 0, pending 0
[info] All tests passed.
[success] Total time: 11 s, completed Nov 23, 2024, 11:51:34 AM

Was this patch authored or co-authored using generative AI tooling?

No.

@dongjoon-hyun
Copy link
Member Author

cc @cloud-fan and @viirya

@viirya
Copy link
Member

viirya commented Nov 23, 2024

This is fine normally, but we added a new contract in ConstantFolding with #36468 , due to the introduction of ANSI mode and we don't want to fail eagerly for expressions within conditional branches. ReorderAssociativeOperator does not follow this contract.

Okay, I think the behavior is only for ANSI mode, according to the PR description.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants