Skip to content

Conversation

@pan3793
Copy link
Member

@pan3793 pan3793 commented Sep 6, 2021

Why are the changes needed?

How was this patch tested?

  • Add some test cases that check the changes thoroughly including negative and positive cases if possible

  • Add screenshots for manual tests if appropriate

  • Run test locally before make a pull request

@pan3793 pan3793 self-assigned this Sep 6, 2021
@pan3793 pan3793 added this to the v1.4.0 milestone Sep 6, 2021
@pan3793 pan3793 linked an issue Sep 6, 2021 that may be closed by this pull request

<dependency>
<groupId>org.antlr</groupId>
<artifactId>antlr4-runtime</artifactId>
Copy link
Member Author

Choose a reason for hiding this comment

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

It's already pulled by spark-sql

Copy link
Contributor

Choose a reason for hiding this comment

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

we can also remove the antlr4-runtime and parent pom.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just make sure we don't miss anything here, Spark-3.1.2 depends antlr4-runtime:4.8-1, but kyuubi generates source code with antlr-maven-plugin:4.7. Is there any compatibility issue?

https://github.com/apache/spark/blob/de351e30a90dd988b133b3d00fa6218bfcaba8b8/pom.xml#L192

    <antlr4.version>4.8-1</antlr4.version>

https://github.com/apache/incubator-kyuubi/blob/4cd83790ae651ee0da92365458b08224d68a6419/pom.xml#L90

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, we should bump 4.8-1 to align with spark, 4.7 should work but will print warning messages if the antlr version is not exactly matched.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, we should bump antlr 4.8, see apache/spark#32603

Copy link
Contributor

Choose a reason for hiding this comment

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

make sense

<dependency>
<groupId>org.antlr</groupId>
<artifactId>antlr4-runtime</artifactId>
<scope>test</scope>
Copy link
Member Author

@pan3793 pan3793 Sep 6, 2021

Choose a reason for hiding this comment

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

it's likely an accident change in #990, reverted.

@pan3793 pan3793 requested a review from ulysses-you September 6, 2021 14:35
@codecov-commenter
Copy link

codecov-commenter commented Sep 6, 2021

Codecov Report

Merging #1041 (29d566c) into master (4cd8379) will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1041      +/-   ##
============================================
+ Coverage     78.40%   78.42%   +0.01%     
  Complexity       61       61              
============================================
  Files           167      167              
  Lines          6196     6196              
  Branches        723      723              
============================================
+ Hits           4858     4859       +1     
+ Misses          915      914       -1     
  Partials        423      423              
Impacted Files Coverage Δ
.../scala/org/apache/kyuubi/server/KyuubiServer.scala 58.69% <0.00%> (+2.17%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4cd8379...29d566c. Read the comment docs.

@ulysses-you
Copy link
Contributor

thanks, merging to master

@pan3793 pan3793 deleted the deps branch September 7, 2021 01:35
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.

[Umbrella] Add Z-Order extensions to optimize table with zorder

4 participants