[KYUUBI #3926] Introduce antlr4 to parse query statement by yikf · Pull Request #3944 · apache/kyuubi

yikf · 2022-12-08T10:46:20Z

Why are the changes needed?

Close #3926, we intend to introduce the parser module based on antlr4 in the Apache kyuubi server. Through this module, we can achieve:

Expand kyuubi capabilities, for example, implement SHOW KYUUBI_SESSIONS through SQL.
Sql translation for mysql front-end protocol.
Sql translation for trino front-end protocol.

This issue is the first step of the parser module, which introduces the parser module and implements SHOW KYUUBI_SESSION, and then improves the Apache kyuubi parser module based on this initial pr.

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

yikf · 2022-12-08T10:46:49Z

@pan3793 @ulysses-you Please take a look if you find a moment, thanks

pan3793 · 2022-12-08T11:08:05Z

Thanks @yikf, overall LGTM, will take a deep look this week. cc @cfmcgrady @yaooqinn @turboFei as well.

KYUUBI_SESSION would be a kind resource defined by KyuubiQL, I propose to add the following syntax in the future.

CREATE KYUUBI_SESSION ...
DROP KYUUBI_SESSION <session_id>
SHOW KYUUBI_SESSIONS [<conditions>]
ALTER KYUUBI_SESSION <session_id> SET ...

Any suggestions are welcome :)

yaooqinn · 2022-12-08T11:17:30Z

will take a deep look tomorrow

kyuubi-server/src/main/antlr4/org/apache/kyuubi/sql/KyuubiSqlBaseParser.g4

codecov-commenter · 2022-12-08T12:03:58Z

Codecov Report

Merging #3944 (eb98f1e) into master (eb98f1e) will not change coverage.
The diff coverage is n/a.

❗ Current head eb98f1e differs from pull request most recent head 4463b29. Consider uploading reports for the commit 4463b29 to get more accurate results

@@            Coverage Diff            @@
##             master    #3944   +/-   ##
=========================================
  Coverage     51.96%   51.96%           
  Complexity       13       13           
=========================================
  Files           522      522           
  Lines         29042    29042           
  Branches       3887     3887           
=========================================
  Hits          15093    15093           
  Misses        12575    12575           
  Partials       1374     1374

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

yaooqinn · 2022-12-09T03:01:10Z

do we need to design our syntaxes with the keyword KYUUBI as a prefix?

pan3793 · 2022-12-09T03:05:15Z

do we need to design our syntaxes with the keyword KYUUBI as a prefix?

I can image the most important resources in Kyuubi are SESSION and ENGINE, considering SESSION is quite a common thing, to avoid potential risk of syntax conflict, I suggest adding KYUUBI_ prefix

.gitignore

kyuubi-server/src/main/scala/org/apache/kyuubi/operation/ExecutedCommandExec.scala

yaooqinn · 2022-12-09T03:17:32Z

kyuubi-server/src/main/scala/org/apache/kyuubi/parser/plan/command/ShowSessions.scala

+ *   SHOW KYUUBI_SESSIONS;
+ * }}}
+ */
+case class ShowSessions() extends RunnableCommand {


why not use the thrift types directly like the spark SQL meta operations

it takes a while to remove type-mapping with spark ones, so I suggest we remove these and use thrift types directly

The Kyuubi parser module has its own SQL capability, so I think it would be better to have its own schema system. In addition, the newly added kyuubi runnable node can conveniently implement its own node's output schema instead of constructing thrift types, which is somewhat troublesome. we only need to convert the output once.

but you add them to the operation manager, which based on TRowSet

It seems not, I add these schema to parser module, Kyuubi operation ExecutedCommandExec call RunnableCommand.getNextRowSet, operation is not aware of the parser schema.

my idea is that the new runnableCommand can easily implement its own schema and data output. If each runnableCommand constructs thrift's schema and rowSet, I don't think it is convenient

You can create a PR(better KPIP) and dev discussion for adding a new type system, it's not a small work.

I'm truly sorry for the inconvenient about this request will cost, but it's necessary as an incomplete type system will result in a nightmare. See the timestamps/dates/intervals in hive/spark SQL type system for an example

Thanks kent, agree with you, i will move out the type system-related changes in this pr and the thrift type is used directly.

Any updates

Remove the newly introduced type system and use thrift's TTypeId

yaooqinn · 2022-12-09T03:23:57Z

do we need to design our syntaxes with the keyword KYUUBI as a prefix?

I can image the most important resources in Kyuubi are SESSION and ENGINE, considering SESSION is quite a common thing, to avoid potential risk of syntax conflict, I suggest adding KYUUBI_ prefix

yes, I agree with you. But I mean a SQL KEYWORD, like KYUUBI CMD/ KYUUBIADMIN CMD as a CMD prefix, not just a prefix of another KEYWORD, which leads us to add a lot of KEYWORDs in the form of KYUUBI_XXXX

pan3793 · 2022-12-09T03:31:36Z

... like KYUUBI CMD/ KYUUBIADMIN CMD as a CMD prefix ...

Emm, I remember in the early offline discussion, we prefer the ANSI-like syntax. But I'm fine w/ both way.

yaooqinn · 2022-12-09T03:52:07Z

... like KYUUBI CMD/ KYUUBIADMIN CMD as a CMD prefix ...

Emm, I remember in the early offline discussion, we prefer the ANSI-like syntax. But I'm fine w/ both way.

Let's list commands we may add as many as possible first?

kyuubi-server/src/main/scala/org/apache/kyuubi/parser/KyuubiParser.scala

kyuubi-server/src/main/scala/org/apache/kyuubi/parser/plan/command/RunnableCommand.scala

kyuubi-server/src/main/scala/org/apache/kyuubi/parser/KyuubiParser.scala

yaooqinn · 2022-12-09T07:46:19Z

kyuubi-server/src/main/scala/org/apache/kyuubi/parser/plan/command/ShowSessions.scala

+case class ShowSessions() extends RunnableCommand {
+
+  override def run(kyuubiSession: KyuubiSession): Unit = {
+    val rows = kyuubiSession.sessionManager.allSessions().map { session =>


So this cmd lists all sessions, is it safe to do such a thing from an end user? @pan3793

The REST api has the similar behavior

org.apache.kyuubi.server.api.v1.SessionsResource#sessions

The REST API is in experimental phase, not declared GA

List all sessions is an insecure operation, we need to have a mechanism to prevent this insecure situation from happening.

Either the person who does this operation is admin, or only list the sessions that the user has permission to.

It seems that there is no good mechanism now? This command can be set up as a experimental at this phase.

let's remove this command from this PR

A describe session command is safe to add I guess

agree with you, will add desc session command

jiaoqingbo · 2022-12-12T06:22:17Z

kyuubi-server/src/main/scala/org/apache/kyuubi/sql/schema/SchemaHelper.scala

+
+object SchemaHelper {
+
+  def toTTTableSchema(schema: List[Column]): TTableSchema = {


toTTableSchema?

Good catch, thanks

jiaoqingbo · 2022-12-12T06:27:54Z

kyuubi-server/src/test/scala/org/apache/kyuubi/parser/KyuubiParserSuite.scala

+    assert(node4.isInstanceOf[PassThroughNode])
+  }
+
+  test("Parse Show Kyuubi_Session Node") {


rename to Show Kyuubi_Sessions Node？

kyuubi-server/src/main/scala/org/apache/kyuubi/sql/plan/KyuubiNode.scala

pan3793 · 2022-12-12T13:24:24Z

kyuubi-server/src/main/scala/org/apache/kyuubi/sql/plan/command/ShowSessions.scala

+    Seq(
+      Column("user", TTypeId.STRING_TYPE, Some("Kyuubi session user")),
+      Column("type", TTypeId.STRING_TYPE, Some("Kyuubi session type")),
+      Column("ip", TTypeId.STRING_TYPE, Some("Kyuubi session remote ip")))


id, user, type

Does the id column indicate the session identify?

pan3793 · 2022-12-12T13:35:25Z

Summary: anyway, we need to introduce Antlr4 framework in server module, and the framework is already in a good shape, the key points here are the grammar and security. Basically, there are two type of styles: command and ANSI-like.

Command:

KYUUBI xxxx
KYUUBIADMIN xxxx

ANSI-like:

CREATE KYUUBI_SESSION xxx
ALTER KYUUBI_SESSION xxx SET xxx

each of those are extendable, and all of them lgtm.

Let's list commands we may add as many as possible first?

It's a good idea, we can do it in another issue, but I don't think it should block this PR.

For security concern about SHOW KYUUBI_SESSIONS, it's just a simplest command I can imagine to demonstrate how does the framework work, we can replace it by another simple command.

yikf · 2022-12-12T14:58:49Z

This PR should be blocked by syntax style, Just like the two styles that @pan3793 mentioned, both are fine to me.

But have a suggestion:

ANSI-like style is a fine syntax style, but it has the problem of duplicate prefixes in grammar files, and there is a certain chance of future grammar conflict problems;
KYUUBI/KYUUBIADMIN cmd style will definitely have no problem with grammar conflicts in the future, and no duplicate prefixes, so the syntax is more concise, but it is not an ansi syntax.

Considering that Kyuubi is not an sql engine and one of the purposes of the parser module we introduced was sql enhancement, I think ANSIstyle is probably not that important, so I prefer cmd style, any thought?

yaooqinn · 2022-12-13T08:00:13Z

kyuubi-server/src/main/antlr4/org/apache/kyuubi/sql/KyuubiSqlBaseLexer.g4

+ * limitations under the License.
+ */
+
+lexer grammar KyuubiSqlBaseLexer;


cc @ulysses-you, can you verify that this parser will work with trino FE?

it is not about trino for now, but we can support trino statement one by one by extending antlr later

how can we tell from a meta operation and regular one for select * from sys.tables ？

It won't affect other FE, that means other FE will pass though it. Only Trino session will match those special statements. I think user can not access the regular one if use Trino FE follows Trino's behavior.

yaooqinn · 2022-12-15T08:37:54Z

I doubt these can be accomplished by the current implementation which is bound to the BackendService

Sql translation for mysql front-end protocol.
Sql translation for trino front-end protocol.

yaooqinn

Anyway, it does not harm anything exists, I'm OK to repair things on a running car

pan3793 · 2022-12-15T09:06:22Z

Thanks all, merging to master

yikf · 2022-12-15T10:13:10Z

Thanks you all guys

pan3793 · 2023-01-13T08:09:56Z

When adding new deps into binary tgz, don't forget to update LICENSE-binary.

### _Why are the changes needed?_ This pr is followup of the #3944, it supplement LICENSE-binary ### _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](https://kyuubi.apache.org/docs/latest/develop_tools/testing.html#running-tests) locally before make a pull request Closes #4163 from Yikf/license. Closes #3926 92d907d [Yikf] [KYUUBI #3926][FOLLOWUP] Supplement LICENSE-binary Authored-by: Yikf <yikaifei1@gmail.com> Signed-off-by: Yikf <yikaifei@apache.org>

github-actions bot added kind:build kind:infra license, community building, project builds, asf infra related, etc. module:server labels Dec 8, 2022

cfmcgrady reviewed Dec 8, 2022

View reviewed changes

kyuubi-server/src/main/antlr4/org/apache/kyuubi/sql/KyuubiSqlBaseParser.g4 Outdated Show resolved Hide resolved