-
Notifications
You must be signed in to change notification settings - Fork 931
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
Conversation
...nk-sql-engine/src/main/scala/org/apache/kyuubi/engine/flink/operation/ExecuteStatement.scala
Outdated
Show resolved
Hide resolved
...nk-sql-engine/src/main/scala/org/apache/kyuubi/engine/flink/operation/ExecuteStatement.scala
Outdated
Show resolved
Hide resolved
...nk-sql-engine/src/main/scala/org/apache/kyuubi/engine/flink/operation/ExecuteStatement.scala
Outdated
Show resolved
Hide resolved
Codecov Report
@@ 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! |
...nk-sql-engine/src/main/scala/org/apache/kyuubi/engine/flink/operation/ExecuteStatement.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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
There was a problem hiding this 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
...sql-engine/src/test/scala/org/apache/kyuubi/engine/flink/operation/FlinkOperationSuite.scala
Outdated
Show resolved
Hide resolved
@YesOrNo828 Merged. Thanks! |
Why are the changes needed?
resolve: #5505
Show available commands when users type 'HELP;' in the beeline.
Solution:
ExtendedParser
parse statement and return Operation of Flink engine.flink-table-planner-loader.jar
.Why not using Flink SQL Client Parser(SqlCommandParserImpl) to obtain the Command enum?
Flink 1.16's approach:
Flink 1.17 & 1.18
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?