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.

Sign up for GitHub

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 apache#4320] Support GetPrimaryKeys for Trino Fe #12

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

[KYUUBI apache#4320] Support GetPrimaryKeys for Trino Fe #12

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b27c140
Fix the wrong parsing of jvm parameters in jdbc URL
yanghua Apr 14, 2023
26aa92a
[KYUUBI #4320] Support GetPrimaryKeys for Trino Fe
yikf Feb 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions kyuubi-hive-jdbc/src/main/java/org/apache/kyuubi/jdbc/hive/Utils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import java.util.*;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import org.apache.commons.lang3.StringUtils;
import org.apache.hive.service.rpc.thrift.TStatus;
import org.apache.hive.service.rpc.thrift.TStatusCode;
import org.slf4j.Logger;
Expand Down Expand Up @@ -193,12 +194,20 @@ public static JdbcConnectionParams extractURLComponents(String uri, Properties i
}
}

Pattern confPattern = Pattern.compile("([^;]*)([^;]*);?");

// parse hive conf settings
String confStr = jdbcURI.getQuery();
if (confStr != null) {
Matcher confMatcher = pattern.matcher(confStr);
Matcher confMatcher = confPattern.matcher(confStr);
while (confMatcher.find()) {
connParams.getHiveConfs().put(confMatcher.group(1), confMatcher.group(2));
String connParam = confMatcher.group(1);
if (StringUtils.isNotBlank(connParam) && connParam.contains("=")) {
int symbolIndex = connParam.indexOf('=');
connParams
.getHiveConfs()
.put(connParam.substring(0, symbolIndex), connParam.substring(symbolIndex + 1));
}
}
}

Expand Down Expand Up @@ -477,4 +486,4 @@ public static String getCanonicalHostName(String hostName) {
public static boolean isKyuubiOperationHint(String hint) {
return KYUUBI_OPERATION_HINT_PATTERN.matcher(hint).matches();
}
}
}
Copy link

Choose a reason for hiding this comment

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

这个代码补丁主要引入了一个新的依赖 - org.apache.commons.lang3.StringUtils，并对一些变量和方法进行了修改。

在方法 extractURLComponents 中，现在使用的是 Pattern 类编译的正则表达式，其中 confPattern 对象的定义被移动到了方法内部，并修改了 while 循环将结果存储到 Map 对象中。同时，还增加了一些新的检查，以避免错误或意外的输入数据。

在 isKyuubiOperationHint 方法中，现在使用的是一个匹配器对象，对 KYUUBI_OPERATION_HINT_PATTERN 进行了预编译，以提高性能。此外，该方法没有任何更改。

put 语句中的 Map 对象如果接收 null 值会导致 NullPointerException 错误。建议在向 Map 中添加之前创建它，每次都判断一下异常情况。

总的来说，这个补丁采取了良好的编码风格和技术实践。虽然可能仍存在一些潜在的错误或忽略的边界情况，但这是必要的优化和重构。

77 changes: 69 additions & 8 deletions kyuubi-hive-jdbc/src/test/java/org/apache/kyuubi/jdbc/hive/UtilsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,13 @@
import static org.apache.kyuubi.jdbc.hive.Utils.extractURLComponents;
import static org.junit.Assert.assertEquals;

import com.google.common.collect.ImmutableMap;
import java.io.UnsupportedEncodingException;
import java.net.URLEncoder;
import java.nio.charset.StandardCharsets;
import java.util.Arrays;
import java.util.Collection;
import java.util.Map;
import java.util.Properties;
import org.junit.Test;
import org.junit.runner.RunWith;
Expand All @@ -35,23 +40,76 @@ public class UtilsTest {
private String expectedPort;
private String expectedCatalog;
private String expectedDb;
private Map<String, String> expectedHiveConf;
private String uri;

@Parameterized.Parameters
public static Collection<String[]> data() {
public static Collection<Object[]> data() throws UnsupportedEncodingException {
return Arrays.asList(
new String[][] {
{"localhost", "10009", null, "db", "jdbc:hive2:///db;k1=v1?k2=v2#k3=v3"},
{"localhost", "10009", null, "default", "jdbc:hive2:///"},
{"localhost", "10009", null, "default", "jdbc:kyuubi://"},
{"localhost", "10009", null, "default", "jdbc:hive2://"},
{"hostname", "10018", null, "db", "jdbc:hive2://hostname:10018/db;k1=v1?k2=v2#k3=v3"},
new Object[][] {
{
"localhost",
"10009",
null,
"db",
new ImmutableMap.Builder<String, String>().put("k2", "v2").build(),
"jdbc:hive2:///db;k1=v1?k2=v2#k3=v3"
},
{
"localhost",
"10009",
null,
"default",
new ImmutableMap.Builder<String, String>().build(),
"jdbc:hive2:///"
},
{
"localhost",
"10009",
null,
"default",
new ImmutableMap.Builder<String, String>().build(),
"jdbc:kyuubi://"
},
{
"localhost",
"10009",
null,
"default",
new ImmutableMap.Builder<String, String>().build(),
"jdbc:hive2://"
},
{
"hostname",
"10018",
null,
"db",
new ImmutableMap.Builder<String, String>().put("k2", "v2").build(),
"jdbc:hive2://hostname:10018/db;k1=v1?k2=v2#k3=v3"
},
{
"hostname",
"10018",
"catalog",
"db",
new ImmutableMap.Builder<String, String>().put("k2", "v2").build(),
"jdbc:hive2://hostname:10018/catalog/db;k1=v1?k2=v2#k3=v3"
},
{
"hostname",
"10018",
"catalog",
"db",
new ImmutableMap.Builder<String, String>()
.put("k2", "v2")
.put("k3", "-Xmx2g -XX:+PrintGCDetails -XX:HeapDumpPath=/heap.hprof")
.build(),
"jdbc:hive2://hostname:10018/catalog/db;k1=v1?"
+ URLEncoder.encode(
"k2=v2;k3=-Xmx2g -XX:+PrintGCDetails -XX:HeapDumpPath=/heap.hprof",
StandardCharsets.UTF_8.toString())
.replaceAll("\\+", "%20")
+ "#k4=v4"
}
});
}
Expand All @@ -61,11 +119,13 @@ public UtilsTest(
String expectedPort,
String expectedCatalog,
String expectedDb,
Map<String, String> expectedHiveConf,
String uri) {
this.expectedHost = expectedHost;
this.expectedPort = expectedPort;
this.expectedCatalog = expectedCatalog;
this.expectedDb = expectedDb;
this.expectedHiveConf = expectedHiveConf;
this.uri = uri;
}

Expand All @@ -76,5 +136,6 @@ public void testExtractURLComponents() throws JdbcUriParseException {
assertEquals(Integer.parseInt(expectedPort), jdbcConnectionParams1.getPort());
assertEquals(expectedCatalog, jdbcConnectionParams1.getCatalogName());
assertEquals(expectedDb, jdbcConnectionParams1.getDbName());
assertEquals(expectedHiveConf, jdbcConnectionParams1.getHiveConfs());
}
}
}
6 changes: 6 additions & 0 deletions kyuubi-server/src/main/antlr4/org/apache/kyuubi/sql/KyuubiTrinoFeBaseLexer.g4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,12 @@ SCOPE_TABLE: 'SCOPE_TABLE';
SOURCE_DATA_TYPE: 'SOURCE_DATA_TYPE';
IS_AUTOINCREMENT: 'IS_AUTOINCREMENT';
IS_GENERATEDCOLUMN: 'IS_GENERATEDCOLUMN';
VARCHAR: 'VARCHAR';
SMALLINT: 'SMALLINT';
CAST: 'CAST';
AS: 'AS';
KEY_SEQ: 'KEY_SEQ';
PK_NAME: 'PK_NAME';

fragment SEARCH_STRING_ESCAPE: '\'' '\\' '\'';

Copy link

Choose a reason for hiding this comment

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

这段代码看起来只是添加了一些常量，并没有直接运行的代码。因此无法确定是否存在bug风险。但是，以下是一些建议：

  1. 为每个常量添加注释，以便其他开发人员更好地理解这些常量的用途和工作原理。
  2. 根据项目的编程规范，统一命名常量名称的格式。

另外，你可以在将来的代码提交中提供更多上下文信息，以便提供更全面的反馈。

Expand Down
7 changes: 7 additions & 0 deletions kyuubi-server/src/main/antlr4/org/apache/kyuubi/sql/KyuubiTrinoFeBaseParser.g4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,13 @@ statement
SOURCE_DATA_TYPE COMMA IS_AUTOINCREMENT COMMA IS_GENERATEDCOLUMN FROM SYSTEM_JDBC_COLUMNS
(WHERE tableCatalogFilter? AND? tableSchemaFilter? AND? tableNameFilter? AND? colNameFilter?)?
ORDER BY TABLE_CAT COMMA TABLE_SCHEM COMMA TABLE_NAME COMMA ORDINAL_POSITION #getColumns
| SELECT CAST LEFT_PAREN NULL AS VARCHAR RIGHT_PAREN TABLE_CAT COMMA
CAST LEFT_PAREN NULL AS VARCHAR RIGHT_PAREN TABLE_SCHEM COMMA
CAST LEFT_PAREN NULL AS VARCHAR RIGHT_PAREN TABLE_NAME COMMA
CAST LEFT_PAREN NULL AS VARCHAR RIGHT_PAREN COLUMN_NAME COMMA
CAST LEFT_PAREN NULL AS SMALLINT RIGHT_PAREN KEY_SEQ COMMA
CAST LEFT_PAREN NULL AS VARCHAR RIGHT_PAREN PK_NAME
WHERE FALSE #getPrimaryKeys
| .*? #passThrough
;

Copy link

Choose a reason for hiding this comment

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

这段代码是一个正则表达式，可以用来匹配 SQL 语句的字符串。其中第一部分会从 SYSTEM_JDBC_COLUMNS 表中选择特定的列，并根据过滤器（tableCatalogFilter、tableSchemaFilter、tableNameFilter、colNameFilter）进行查询。第二部分则是从任何字符串中捕获所有字符。

根据这个补丁的内容，可以看出作者在数据库的获取和操作上进行了优化，新增了查询主键的功能。同时这个代码片段看上去没有明显的错误，但是如果整块代码给出会更容易进行完整的 review。

Expand Down
7 changes: 6 additions & 1 deletion ...er/src/main/scala/org/apache/kyuubi/server/trino/api/KyuubiTrinoOperationTranslator.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import org.apache.kyuubi.operation.OperationHandle
import org.apache.kyuubi.service.BackendService
import org.apache.kyuubi.sql.parser.trino.KyuubiTrinoFeParser
import org.apache.kyuubi.sql.plan.PassThroughNode
import org.apache.kyuubi.sql.plan.trino.{GetCatalogs, GetColumns, GetSchemas, GetTables, GetTableTypes, GetTypeInfo}
import org.apache.kyuubi.sql.plan.trino.{GetCatalogs, GetColumns, GetPrimaryKeys, GetSchemas, GetTables, GetTableTypes, GetTypeInfo}

class KyuubiTrinoOperationTranslator(backendService: BackendService) {
lazy val parser = new KyuubiTrinoFeParser()
Expand Down Expand Up @@ -68,6 +68,11 @@ class KyuubiTrinoOperationTranslator(backendService: BackendService) {
schemaPattern,
tableNamePattern,
colNamePattern)
case GetPrimaryKeys() =>
val operationHandle = backendService.getPrimaryKeys(sessionHandle, null, null, null)
// The trino implementation always returns empty.
operationHandle.setHasResultSet(false)
operationHandle
case PassThroughNode() =>
backendService.executeStatement(sessionHandle, statement, configs, runAsync, queryTimeout)
}
Copy link

Choose a reason for hiding this comment

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

这段代码的变化是将原来操作Plan中将 GetPrimaryKeys（） 加入引用，执行某些SQL时获取主键。建议考虑以下几点改进和风险：

  • 这段代码看起来不错，很难找到潜在错误或危险。
  • GetPrimaryKeys实现总是返回空值，因此只是简单地创建操作句柄并设置为没有结果集。
  • 您可以考虑添加日志记录和异常处理，以便更好地跟踪异常情况和出现问题的位置。这将有助于快速诊断问题。
  • 最后，您还可以调查一下通过语音转换功能可以将代码转化成其他自然语言。

希望我的答复可以帮到您！

Expand Down
6 changes: 5 additions & 1 deletion ...bi-server/src/main/scala/org/apache/kyuubi/sql/parser/trino/KyuubiTrinoFeAstBuilder.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import org.apache.kyuubi.sql.KyuubiTrinoFeBaseParser._
import org.apache.kyuubi.sql.KyuubiTrinoFeBaseParserBaseVisitor
import org.apache.kyuubi.sql.parser.KyuubiParser.unescapeSQLString
import org.apache.kyuubi.sql.plan.{KyuubiTreeNode, PassThroughNode}
import org.apache.kyuubi.sql.plan.trino.{GetCatalogs, GetColumns, GetSchemas, GetTables, GetTableTypes, GetTypeInfo}
import org.apache.kyuubi.sql.plan.trino.{GetCatalogs, GetColumns, GetPrimaryKeys, GetSchemas, GetTables, GetTableTypes, GetTypeInfo}

class KyuubiTrinoFeAstBuilder extends KyuubiTrinoFeBaseParserBaseVisitor[AnyRef] {

Expand Down Expand Up @@ -92,6 +92,10 @@ class KyuubiTrinoFeAstBuilder extends KyuubiTrinoFeBaseParserBaseVisitor[AnyRef]
GetColumns(catalog, schemaPattern, tableNamePattern, colNamePattern)
}

override def visitGetPrimaryKeys(ctx: GetPrimaryKeysContext): KyuubiTreeNode = {
GetPrimaryKeys()
}

override def visitNullCatalog(ctx: NullCatalogContext): AnyRef = {
null
}
Copy link

Choose a reason for hiding this comment

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

这段代码看起来是在引入一些Trino SQL查询计划的类，并重新实现了KyuubiTrinoFeAstBuilder这个访问者。根据代码变更和上下文，以下是我的一些建议：

  • 可能会有风险：我没有看到可以明确识别已经存在的潜在bug，这段代码可能需要进行额外测试以确保其正确性。
  • 建议改进：如果你只是想添加新的函数功能，那么这份补丁很不错。 在重构代码之前，考虑添加必要的注释，以便其他开发人员更容易阅读新的Trino支持代码。 如果要添加更多的语法支持，可以考虑将每个解析器功能分离成单独的方法，以提高可读性。

希望我的建议能对你有所帮助！

Expand Down
4 changes: 4 additions & 0 deletions kyuubi-server/src/main/scala/org/apache/kyuubi/sql/plan/trino/TrinoFeOperations.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,3 +55,7 @@ case class GetColumns(
colNamePattern: String) extends KyuubiTreeNode {
override def name(): String = "Get Columns"
}

case class GetPrimaryKeys() extends KyuubiTreeNode {
override def name(): String = "Get Primary Keys"
}
17 changes: 16 additions & 1 deletion kyuubi-server/src/test/scala/org/apache/kyuubi/parser/trino/KyuubiTrinoFeParserSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ package org.apache.kyuubi.parser.trino
import org.apache.kyuubi.KyuubiFunSuite
import org.apache.kyuubi.sql.parser.trino.KyuubiTrinoFeParser
import org.apache.kyuubi.sql.plan.{KyuubiTreeNode, PassThroughNode}
import org.apache.kyuubi.sql.plan.trino.{GetCatalogs, GetColumns, GetSchemas, GetTables, GetTableTypes, GetTypeInfo}
import org.apache.kyuubi.sql.plan.trino.{GetCatalogs, GetColumns, GetPrimaryKeys, GetSchemas, GetTables, GetTableTypes, GetTypeInfo}

class KyuubiTrinoFeParserSuite extends KyuubiFunSuite {
val parser = new KyuubiTrinoFeParser()
Expand Down Expand Up @@ -354,4 +354,19 @@ class KyuubiTrinoFeParserSuite extends KyuubiFunSuite {
tableName = "%aa",
colName = "%bb")
}

test("Support GetPrimaryKeys for Trino Fe") {
val kyuubiTreeNode = parse(
"""
| SELECT CAST(NULL AS varchar) TABLE_CAT,
| CAST(NULL AS varchar) TABLE_SCHEM,
| CAST(NULL AS varchar) TABLE_NAME,
| CAST(NULL AS varchar) COLUMN_NAME,
| CAST(NULL AS smallint) KEY_SEQ,
| CAST(NULL AS varchar) PK_NAME
| WHERE false
|""".stripMargin)

assert(kyuubiTreeNode.isInstanceOf[GetPrimaryKeys])
}
}