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

build: Add Spark SQL test pipeline with ANSI mode enabled #321

Merged
merged 6 commits into from
May 3, 2024

Conversation

parthchandra
Copy link
Contributor

Closes #320 .

Rationale for this change

Setup a test pipeline to run Spark sql tests with ANSI modeenabled. This is part of the effort to reach full ANSI mode support

What changes are included in this PR?

Adds a new manual pipeline to run Spark SQL tests with ANSOI mode enabled.
Sets the environment variable ENABLE_COMET_ANSI_MODE=true before running the tests.

How are these changes tested?

Manually run spark tests on command line with Spark 3.4.2.

Comment on lines +1332 to +1333
+ val v = System.getenv("ENABLE_COMET_ANSI_MODE")
+ v != null && v.toBoolean
Copy link
Member

Choose a reason for hiding this comment

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

I think this could be simplified as:

Suggested change
+ val v = System.getenv("ENABLE_COMET_ANSI_MODE")
+ v != null && v.toBoolean
+ System.getenv("ENABLE_COMET_ANSI_MODE").exists(_.toBoolean)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Compiler didn't like the suggested change and I didn't quite understand why. Also, the way it is in the PR, matches the pattern followed by similar other code in the diff, so leaving it as it is.

@viirya
Copy link
Member

viirya commented Apr 25, 2024

Hmm, I think currently this is going to have failed pipeline, do you want to have this in CI? Or you want to hold it here until we have ANSI mode support to make this passed?

@parthchandra
Copy link
Contributor Author

Hmm, I think currently this is going to have failed pipeline, do you want to have this in CI? Or you want to hold it here until we have ANSI mode support to make this passed?

I think we should exclude this from ci but it should be available to run manually so we can run this after every improvement to ANSI support. Let me investigate how to make it manual/on demand.

@parthchandra
Copy link
Contributor Author

@liang-chi made this manual run only. Thanks for pointing that out!

@parthchandra
Copy link
Contributor Author

@viirya @andygrove, perhaps we can merge this?

@viirya viirya merged commit e48b12e into apache:main May 3, 2024
28 checks passed
@viirya
Copy link
Member

viirya commented May 3, 2024

Merged. Thanks.

@parthchandra
Copy link
Contributor Author

Thank you @viirya @andygrove

himadripal pushed a commit to himadripal/datafusion-comet that referenced this pull request Sep 7, 2024
* build: Add Spark SQL test pipeline with ANSI mode enabled

* add ENABLE_COMET_ANSI_MODE to actual run of tests

* fix diff and rat exclusion

* fix diff

* Make workflow manual run only

* fix diff
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.

Add test pipeline to run spark sql tests in ANSI mode
3 participants