Skip to content

Conversation

@gengliangwang
Copy link
Member

@gengliangwang gengliangwang commented Mar 10, 2022

What changes were proposed in this pull request?

Add Github action test job for ANSI SQL mode. It will be triggered after each commit push on the master branch of apache/spark.
image

To make the implementation easy, ANSI SQL mode becomes enabled if ENV variable SPARK_ANSI_SQL_MODE is set as true

Why are the changes needed?

Testing the ANSI SQL mode on each commit push, so that we can find issues in time.

Does this PR introduce any user-facing change?

Yes, ANSI SQL mode becomes enabled if ENV variable SPARK_ANSI_SQL_MODE is set as true. This won't be in documentation since it is for testing purpose.

How was this patch tested?

Manually try on my repo: https://github.com/gengliangwang/spark/actions/runs/1961176426

.version("3.0.0")
.booleanConf
.createWithDefault(false)
.createWithDefault(sys.env.get("SPARK_ANSI_SQL_MODE").contains("true"))
Copy link
Contributor

Choose a reason for hiding this comment

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

will sys.env.get("SPARK_ANSI_SQL_MODE") be null?

Copy link
Member Author

Choose a reason for hiding this comment

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

The result type of sys.env.get("SPARK_ANSI_SQL_MODE") is Option[String]. It is totally fine here.


jobs:
ansi_sql_test:
uses: ./.github/workflows/build_and_test.yml
Copy link
Member

Choose a reason for hiding this comment

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

oh wow. This is nice. Now we can reuse workflow files. cc @dongjoon-hyun FYI

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. I suggest that we move all the scheduled workflows to separated files.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, we should separate them. This is nice. @Yikun too FYI

Copy link
Member

Choose a reason for hiding this comment

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

Ya, it looks like a nice tip. Thanks, @HyukjinKwon .

Copy link
Member

@Yikun Yikun Mar 10, 2022

Choose a reason for hiding this comment

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

Cool, really good, also is an inspiration to reuse workflow in downstream repo.

Looks like there are some limitations we can only separated our workflow which is not using strategy.

[1] https://docs.github.com/en/actions/using-workflows/reusing-workflows#limitations

Copy link
Member Author

Choose a reason for hiding this comment

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

@Yikun the limitation is for the caller. The strategy in "build_and_test.yml" still works.

@gengliangwang gengliangwang changed the title [WIP][SPARK-38490][SQL][INFRA] Add Github action test job for ANSI SQL mode [SPARK-38490][SQL][INFRA] Add Github action test job for ANSI SQL mode Mar 10, 2022
@gengliangwang
Copy link
Member Author

gengliangwang commented Mar 10, 2022

This one is ready. The test result https://github.com/gengliangwang/spark/actions/runs/1961390528 is the same as changing the SQLConf manually: #34970

@gengliangwang
Copy link
Member Author

@cloud-fan @HyukjinKwon @dongjoon-hyun @Yikun Thanks for the reviews!
Merging to master.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants