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

[KYUUBI #5505][FLINK] Support HELP command #5585

Closed
wants to merge 5 commits into from

Conversation

YesOrNo828
Copy link
Contributor

@YesOrNo828 YesOrNo828 commented Oct 31, 2023

Why are the changes needed?

resolve: #5505

Show available commands when users type 'HELP;' in the beeline.

Solution:

  • Using ExtendedParser parse statement and return Operation of Flink engine.
  • Check whether the operation is HelpOperation or not.
  • dependency on flink-table-planner-loader.jar.

Why not using Flink SQL Client Parser(SqlCommandParserImpl) to obtain the Command enum?

Flink 1.16's approach:

val opt:Optional[Operation] = org.apache.flink.table.client.cli.SqlCommandParserImpl.parseCommand()
check opt.get() instance of HelpOperation or not
if yes return CliStrings.MESSAGE_HELP

Flink 1.17 & 1.18

val opt: Optional[Command] = org.apache.flink.table.client.cli.parser.SqlCommandParserImpl.parseStatement()
check opt.get() is Command.HELP or not
if yes return CliStrings.MESSAGE_HELP

The Command class is added from Flink1.17;
The SqlCommandParserImpl package is changed, and the method name is changed from Flink1.17;

This approach requires distinguishing between different Flink versions and maintaining both implementations.
It's more complicated, so abandoned.

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

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

pom.xml Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Nov 1, 2023

Codecov Report

Merging #5585 (e73b15e) into master (591250c) will decrease coverage by 0.29%.
Report is 26 commits behind head on master.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master    #5585      +/-   ##
============================================
- Coverage     61.69%   61.40%   -0.29%     
  Complexity       23       23              
============================================
  Files           603      603              
  Lines         35670    35635      -35     
  Branches       4869     4874       +5     
============================================
- Hits          22005    21883     -122     
- Misses        11281    11370      +89     
+ Partials       2384     2382       -2     

see 43 files with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

Copy link
Member

@pan3793 pan3793 left a comment

Choose a reason for hiding this comment

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

LGTM, except to one minor comment

@pan3793 pan3793 requested a review from link3280 November 1, 2023 14:35
@cxzl25 cxzl25 changed the title [KYUUBI #5505] [FLINK] Support HELP command [KYUUBI #5505][FLINK] Support HELP command Nov 2, 2023
Copy link
Contributor

@link3280 link3280 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I left some comments please kindly take a look. @YesOrNo828

@link3280 link3280 added this to the v1.9.0 milestone Nov 7, 2023
@link3280 link3280 closed this in d4320e7 Nov 7, 2023
@link3280
Copy link
Contributor

link3280 commented Nov 7, 2023

@YesOrNo828 Merged. Thanks!

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.

[TASK][EASY] Introduce HELP command in Flink engine
4 participants