Support to return null value for OperationsResource rowset #13
Conversation
### _Why are the changes needed?_ Support GetPrimaryKeys for Trino Fe, close apache#4320 ### _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 Closes apache#4321 from Yikf/primaryKeys. Closes apache#4320 3690a2c [Yikf] Support GetPrimaryKeys for Trino Fe Authored-by: Yikf <yikaifei@apache.org> Signed-off-by: ulyssesyou <ulyssesyou@apache.org>
…rce rowset ### _Why are the changes needed?_ With restful api, for query `select cast(null as int) c1` #### Before the result is `0` #### After the result is `null`. ### _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 Closes apache#4372 from turboFei/null_value. Closes apache#4372 b15f1eb [fwang12] nit 5f16855 [fwang12] fix 45c60dd [fwang12] check is set Authored-by: fwang12 <fwang12@ebay.com> Signed-off-by: fwang12 <fwang12@ebay.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -477,4 +486,4 @@ public static String getCanonicalHostName(String hostName) { | |||
| public static boolean isKyuubiOperationHint(String hint) { | |||
| return KYUUBI_OPERATION_HINT_PATTERN.matcher(hint).matches(); | |||
| } | |||
| } | |||
| } No newline at end of file | |||
There was a problem hiding this comment.
这段代码通过引入Apache Commons Lang库中的StringUtils类来提高字符串处理效率。另外，它用正则表达式（Pattern）和Matcher类来解析查询参数字符串中的键值对项，将其添加到connParams中的hiveConfs Map中。这里没有明显的逻辑错误或成本问题，但是未定义KYUUBI_OPERATION_HINT_PATTERN常量，需要检查是否在其他地方定义了它。
| CAST: 'CAST'; | ||
| AS: 'AS'; | ||
| KEY_SEQ: 'KEY_SEQ'; | ||
| PK_NAME: 'PK_NAME'; | ||
|
|
||
| fragment SEARCH_STRING_ESCAPE: '\'' '\\' '\''; | ||
|
|
There was a problem hiding this comment.
这段代码增加了一些常量定义，没有明显的风险。但有以下改进建议：
- 常量名应该全大写。
- 部分常量没有相应的注释说明，需要补充。
- 对于CAST和AS这样的关键字，应该为其加上特殊标识符以示区别（比如加上下划线），防止与变量名冲突。
除此之外，这段代码没有涉及功能实现，无法进行更细致的审查。
| 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 | ||
| ; | ||
|
|
There was a problem hiding this comment.
这段代码使用了正则表达式对 SQL 查询语句进行匹配和分类。具体而言，它覆盖了 "getColumns" 和 "getPrimaryKeys" 两个查询。
对于风险和改进建议方面，我的评论如下：
看起来这段代码只是进行了正则表达式的模式匹配和转换，并没有直接执行任何SQL语句并处理结果。因此，这里的风险应该主要集中在 SQL 语句本身。
首先，“getPrimaryKeys”查询非常简单并且只返回一个空结果的SELECT语句（WHERE FALSE）。这看起来是一种占位符行为，用于避免出现一个基本查询结果为空时产生的异常情况。但是我无法确定这种做法是否在代码中的所有情况下都是安全的。如果其他部分需要借助“getPrimaryKeys”来与数据库交互，则可能会对应用程序产生隐含的问题。因此，我建议将这个查询用更好的方式实现，以便更加清晰明确地提供所需的占位符值。
还有，在第一个查询中，这些 CAST LEFT_PAREN NULL AS VARCHAR 的语句也不太清楚。我的推断是，它们是为了填充 SELECT 子句中的列类型，因为在某些情况下数据库驱动程序无法明确指定每个SELECT的列。但是，除了增加可读性之外，这些 CAST 语句可能会引入其他问题。例如，如果某个数据库中的表不支持NULL值，则强迫它以字符串形式呈现很可能会导致类型错误或语义问题。因此，如果这里使用不当，就有可能导致查询结果无法准确反映实际数据库结构。
对于改进方面，我认为需要更多的文档和注释来解释这些正则表达式及其用途，这样可以使代码更易于维护、扩展和理解。此外，也应该关注已知的SQL注入漏洞，尽量避免在正则表达式中直接嵌入用户提供的输入。
| i.getI64Val.getFieldValue(TI64Value._Fields.VALUE) | ||
| } else { | ||
| null | ||
| } | ||
| }) | ||
| }).asJava) | ||
| }) |
There was a problem hiding this comment.
这段代码看起来是在一个Scala代码库中，主要作用可能是将不同类型的值转换为Java表达式。
这段代码中检查了每个i.getSetField，并根据case语句中每种类型的情况，使用if-else块检查是否设置了值。如果设置了值，它会返回value，否则返回null。
建议将重复的if-else块提取出来并封装在函数中，以避免冗余代码。
此外，应该记得添加注释，以便其他开发人员更轻松地理解代码的目的和功能。
| 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) | ||
| } |
There was a problem hiding this comment.
这段代码补丁主要有以下几处修改：
- 导入了新的
GetPrimaryKeys类； - 在
translate方法中增加了对GetPrimaryKeys的处理； - 在获取
GetPrimaryKeys操作结果后，将其标记为无结果集。
从可读性和可维护性角度考虑，此次修改没有明显问题。但是如果当前代码库中存在针对 Trino 获取主键的 bug，那么这个修改可能会掩盖此一问题，难以修复。建议开发人员需结合具体业务和上下文环境进行全面评估，并考虑添加相应测试用例确保此变更的正确性。
Why are the changes needed?
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