You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@kyuubi.apache.org by "yehere (via GitHub)" <gi...@apache.org> on 2023/02/26 09:14:29 UTC
[GitHub] [kyuubi] yehere opened a new pull request, #4417: [KYUUBI #4325] Support replace preparedStatement for Trino-jdbc
yehere opened a new pull request, #4417:
URL: https://github.com/apache/kyuubi/pull/4417
<!--
Thanks for sending a pull request!
Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://kyuubi.readthedocs.io/en/latest/community/CONTRIBUTING.html
2. If the PR is related to an issue in https://github.com/apache/kyuubi/issues, add '[KYUUBI #XXXX]' in your PR title, e.g., '[KYUUBI #XXXX] Your PR title ...'.
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][KYUUBI #XXXX] Your PR title ...'.
-->
### _Why are the changes needed?_
<!--
Please clarify why the changes are needed. For instance,
1. If you add a feature, you can talk about the use case of it.
2. If you fix a bug, you can clarify why it is a bug.
-->
close #4325
### _How was this patch tested?_
- [x] Add some test cases that check the changes thoroughly including negative and positive cases if possible
- [ ] Add screenshots for manual tests if appropriate
- [x] [Run test](https://kyuubi.readthedocs.io/en/master/develop_tools/testing.html#running-tests) locally before make a pull request
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] yehere commented on pull request #4417: [KYUUBI #4325] Support replace preparedStatement for Trino-jdbc
Posted by "yehere (via GitHub)" <gi...@apache.org>.
yehere commented on PR #4417:
URL: https://github.com/apache/kyuubi/pull/4417#issuecomment-1445306766
@ulysses-you Is there a reusable trino g4 file for me to resolve parameters，like EXECUTE statement1 USING INTEGER '1','abc' ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] ulysses-you commented on a diff in pull request #4417: [KYUUBI #4325] Support replace preparedStatement for Trino-jdbc
Posted by "ulysses-you (via GitHub)" <gi...@apache.org>.
ulysses-you commented on code in PR #4417:
URL: https://github.com/apache/kyuubi/pull/4417#discussion_r1128881825
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/trino/api/TrinoContext.scala:
##########
@@ -187,13 +187,20 @@ object TrinoContext {
queryHtmlUri: URI,
queryStatus: OperationStatus,
columns: Option[TGetResultSetMetadataResp] = None,
- data: Option[TRowSet] = None): QueryResults = {
+ data: Option[TRowSet] = None,
+ updateType: String = null): QueryResults = {
Review Comment:
`updateType: Option[String] = None`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] pan3793 commented on a diff in pull request #4417: [KYUUBI #4325] Support replace preparedStatement for Trino-jdbc
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #4417:
URL: https://github.com/apache/kyuubi/pull/4417#discussion_r1153204120
##########
dev/dependencyList:
##########
@@ -22,6 +22,10 @@ annotations/4.1.1.4//annotations-4.1.1.4.jar
antlr-runtime/3.5.3//antlr-runtime-3.5.3.jar
antlr4-runtime/4.9.3//antlr4-runtime-4.9.3.jar
aopalliance-repackaged/2.6.1//aopalliance-repackaged-2.6.1.jar
+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
Review Comment:
we don't need to do arrow codec in server before, and since new deps is added, please update the LICENSE and NOTICE
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] ulysses-you closed pull request #4417: [KYUUBI #4325] Support replace preparedStatement for Trino-jdbc
Posted by "ulysses-you (via GitHub)" <gi...@apache.org>.
ulysses-you closed pull request #4417: [KYUUBI #4325] Support replace preparedStatement for Trino-jdbc
URL: https://github.com/apache/kyuubi/pull/4417
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] ulysses-you commented on a diff in pull request #4417: [KYUUBI #4325] Support replace preparedStatement for Trino-jdbc
Posted by "ulysses-you (via GitHub)" <gi...@apache.org>.
ulysses-you commented on code in PR #4417:
URL: https://github.com/apache/kyuubi/pull/4417#discussion_r1123966697
##########
kyuubi-server/src/main/antlr4/org/apache/kyuubi/sql/KyuubiTrinoFeBaseParser.g4:
##########
@@ -54,9 +54,19 @@ statement
CAST LEFT_PAREN NULL AS SMALLINT RIGHT_PAREN KEY_SEQ COMMA
CAST LEFT_PAREN NULL AS VARCHAR RIGHT_PAREN PK_NAME
WHERE FALSE #getPrimaryKeys
+ | EXECUTE IDENTIFIER (USING parameterList)? #getPrepareParameters
+ | PREPARE IDENTIFIER FROM statement #getPrepareSql
Review Comment:
is that required ? if so can we reuse that from Trino ? introduce dependency or copy some code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] ulysses-you commented on a diff in pull request #4417: [KYUUBI #4325] Support replace preparedStatement for Trino-jdbc
Posted by "ulysses-you (via GitHub)" <gi...@apache.org>.
ulysses-you commented on code in PR #4417:
URL: https://github.com/apache/kyuubi/pull/4417#discussion_r1147146204
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/trino/api/v1/StatementResource.scala:
##########
@@ -73,9 +78,32 @@ private[v1] class StatementResource extends ApiRequestContext with Logging {
val trinoContext = TrinoContext(headers, remoteAddr)
try {
- val query = Query(statement, trinoContext, translator, fe.be)
- val qr = query.getQueryResults(query.getLastToken, uriInfo)
- TrinoContext.buildTrinoResponse(qr, query.context)
+ parser.parsePlan(statement) match {
+ case GetPrepareSql(statementId, _) =>
+ val query = Query(
+ statementId,
+ statement.split(s"$statementId FROM")(1),
Review Comment:
I mean we can use `prepare.sql`, which is parsed by antlr
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] ulysses-you commented on pull request #4417: [KYUUBI #4325] Support replace preparedStatement for Trino-jdbc
Posted by "ulysses-you (via GitHub)" <gi...@apache.org>.
ulysses-you commented on PR #4417:
URL: https://github.com/apache/kyuubi/pull/4417#issuecomment-1445604471
please remove test code when it's ready to review, thank you
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] yehere commented on a diff in pull request #4417: [KYUUBI #4325] Support replace preparedStatement for Trino-jdbc
Posted by "yehere (via GitHub)" <gi...@apache.org>.
yehere commented on code in PR #4417:
URL: https://github.com/apache/kyuubi/pull/4417#discussion_r1150537701
##########
kyuubi-hive-jdbc/src/main/java/org/apache/kyuubi/jdbc/hive/Utils.java:
##########
@@ -89,6 +90,62 @@ static void verifySuccess(TStatus status, boolean withInfo) throws SQLException
throw new KyuubiSQLException(status);
}
+ /**
+ * Splits the parametered sql statement at parameter boundaries.
+ *
+ * <p>taking into account ' and \ escaping.
+ *
+ * <p>output for: 'select 1 from ? where a = ?' ['select 1 from ',' where a = ','']
+ */
+ static List<String> splitSqlStatement(String sql) {
+ List<String> parts = new ArrayList<>();
+ int apCount = 0;
+ int off = 0;
+ boolean skip = false;
+
+ for (int i = 0; i < sql.length(); i++) {
+ char c = sql.charAt(i);
+ if (skip) {
+ skip = false;
+ continue;
+ }
+ switch (c) {
+ case '\'':
+ apCount++;
+ break;
+ case '\\':
+ skip = true;
+ break;
+ case '?':
+ if ((apCount & 1) == 0) {
+ parts.add(sql.substring(off, i));
+ off = i + 1;
+ }
+ break;
+ default:
+ break;
+ }
+ }
+ parts.add(sql.substring(off, sql.length()));
+ return parts;
+ }
+
+ /** update the SQL string with parameters set by setXXX methods of {@link PreparedStatement} */
+ public static String updateSql(final String sql, HashMap<Integer, String> parameters)
Review Comment:
This code is a copied from org.apache.kyuubi.jdbc.hive.KyuubiPreparedStatement#updateSql
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] ulysses-you commented on a diff in pull request #4417: [KYUUBI #4325] Support replace preparedStatement for Trino-jdbc
Posted by "ulysses-you (via GitHub)" <gi...@apache.org>.
ulysses-you commented on code in PR #4417:
URL: https://github.com/apache/kyuubi/pull/4417#discussion_r1128888205
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/trino/api/v1/StatementResource.scala:
##########
@@ -73,9 +78,32 @@ private[v1] class StatementResource extends ApiRequestContext with Logging {
val trinoContext = TrinoContext(headers, remoteAddr)
try {
- val query = Query(statement, trinoContext, translator, fe.be)
- val qr = query.getQueryResults(query.getLastToken, uriInfo)
- TrinoContext.buildTrinoResponse(qr, query.context)
+ parser.parsePlan(statement) match {
+ case GetPrepareSql(statementId, _) =>
+ val query = Query(
+ statementId,
+ statement.split(s"$statementId FROM")(1),
Review Comment:
should it be statement of GetPrepareSql ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] ulysses-you commented on a diff in pull request #4417: [KYUUBI #4325] Support replace preparedStatement for Trino-jdbc
Posted by "ulysses-you (via GitHub)" <gi...@apache.org>.
ulysses-you commented on code in PR #4417:
URL: https://github.com/apache/kyuubi/pull/4417#discussion_r1128885695
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/trino/api/Query.scala:
##########
@@ -152,6 +192,21 @@ object Query {
Query(QueryId(operationHandle), updatedContext, backendService)
}
+ def apply(
+ statementId: String,
+ statement: String,
+ context: TrinoContext,
+ backendService: BackendService,
+ aa: String): Query = {
Review Comment:
what does this new construct do ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] ulysses-you commented on pull request #4417: [KYUUBI #4325] Support replace preparedStatement for Trino-jdbc
Posted by "ulysses-you (via GitHub)" <gi...@apache.org>.
ulysses-you commented on PR #4417:
URL: https://github.com/apache/kyuubi/pull/4417#issuecomment-1459182287
also cc @iodone if you have time to help review
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] ulysses-you commented on a diff in pull request #4417: [KYUUBI #4325] Support replace preparedStatement for Trino-jdbc
Posted by "ulysses-you (via GitHub)" <gi...@apache.org>.
ulysses-you commented on code in PR #4417:
URL: https://github.com/apache/kyuubi/pull/4417#discussion_r1118231466
##########
kyuubi-server/src/main/antlr4/org/apache/kyuubi/sql/KyuubiTrinoFeBaseParser.g4:
##########
@@ -54,9 +54,19 @@ statement
CAST LEFT_PAREN NULL AS SMALLINT RIGHT_PAREN KEY_SEQ COMMA
CAST LEFT_PAREN NULL AS VARCHAR RIGHT_PAREN PK_NAME
WHERE FALSE #getPrimaryKeys
+ | EXECUTE IDENTIFIER (USING parameterList)? #getPrepareParameters
+ | PREPARE IDENTIFIER FROM statement #getPrepareSql
Review Comment:
let's follow the Trino style:
`getPrepareParameters` -> `execute`
`getPrepareSql` -> `prepare`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] ulysses-you commented on a diff in pull request #4417: [KYUUBI #4325] Support replace preparedStatement for Trino-jdbc
Posted by "ulysses-you (via GitHub)" <gi...@apache.org>.
ulysses-you commented on code in PR #4417:
URL: https://github.com/apache/kyuubi/pull/4417#discussion_r1147154093
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/sql/plan/trino/TrinoFeOperations.scala:
##########
@@ -59,3 +59,18 @@ case class GetColumns(
case class GetPrimaryKeys() extends KyuubiTreeNode {
override def name(): String = "Get Primary Keys"
}
+
+case class ExecuteForPreparing(statementId: String, parameters: List[String])
+ extends KyuubiTreeNode {
+ override def name(): String = "Execute For Preparing"
+}
+
+case class Prepare(statementId: String, sql: String)
+ extends KyuubiTreeNode {
+ override def name(): String = "Prepare Sql"
+}
+
+case class Deallocate(statementId: String)
Review Comment:
can we add test for this node ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] yehere commented on a diff in pull request #4417: [KYUUBI #4325] Support replace preparedStatement for Trino-jdbc
Posted by "yehere (via GitHub)" <gi...@apache.org>.
yehere commented on code in PR #4417:
URL: https://github.com/apache/kyuubi/pull/4417#discussion_r1151611461
##########
kyuubi-hive-jdbc/src/main/java/org/apache/kyuubi/jdbc/hive/Utils.java:
##########
@@ -89,6 +90,62 @@ static void verifySuccess(TStatus status, boolean withInfo) throws SQLException
throw new KyuubiSQLException(status);
}
+ /**
+ * Splits the parametered sql statement at parameter boundaries.
+ *
+ * <p>taking into account ' and \ escaping.
+ *
+ * <p>output for: 'select 1 from ? where a = ?' ['select 1 from ',' where a = ','']
+ */
+ static List<String> splitSqlStatement(String sql) {
+ List<String> parts = new ArrayList<>();
+ int apCount = 0;
+ int off = 0;
+ boolean skip = false;
+
+ for (int i = 0; i < sql.length(); i++) {
+ char c = sql.charAt(i);
+ if (skip) {
+ skip = false;
+ continue;
+ }
+ switch (c) {
+ case '\'':
+ apCount++;
+ break;
+ case '\\':
+ skip = true;
+ break;
+ case '?':
+ if ((apCount & 1) == 0) {
+ parts.add(sql.substring(off, i));
+ off = i + 1;
+ }
+ break;
+ default:
+ break;
+ }
+ }
+ parts.add(sql.substring(off, sql.length()));
+ return parts;
+ }
+
+ /** update the SQL string with parameters set by setXXX methods of {@link PreparedStatement} */
+ public static String updateSql(final String sql, HashMap<Integer, String> parameters)
Review Comment:
sure，I think so too
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] ulysses-you commented on a diff in pull request #4417: [KYUUBI #4325] Support replace preparedStatement for Trino-jdbc
Posted by "ulysses-you (via GitHub)" <gi...@apache.org>.
ulysses-you commented on code in PR #4417:
URL: https://github.com/apache/kyuubi/pull/4417#discussion_r1152698017
##########
kyuubi-server/pom.xml:
##########
@@ -251,6 +257,11 @@
<artifactId>trino-client</artifactId>
</dependency>
+ <dependency>
+ <groupId>io.trino</groupId>
+ <artifactId>trino-jdbc</artifactId>
+ </dependency>
Review Comment:
do we need import `trino-jdbc` ? I see the dependency list changed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] ulysses-you commented on a diff in pull request #4417: [KYUUBI #4325] Support replace preparedStatement for Trino-jdbc
Posted by "ulysses-you (via GitHub)" <gi...@apache.org>.
ulysses-you commented on code in PR #4417:
URL: https://github.com/apache/kyuubi/pull/4417#discussion_r1128872713
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/sql/plan/trino/TrinoFeOperations.scala:
##########
@@ -59,3 +59,13 @@ case class GetColumns(
case class GetPrimaryKeys() extends KyuubiTreeNode {
override def name(): String = "Get Primary Keys"
}
+
+case class GetPrepareParameters(statementId: String, parameters: List[String])
+ extends KyuubiTreeNode {
+ override def name(): String = "Get Prepare Parameters"
+}
+
+case class GetPrepareSql(statementId: String, sql: String)
Review Comment:
this class name should be updated, i.e., Prepare and ExecuteForPreparing
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] yehere commented on a diff in pull request #4417: [KYUUBI #4325] Support replace preparedStatement for Trino-jdbc
Posted by "yehere (via GitHub)" <gi...@apache.org>.
yehere commented on code in PR #4417:
URL: https://github.com/apache/kyuubi/pull/4417#discussion_r1150532628
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/sql/plan/trino/TrinoFeOperations.scala:
##########
@@ -59,3 +59,18 @@ case class GetColumns(
case class GetPrimaryKeys() extends KyuubiTreeNode {
override def name(): String = "Get Primary Keys"
}
+
+case class ExecuteForPreparing(statementId: String, parameters: List[String])
+ extends KyuubiTreeNode {
+ override def name(): String = "Execute For Preparing"
+}
+
+case class Prepare(statementId: String, sql: String)
+ extends KyuubiTreeNode {
+ override def name(): String = "Prepare Sql"
+}
+
+case class Deallocate(statementId: String)
Review Comment:
Of course
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] ulysses-you commented on a diff in pull request #4417: [KYUUBI #4325] Support replace preparedStatement for Trino-jdbc
Posted by "ulysses-you (via GitHub)" <gi...@apache.org>.
ulysses-you commented on code in PR #4417:
URL: https://github.com/apache/kyuubi/pull/4417#discussion_r1141899541
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/trino/api/Query.scala:
##########
@@ -152,6 +192,20 @@ object Query {
Query(QueryId(operationHandle), updatedContext, backendService)
}
+ def apply(
+ statementId: String,
+ statement: String,
+ context: TrinoContext,
+ backendService: BackendService): Query = {
+ val sessionHandle = getOrCreateSession(context, backendService)
+ val sessionWithId =
+ context.session + (KYUUBI_SESSION_ID -> sessionHandle.identifier.toString)
+ Query(
+ queryId = QueryId("9231360e-8673-477e-8f00-63167efe1744"),
Review Comment:
why does this query id hard code ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] yehere commented on a diff in pull request #4417: [KYUUBI #4325] Support replace preparedStatement for Trino-jdbc
Posted by "yehere (via GitHub)" <gi...@apache.org>.
yehere commented on code in PR #4417:
URL: https://github.com/apache/kyuubi/pull/4417#discussion_r1145949911
##########
kyuubi-common/src/test/scala/org/apache/kyuubi/operation/JDBCTestHelper.scala:
##########
@@ -97,4 +122,10 @@ trait JDBCTestHelper extends KyuubiFunSuite {
def withJdbcStatement(tableNames: String*)(f: Statement => Unit): Unit = {
withMultipleConnectionJdbcStatement(tableNames: _*)(f)
}
+
+ def withJdbcPrepareStatement(
Review Comment:
it has been used for test, to cover 'DEALLOCATE PREPARE' syntax support
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] ulysses-you commented on a diff in pull request #4417: [KYUUBI #4325] Support replace preparedStatement for Trino-jdbc
Posted by "ulysses-you (via GitHub)" <gi...@apache.org>.
ulysses-you commented on code in PR #4417:
URL: https://github.com/apache/kyuubi/pull/4417#discussion_r1147154674
##########
kyuubi-hive-jdbc/src/main/java/org/apache/kyuubi/jdbc/hive/Utils.java:
##########
@@ -89,6 +90,62 @@ static void verifySuccess(TStatus status, boolean withInfo) throws SQLException
throw new KyuubiSQLException(status);
}
+ /**
+ * Splits the parametered sql statement at parameter boundaries.
+ *
+ * <p>taking into account ' and \ escaping.
+ *
+ * <p>output for: 'select 1 from ? where a = ?' ['select 1 from ',' where a = ','']
+ */
+ static List<String> splitSqlStatement(String sql) {
+ List<String> parts = new ArrayList<>();
+ int apCount = 0;
+ int off = 0;
+ boolean skip = false;
+
+ for (int i = 0; i < sql.length(); i++) {
+ char c = sql.charAt(i);
+ if (skip) {
+ skip = false;
+ continue;
+ }
+ switch (c) {
+ case '\'':
+ apCount++;
+ break;
+ case '\\':
+ skip = true;
+ break;
+ case '?':
+ if ((apCount & 1) == 0) {
+ parts.add(sql.substring(off, i));
+ off = i + 1;
+ }
+ break;
+ default:
+ break;
+ }
+ }
+ parts.add(sql.substring(off, sql.length()));
+ return parts;
+ }
+
+ /** update the SQL string with parameters set by setXXX methods of {@link PreparedStatement} */
+ public static String updateSql(final String sql, HashMap<Integer, String> parameters)
Review Comment:
it seems hack to me. How does Trino update parameters ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] ulysses-you commented on a diff in pull request #4417: [KYUUBI #4325] Support replace preparedStatement for Trino-jdbc
Posted by "ulysses-you (via GitHub)" <gi...@apache.org>.
ulysses-you commented on code in PR #4417:
URL: https://github.com/apache/kyuubi/pull/4417#discussion_r1147146204
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/trino/api/v1/StatementResource.scala:
##########
@@ -73,9 +78,32 @@ private[v1] class StatementResource extends ApiRequestContext with Logging {
val trinoContext = TrinoContext(headers, remoteAddr)
try {
- val query = Query(statement, trinoContext, translator, fe.be)
- val qr = query.getQueryResults(query.getLastToken, uriInfo)
- TrinoContext.buildTrinoResponse(qr, query.context)
+ parser.parsePlan(statement) match {
+ case GetPrepareSql(statementId, _) =>
+ val query = Query(
+ statementId,
+ statement.split(s"$statementId FROM")(1),
Review Comment:
I mean we can use `prepare.sql`, which is parsed by anltr
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] yehere commented on a diff in pull request #4417: [KYUUBI #4325] Support replace preparedStatement for Trino-jdbc
Posted by "yehere (via GitHub)" <gi...@apache.org>.
yehere commented on code in PR #4417:
URL: https://github.com/apache/kyuubi/pull/4417#discussion_r1128892298
##########
integration-tests/kyuubi-trino-it/src/test/scala/org/apache/kyuubi/it/trino/server/TrinoFrontendSuite.scala:
##########
@@ -46,16 +67,8 @@ class TrinoFrontendSuite extends WithKyuubiServer with SparkMetadataTests {
override protected val password: String = ""
override def beforeAll(): Unit = {
- super.beforeAll()
-
// eagerly start spark engine before running test, it's a workaround for trino jdbc driver
// since it does not support changing http connect timeout
- try {
- withJdbcStatement() { statement =>
- statement.execute("SELECT 1")
- }
- } catch {
- case NonFatal(e) =>
- }
Review Comment:
ok，I thought it was for init testing .. it can skip the timeout exception
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] ulysses-you commented on a diff in pull request #4417: [KYUUBI #4325] Support replace preparedStatement for Trino-jdbc
Posted by "ulysses-you (via GitHub)" <gi...@apache.org>.
ulysses-you commented on code in PR #4417:
URL: https://github.com/apache/kyuubi/pull/4417#discussion_r1128870315
##########
kyuubi-common/src/test/scala/org/apache/kyuubi/operation/JDBCTestHelper.scala:
##########
@@ -97,4 +122,10 @@ trait JDBCTestHelper extends KyuubiFunSuite {
def withJdbcStatement(tableNames: String*)(f: Statement => Unit): Unit = {
withMultipleConnectionJdbcStatement(tableNames: _*)(f)
}
+
+ def withJdbcPrepareStatement(
Review Comment:
where does it be used ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] ulysses-you commented on pull request #4417: [KYUUBI #4325] Support replace preparedStatement for Trino-jdbc
Posted by "ulysses-you (via GitHub)" <gi...@apache.org>.
ulysses-you commented on PR #4417:
URL: https://github.com/apache/kyuubi/pull/4417#issuecomment-1475968848
thank you @yehere for the updating. There are still some comments not addressed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] yehere commented on a diff in pull request #4417: [KYUUBI #4325] Support replace preparedStatement for Trino-jdbc
Posted by "yehere (via GitHub)" <gi...@apache.org>.
yehere commented on code in PR #4417:
URL: https://github.com/apache/kyuubi/pull/4417#discussion_r1150531023
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/trino/api/v1/StatementResource.scala:
##########
@@ -73,9 +78,32 @@ private[v1] class StatementResource extends ApiRequestContext with Logging {
val trinoContext = TrinoContext(headers, remoteAddr)
try {
- val query = Query(statement, trinoContext, translator, fe.be)
- val qr = query.getQueryResults(query.getLastToken, uriInfo)
- TrinoContext.buildTrinoResponse(qr, query.context)
+ parser.parsePlan(statement) match {
+ case GetPrepareSql(statementId, _) =>
+ val query = Query(
+ statementId,
+ statement.split(s"$statementId FROM")(1),
Review Comment:
This parse module I want my colleague to improve based on my code. He is responsible for trino
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] yehere commented on a diff in pull request #4417: [KYUUBI #4325] Support replace preparedStatement for Trino-jdbc
Posted by "yehere (via GitHub)" <gi...@apache.org>.
yehere commented on code in PR #4417:
URL: https://github.com/apache/kyuubi/pull/4417#discussion_r1152814608
##########
kyuubi-server/pom.xml:
##########
@@ -251,6 +257,11 @@
<artifactId>trino-client</artifactId>
</dependency>
+ <dependency>
+ <groupId>io.trino</groupId>
+ <artifactId>trino-jdbc</artifactId>
+ </dependency>
Review Comment:
jdbc tests were written on the server before，them were moved to integration-test and i will remove it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] ulysses-you commented on pull request #4417: [KYUUBI #4325] Support replace preparedStatement for Trino-jdbc
Posted by "ulysses-you (via GitHub)" <gi...@apache.org>.
ulysses-you commented on PR #4417:
URL: https://github.com/apache/kyuubi/pull/4417#issuecomment-1445603923
@yehere you can add in `KyuubiTrinoFeBaseParser.g4`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] yehere commented on a diff in pull request #4417: [KYUUBI #4325] Support replace preparedStatement for Trino-jdbc
Posted by "yehere (via GitHub)" <gi...@apache.org>.
yehere commented on code in PR #4417:
URL: https://github.com/apache/kyuubi/pull/4417#discussion_r1123018419
##########
kyuubi-server/src/main/antlr4/org/apache/kyuubi/sql/KyuubiTrinoFeBaseParser.g4:
##########
@@ -54,9 +54,19 @@ statement
CAST LEFT_PAREN NULL AS SMALLINT RIGHT_PAREN KEY_SEQ COMMA
CAST LEFT_PAREN NULL AS VARCHAR RIGHT_PAREN PK_NAME
WHERE FALSE #getPrimaryKeys
+ | EXECUTE IDENTIFIER (USING parameterList)? #getPrepareParameters
+ | PREPARE IDENTIFIER FROM statement #getPrepareSql
Review Comment:
ok，I misunderstood the api named mapping relationship. It should correspond to the engine's api
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] codecov-commenter commented on pull request #4417: [KYUUBI #4325] Support replace preparedStatement for Trino-jdbc
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #4417:
URL: https://github.com/apache/kyuubi/pull/4417#issuecomment-1459170541
# [Codecov](https://codecov.io/gh/apache/kyuubi/pull/4417?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#4417](https://codecov.io/gh/apache/kyuubi/pull/4417?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9f09f72) into [master](https://codecov.io/gh/apache/kyuubi/commit/222a3345f2d615df3c32270c5a546db0f5f4a23c?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (222a334) will **increase** coverage by `3.97%`.
> The diff coverage is `11.00%`.
> :exclamation: Current head 9f09f72 differs from pull request most recent head f1349ba. Consider uploading reports for the commit f1349ba to get more accurate results
```diff
@@ Coverage Diff @@
## master #4417 +/- ##
============================================
+ Coverage 53.32% 57.29% +3.97%
Complexity 13 13
============================================
Files 571 571
Lines 31249 31353 +104
Branches 4214 4227 +13
============================================
+ Hits 16663 17964 +1301
+ Misses 13011 11632 -1379
- Partials 1575 1757 +182
```
| [Impacted Files](https://codecov.io/gh/apache/kyuubi/pull/4417?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...c/main/java/org/apache/kyuubi/jdbc/hive/Utils.java](https://codecov.io/gh/apache/kyuubi/pull/4417?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWhpdmUtamRiYy9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUva3l1dWJpL2pkYmMvaGl2ZS9VdGlscy5qYXZh) | `58.11% <0.00%> (+17.05%)` | :arrow_up: |
| [...ala/org/apache/kyuubi/server/trino/api/Query.scala](https://codecov.io/gh/apache/kyuubi/pull/4417?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2ZXIvdHJpbm8vYXBpL1F1ZXJ5LnNjYWxh) | `62.58% <0.00%> (-21.06%)` | :arrow_down: |
| [.../kyuubi/sql/parser/trino/KyuubiTrinoFeParser.scala](https://codecov.io/gh/apache/kyuubi/pull/4417?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zcWwvcGFyc2VyL3RyaW5vL0t5dXViaVRyaW5vRmVQYXJzZXIuc2NhbGE=) | `75.00% <ø> (ø)` | |
| [...ache/kyuubi/sql/plan/trino/TrinoFeOperations.scala](https://codecov.io/gh/apache/kyuubi/pull/4417?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zcWwvcGxhbi90cmluby9Ucmlub0ZlT3BlcmF0aW9ucy5zY2FsYQ==) | `34.48% <16.66%> (-4.65%)` | :arrow_down: |
| [...kyuubi/server/trino/api/v1/StatementResource.scala](https://codecov.io/gh/apache/kyuubi/pull/4417?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2ZXIvdHJpbm8vYXBpL3YxL1N0YXRlbWVudFJlc291cmNlLnNjYWxh) | `41.05% <17.39%> (-9.62%)` | :arrow_down: |
| [.../apache/kyuubi/server/trino/api/TrinoContext.scala](https://codecov.io/gh/apache/kyuubi/pull/4417?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2ZXIvdHJpbm8vYXBpL1RyaW5vQ29udGV4dC5zY2FsYQ==) | `52.92% <33.33%> (-0.96%)` | :arrow_down: |
| [...ubi/sql/parser/trino/KyuubiTrinoFeAstBuilder.scala](https://codecov.io/gh/apache/kyuubi/pull/4417?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zcWwvcGFyc2VyL3RyaW5vL0t5dXViaVRyaW5vRmVBc3RCdWlsZGVyLnNjYWxh) | `93.02% <50.00%> (-6.98%)` | :arrow_down: |
| [...uubi/engine/spark/operation/ExecuteStatement.scala](https://codecov.io/gh/apache/kyuubi/pull/4417?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9vcGVyYXRpb24vRXhlY3V0ZVN0YXRlbWVudC5zY2FsYQ==) | `81.00% <100.00%> (+0.19%)` | :arrow_up: |
| [...client/exception/RetryableKyuubiRestException.java](https://codecov.io/gh/apache/kyuubi/pull/4417?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXJlc3QtY2xpZW50L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9reXV1YmkvY2xpZW50L2V4Y2VwdGlvbi9SZXRyeWFibGVLeXV1YmlSZXN0RXhjZXB0aW9uLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../org/apache/kyuubi/client/RetryableRestClient.java](https://codecov.io/gh/apache/kyuubi/pull/4417?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXJlc3QtY2xpZW50L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9reXV1YmkvY2xpZW50L1JldHJ5YWJsZVJlc3RDbGllbnQuamF2YQ==) | `48.78% <0.00%> (-24.40%)` | :arrow_down: |
| ... and [39 more](https://codecov.io/gh/apache/kyuubi/pull/4417?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] ulysses-you commented on a diff in pull request #4417: [KYUUBI #4325] Support replace preparedStatement for Trino-jdbc
Posted by "ulysses-you (via GitHub)" <gi...@apache.org>.
ulysses-you commented on code in PR #4417:
URL: https://github.com/apache/kyuubi/pull/4417#discussion_r1128885110
##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/operation/ExecuteStatement.scala:
##########
@@ -83,6 +83,7 @@ class ExecuteStatement(
info(diagnostics)
Thread.currentThread().setContextClassLoader(spark.sharedState.jarClassLoader)
addOperationListener()
+ info(statement)
Review Comment:
please remove this logging
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] ulysses-you commented on a diff in pull request #4417: [KYUUBI #4325] Support replace preparedStatement for Trino-jdbc
Posted by "ulysses-you (via GitHub)" <gi...@apache.org>.
ulysses-you commented on code in PR #4417:
URL: https://github.com/apache/kyuubi/pull/4417#discussion_r1151562637
##########
kyuubi-hive-jdbc/src/main/java/org/apache/kyuubi/jdbc/hive/Utils.java:
##########
@@ -89,6 +90,62 @@ static void verifySuccess(TStatus status, boolean withInfo) throws SQLException
throw new KyuubiSQLException(status);
}
+ /**
+ * Splits the parametered sql statement at parameter boundaries.
+ *
+ * <p>taking into account ' and \ escaping.
+ *
+ * <p>output for: 'select 1 from ? where a = ?' ['select 1 from ',' where a = ','']
+ */
+ static List<String> splitSqlStatement(String sql) {
+ List<String> parts = new ArrayList<>();
+ int apCount = 0;
+ int off = 0;
+ boolean skip = false;
+
+ for (int i = 0; i < sql.length(); i++) {
+ char c = sql.charAt(i);
+ if (skip) {
+ skip = false;
+ continue;
+ }
+ switch (c) {
+ case '\'':
+ apCount++;
+ break;
+ case '\\':
+ skip = true;
+ break;
+ case '?':
+ if ((apCount & 1) == 0) {
+ parts.add(sql.substring(off, i));
+ off = i + 1;
+ }
+ break;
+ default:
+ break;
+ }
+ }
+ parts.add(sql.substring(off, sql.length()));
+ return parts;
+ }
+
+ /** update the SQL string with parameters set by setXXX methods of {@link PreparedStatement} */
+ public static String updateSql(final String sql, HashMap<Integer, String> parameters)
Review Comment:
oh I see, shall we unifiy them ? can we make `org.apache.kyuubi.jdbc.hive.KyuubiPreparedStatement` use the updateSql from `Utils`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] ulysses-you commented on a diff in pull request #4417: [KYUUBI #4325] Support replace preparedStatement for Trino-jdbc
Posted by "ulysses-you (via GitHub)" <gi...@apache.org>.
ulysses-you commented on code in PR #4417:
URL: https://github.com/apache/kyuubi/pull/4417#discussion_r1147146646
##########
dev/dependencyList:
##########
@@ -22,6 +22,10 @@ annotations/4.1.1.4//annotations-4.1.1.4.jar
antlr-runtime/3.5.3//antlr-runtime-3.5.3.jar
antlr4-runtime/4.9.3//antlr4-runtime-4.9.3.jar
aopalliance-repackaged/2.6.1//aopalliance-repackaged-2.6.1.jar
+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
Review Comment:
do we lose arrow dependency list before ? @pan3793
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] ulysses-you commented on a diff in pull request #4417: [KYUUBI #4325] Support replace preparedStatement for Trino-jdbc
Posted by "ulysses-you (via GitHub)" <gi...@apache.org>.
ulysses-you commented on code in PR #4417:
URL: https://github.com/apache/kyuubi/pull/4417#discussion_r1128881611
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/trino/api/TrinoContext.scala:
##########
@@ -187,13 +187,20 @@ object TrinoContext {
queryHtmlUri: URI,
queryStatus: OperationStatus,
columns: Option[TGetResultSetMetadataResp] = None,
- data: Option[TRowSet] = None): QueryResults = {
+ data: Option[TRowSet] = None,
+ updateType: String = null): QueryResults = {
val columnList = columns match {
case Some(value) => convertTColumn(value)
case None => null
}
val rowList = data match {
+ case Some(value) if "PREPARE".equals(updateType) =>
+ val list1 = new util.LinkedList[util.List[Object]]
+ val list2 = new util.LinkedList[Object]()
+ list2.add(true.asInstanceOf[Object])
+ list1.add(list2)
+ list1
case Some(value) => convertTRowSet(value)
case None => null
}
Review Comment:
```
updateType match {
case Some("PREPARE") => ImmutableList.of(ImmutableList.of(true))
case _ => convertTRowSet(value)
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] ulysses-you commented on a diff in pull request #4417: [KYUUBI #4325] Support replace preparedStatement for Trino-jdbc
Posted by "ulysses-you (via GitHub)" <gi...@apache.org>.
ulysses-you commented on code in PR #4417:
URL: https://github.com/apache/kyuubi/pull/4417#discussion_r1128868812
##########
integration-tests/kyuubi-trino-it/src/test/scala/org/apache/kyuubi/it/trino/server/TrinoFrontendSuite.scala:
##########
@@ -46,16 +67,8 @@ class TrinoFrontendSuite extends WithKyuubiServer with SparkMetadataTests {
override protected val password: String = ""
override def beforeAll(): Unit = {
- super.beforeAll()
-
// eagerly start spark engine before running test, it's a workaround for trino jdbc driver
// since it does not support changing http connect timeout
- try {
- withJdbcStatement() { statement =>
- statement.execute("SELECT 1")
- }
- } catch {
- case NonFatal(e) =>
- }
Review Comment:
please do not remove this workaround, see the comment ..
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] ulysses-you commented on a diff in pull request #4417: [KYUUBI #4325] Support replace preparedStatement for Trino-jdbc
Posted by "ulysses-you (via GitHub)" <gi...@apache.org>.
ulysses-you commented on code in PR #4417:
URL: https://github.com/apache/kyuubi/pull/4417#discussion_r1128877168
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/trino/api/TrinoContext.scala:
##########
@@ -187,13 +187,20 @@ object TrinoContext {
queryHtmlUri: URI,
queryStatus: OperationStatus,
columns: Option[TGetResultSetMetadataResp] = None,
- data: Option[TRowSet] = None): QueryResults = {
+ data: Option[TRowSet] = None,
+ updateType: String = null): QueryResults = {
Review Comment:
how about pass a functon how to convert row se to list ? `convertRowListFunc: => List[List[Object]]`
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/trino/api/TrinoContext.scala:
##########
@@ -187,13 +187,20 @@ object TrinoContext {
queryHtmlUri: URI,
queryStatus: OperationStatus,
columns: Option[TGetResultSetMetadataResp] = None,
- data: Option[TRowSet] = None): QueryResults = {
+ data: Option[TRowSet] = None,
+ updateType: String = null): QueryResults = {
Review Comment:
how about pass a functon how to convert row set to list ? `convertRowListFunc: => List[List[Object]]`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] yehere commented on a diff in pull request #4417: [KYUUBI #4325] Support replace preparedStatement for Trino-jdbc
Posted by "yehere (via GitHub)" <gi...@apache.org>.
yehere commented on code in PR #4417:
URL: https://github.com/apache/kyuubi/pull/4417#discussion_r1123025769
##########
kyuubi-server/src/main/antlr4/org/apache/kyuubi/sql/KyuubiTrinoFeBaseParser.g4:
##########
@@ -54,9 +54,19 @@ statement
CAST LEFT_PAREN NULL AS SMALLINT RIGHT_PAREN KEY_SEQ COMMA
CAST LEFT_PAREN NULL AS VARCHAR RIGHT_PAREN PK_NAME
WHERE FALSE #getPrimaryKeys
+ | EXECUTE IDENTIFIER (USING parameterList)? #getPrepareParameters
+ | PREPARE IDENTIFIER FROM statement #getPrepareSql
Review Comment:
> please remove test/debug code when it's ready to review, thank you
Thanks for your reminder and I will close it this weekend . Now this g4 file cannot match all cases， are there any matching rules that can be reused to parse trino's execute-grammar
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] yehere commented on a diff in pull request #4417: [KYUUBI #4325] Support replace preparedStatement for Trino-jdbc
Posted by "yehere (via GitHub)" <gi...@apache.org>.
yehere commented on code in PR #4417:
URL: https://github.com/apache/kyuubi/pull/4417#discussion_r1145854906
##########
kyuubi-server/src/main/scala/org/apache/kyuubi/server/trino/api/TrinoContext.scala:
##########
@@ -187,13 +187,20 @@ object TrinoContext {
queryHtmlUri: URI,
queryStatus: OperationStatus,
columns: Option[TGetResultSetMetadataResp] = None,
- data: Option[TRowSet] = None): QueryResults = {
+ data: Option[TRowSet] = None,
+ updateType: String = null): QueryResults = {
Review Comment:
It was packaged with option when used
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org