Skip to content

Conversation

@dilipbiswal
Copy link
Contributor

What changes were proposed in this pull request?

Use the sbt maven plugin optionantlr4TreatWarningsAsErrors to make sure the warnings are treated as errors while generating the parser. In the absence of it, we may inadvertently introduce problems while making grammar changes. Please refer to PR-23897 to know more about the context. We made a change in pr-23925 which handled only the maven build.

In this PR, we handle the sbt build. I had submitted PR-23 to enhance the sbt-antlr plugin to make is possible to pass the error on warning option.

How was this patch tested?

Force an warning in the grammar file to check if the build fails. Then remove the warning to verify the build succeeds.

@SparkQA
Copy link

SparkQA commented Mar 11, 2019

Test build #103349 has finished for PR 24060 at commit c3624bf.

  • This patch fails to build.
  • This patch merges cleanly.
  • This patch adds no public classes.

@dongjoon-hyun
Copy link
Member

Nice. Thank you for fixing this from the upstream, @dilipbiswal !

@dilipbiswal
Copy link
Contributor Author

@dongjoon-hyun Thank you :-)

@dongjoon-hyun dongjoon-hyun changed the title [SPARK-27016][SQL][BUILD][SBT][FOLLOW-UP] Treat all antlr warnings as errors while generating the parser. [SPARK-27016][SQL][BUILD][FOLLOW-UP] Treat all antlr warnings as errors while generating the parser. Mar 11, 2019
Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM. (Pending Jenkins).
The first build and second build verified this PR.

@HyukjinKwon
Copy link
Member

+1 looks good

@SparkQA
Copy link

SparkQA commented Mar 12, 2019

Test build #103351 has finished for PR 24060 at commit f4c06b0.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@dongjoon-hyun
Copy link
Member

Thank you, @dilipbiswal and @HyukjinKwon .
Merged to master.

@dilipbiswal
Copy link
Contributor Author

Thanks a LOT @dongjoon-hyun @HyukjinKwon

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.

4 participants