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.

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

Support to return null value for OperationsResource rowset #13

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -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;
@@ -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));
}
}
}

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

Choose a reason for hiding this comment

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

这段代码通过引入Apache Commons Lang库中的StringUtils类来提高字符串处理效率。另外,它用正则表达式(Pattern)和Matcher类来解析查询参数字符串中的键值对项,将其添加到connParams中的hiveConfs Map中。这里没有明显的逻辑错误或成本问题,但是未定义KYUUBI_OPERATION_HINT_PATTERN常量,需要检查是否在其他地方定义了它。

Original file line number Diff line number Diff line change
@@ -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;
@@ -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"
}
});
}
@@ -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;
}

@@ -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());
}
}
}
Original file line number Diff line number Diff line change
@@ -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: '\'' '\\' '\'';

Choose a reason for hiding this comment

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

这段代码增加了一些常量定义,没有明显的风险。但有以下改进建议:

  • 常量名应该全大写。
  • 部分常量没有相应的注释说明,需要补充。
  • 对于CAST和AS这样的关键字,应该为其加上特殊标识符以示区别(比如加上下划线),防止与变量名冲突。

除此之外,这段代码没有涉及功能实现,无法进行更细致的审查。

Original file line number Diff line number Diff line change
@@ -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
;

Choose a reason for hiding this comment

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

这段代码使用了正则表达式对 SQL 查询语句进行匹配和分类。具体而言,它覆盖了 "getColumns" 和 "getPrimaryKeys" 两个查询。

对于风险和改进建议方面,我的评论如下:

看起来这段代码只是进行了正则表达式的模式匹配和转换,并没有直接执行任何SQL语句并处理结果。因此,这里的风险应该主要集中在 SQL 语句本身。

首先,“getPrimaryKeys”查询非常简单并且只返回一个空结果的SELECT语句(WHERE FALSE)。这看起来是一种占位符行为,用于避免出现一个基本查询结果为空时产生的异常情况。但是我无法确定这种做法是否在代码中的所有情况下都是安全的。如果其他部分需要借助“getPrimaryKeys”来与数据库交互,则可能会对应用程序产生隐含的问题。因此,我建议将这个查询用更好的方式实现,以便更加清晰明确地提供所需的占位符值。

还有,在第一个查询中,这些 CAST LEFT_PAREN NULL AS VARCHAR 的语句也不太清楚。我的推断是,它们是为了填充 SELECT 子句中的列类型,因为在某些情况下数据库驱动程序无法明确指定每个SELECT的列。但是,除了增加可读性之外,这些 CAST 语句可能会引入其他问题。例如,如果某个数据库中的表不支持NULL值,则强迫它以字符串形式呈现很可能会导致类型错误或语义问题。因此,如果这里使用不当,就有可能导致查询结果无法准确反映实际数据库结构。

对于改进方面,我认为需要更多的文档和注释来解释这些正则表达式及其用途,这样可以使代码更易于维护、扩展和理解。此外,也应该关注已知的SQL注入漏洞,尽量避免在正则表达式中直接嵌入用户提供的输入。

Original file line number Diff line number Diff line change
@@ -182,19 +182,47 @@ private[v1] class OperationsResource extends ApiRequestContext with Logging {
i.getSetField.name(),
i.getSetField match {
case TColumnValue._Fields.STRING_VAL =>
i.getStringVal.getFieldValue(TStringValue._Fields.VALUE)
if (i.getStringVal.isSetValue) {
i.getStringVal.getFieldValue(TStringValue._Fields.VALUE)
} else {
null
}
case TColumnValue._Fields.BOOL_VAL =>
i.getBoolVal.getFieldValue(TBoolValue._Fields.VALUE)
if (i.getBoolVal.isSetValue) {
i.getBoolVal.getFieldValue(TBoolValue._Fields.VALUE)
} else {
null
}
case TColumnValue._Fields.BYTE_VAL =>
i.getByteVal.getFieldValue(TByteValue._Fields.VALUE)
if (i.getByteVal.isSetValue) {
i.getByteVal.getFieldValue(TByteValue._Fields.VALUE)
} else {
null
}
case TColumnValue._Fields.DOUBLE_VAL =>
i.getDoubleVal.getFieldValue(TDoubleValue._Fields.VALUE)
if (i.getDoubleVal.isSetValue) {
i.getDoubleVal.getFieldValue(TDoubleValue._Fields.VALUE)
} else {
null
}
case TColumnValue._Fields.I16_VAL =>
i.getI16Val.getFieldValue(TI16Value._Fields.VALUE)
if (i.getI16Val.isSetValue) {
i.getI16Val.getFieldValue(TI16Value._Fields.VALUE)
} else {
null
}
case TColumnValue._Fields.I32_VAL =>
i.getI32Val.getFieldValue(TI32Value._Fields.VALUE)
if (i.getI32Val.isSetValue) {
i.getI32Val.getFieldValue(TI32Value._Fields.VALUE)
} else {
null
}
case TColumnValue._Fields.I64_VAL =>
i.getI64Val.getFieldValue(TI64Value._Fields.VALUE)
if (i.getI64Val.isSetValue) {
i.getI64Val.getFieldValue(TI64Value._Fields.VALUE)
} else {
null
}
})
}).asJava)
})

Choose a reason for hiding this comment

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

这段代码看起来是在一个Scala代码库中,主要作用可能是将不同类型的值转换为Java表达式。

这段代码中检查了每个i.getSetField,并根据case语句中每种类型的情况,使用if-else块检查是否设置了值。如果设置了值,它会返回value,否则返回null。

建议将重复的if-else块提取出来并封装在函数中,以避免冗余代码。

此外,应该记得添加注释,以便其他开发人员更轻松地理解代码的目的和功能。

Original file line number Diff line number Diff line change
@@ -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()
@@ -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)
}

Choose a reason for hiding this comment

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

这段代码补丁主要有以下几处修改:

  1. 导入了新的 GetPrimaryKeys 类;
  2. translate 方法中增加了对 GetPrimaryKeys 的处理;
  3. 在获取 GetPrimaryKeys 操作结果后,将其标记为无结果集。

从可读性和可维护性角度考虑,此次修改没有明显问题。但是如果当前代码库中存在针对 Trino 获取主键的 bug,那么这个修改可能会掩盖此一问题,难以修复。建议开发人员需结合具体业务和上下文环境进行全面评估,并考虑添加相应测试用例确保此变更的正确性。

Original file line number Diff line number Diff line change
@@ -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] {

@@ -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
}
Original file line number Diff line number Diff line change
@@ -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"
}
Original file line number Diff line number Diff line change
@@ -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()
@@ -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])
}
}
Original file line number Diff line number Diff line change
@@ -126,6 +126,30 @@ class OperationsResourceSuite extends KyuubiFunSuite with RestFrontendTestHelper
assert(logRowSet.getRowCount == 1)
}

test("test get result row set with null value") {
val opHandleStr = getOpHandleStr(
s"""
|select
|cast(null as string) as c1,
|cast(null as boolean) as c2,
|cast(null as byte) as c3,
|cast(null as double) as c4,
|cast(null as short) as c5,
|cast(null as int) as c6,
|cast(null as bigint) as c7
|""".stripMargin)
checkOpState(opHandleStr, FINISHED)
val response = webTarget.path(
s"api/v1/operations/$opHandleStr/rowset")
.queryParam("maxrows", "2")
.queryParam("fetchorientation", "FETCH_NEXT")
.request(MediaType.APPLICATION_JSON).get()
assert(200 == response.getStatus)
val logRowSet = response.readEntity(classOf[ResultRowSet])
assert(logRowSet.getRows.asScala.head.getFields.asScala.forall(_.getValue == null))
assert(logRowSet.getRowCount == 1)
}

def getOpHandleStr(statement: String = "show tables"): String = {
val sessionHandle = fe.be.openSession(
HIVE_CLI_SERVICE_PROTOCOL_V2,