[KYUUBI #4325] Support replace preparedStatement for Trino-jdbc#4417
Closed
yehere wants to merge 11 commits intoapache:masterfrom
Closed
[KYUUBI #4325] Support replace preparedStatement for Trino-jdbc#4417yehere wants to merge 11 commits intoapache:masterfrom
yehere wants to merge 11 commits intoapache:masterfrom
Conversation
Contributor
Author
|
@ulysses-you Is there a reusable trino g4 file for me to resolve parameters，like EXECUTE statement1 USING INTEGER '1','abc' ? |
Contributor
|
@yehere you can add in |
Contributor
|
please remove test/debug code when it's ready to review, thank you |
ulysses-you
reviewed
Feb 27, 2023
kyuubi-server/src/main/antlr4/org/apache/kyuubi/sql/KyuubiTrinoFeBaseParser.g4
Outdated
Show resolved
Hide resolved
ulysses-you
reviewed
Mar 8, 2023
...ts/kyuubi-trino-it/src/test/scala/org/apache/kyuubi/it/trino/server/TrinoFrontendSuite.scala
Show resolved
Hide resolved
ulysses-you
reviewed
Mar 8, 2023
kyuubi-common/src/test/scala/org/apache/kyuubi/operation/JDBCTestHelper.scala
Show resolved
Hide resolved
ulysses-you
reviewed
Mar 8, 2023
kyuubi-server/src/main/scala/org/apache/kyuubi/sql/plan/trino/TrinoFeOperations.scala
Outdated
Show resolved
Hide resolved
ulysses-you
reviewed
Mar 8, 2023
kyuubi-server/src/main/scala/org/apache/kyuubi/server/trino/api/TrinoContext.scala
Show resolved
Hide resolved
ulysses-you
reviewed
Mar 8, 2023
kyuubi-server/src/main/scala/org/apache/kyuubi/server/trino/api/TrinoContext.scala
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #4417 +/- ##
============================================
+ Coverage 53.59% 57.62% +4.03%
Complexity 13 13
============================================
Files 579 579
Lines 31802 31888 +86
Branches 4253 4262 +9
============================================
+ Hits 17045 18377 +1332
+ Misses 13172 11754 -1418
- Partials 1585 1757 +172
... and 35 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
ulysses-you
reviewed
Mar 8, 2023
...rk-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/operation/ExecuteStatement.scala
Outdated
Show resolved
Hide resolved
ulysses-you
reviewed
Mar 8, 2023
kyuubi-server/src/main/scala/org/apache/kyuubi/server/trino/api/Query.scala
Outdated
Show resolved
Hide resolved
ulysses-you
reviewed
Mar 8, 2023
| case GetPrepareSql(statementId, _) => | ||
| val query = Query( | ||
| statementId, | ||
| statement.split(s"$statementId FROM")(1), |
Contributor
There was a problem hiding this comment.
should it be statement of GetPrepareSql ?
Contributor
There was a problem hiding this comment.
I mean we can use prepare.sql, which is parsed by antlr
Contributor
Author
There was a problem hiding this comment.
This parse module I want my colleague to improve based on my code. He is responsible for trino
Contributor
|
also cc @iodone if you have time to help review |
ulysses-you
reviewed
Mar 20, 2023
kyuubi-server/src/main/scala/org/apache/kyuubi/server/trino/api/Query.scala
Outdated
Show resolved
Hide resolved
Contributor
|
thank you @yehere for the updating. There are still some comments not addressed. |
yehere
force-pushed
the
kyuubi-4325
branch
2 times, most recently
from
7614b11 to
1302cef
Compare
ulysses-you
reviewed
Mar 24, 2023
| arrow-format/11.0.0//arrow-format-11.0.0.jar | ||
| arrow-memory-core/11.0.0//arrow-memory-core-11.0.0.jar | ||
| arrow-memory-netty/11.0.0//arrow-memory-netty-11.0.0.jar | ||
| arrow-vector/11.0.0//arrow-vector-11.0.0.jar |
Contributor
There was a problem hiding this comment.
do we lose arrow dependency list before ? @pan3793
Member
There was a problem hiding this comment.
we don't need to do arrow codec in server before, and since new deps is added, please update the LICENSE and NOTICE
ulysses-you
reviewed
Mar 24, 2023
kyuubi-server/src/main/scala/org/apache/kyuubi/sql/plan/trino/TrinoFeOperations.scala
Show resolved
Hide resolved
ulysses-you
reviewed
Mar 24, 2023
yehere
force-pushed
the
kyuubi-4325
branch
2 times, most recently
from
7020e2b to
e5ff618
Compare
ulysses-you
reviewed
Mar 30, 2023
…,Support DEALLOCATE PREPARE
…,Add test case for DEALLOCATE PREPARE
…,Code optimization
…,Code optimization
…,update the dependency file, run './build/dependency.sh --replace'
ulysses-you
approved these changes
Mar 30, 2023
Contributor
ulysses-you
left a comment
ulysses-you
left a comment
There was a problem hiding this comment.
thanks, merging to master
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why are the changes needed?
close #4325
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