[KYUUBI apache#4320] Support GetPrimaryKeys for Trino Fe #12
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>
![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.
这个代码补丁主要引入了一个新的依赖 - org.apache.commons.lang3.StringUtils，并对一些变量和方法进行了修改。
在方法 extractURLComponents 中，现在使用的是 Pattern 类编译的正则表达式，其中 confPattern 对象的定义被移动到了方法内部，并修改了 while 循环将结果存储到 Map 对象中。同时，还增加了一些新的检查，以避免错误或意外的输入数据。
在 isKyuubiOperationHint 方法中，现在使用的是一个匹配器对象，对 KYUUBI_OPERATION_HINT_PATTERN 进行了预编译，以提高性能。此外，该方法没有任何更改。
put 语句中的 Map 对象如果接收 null 值会导致 NullPointerException 错误。建议在向 Map 中添加之前创建它，每次都判断一下异常情况。
总的来说，这个补丁采取了良好的编码风格和技术实践。虽然可能仍存在一些潜在的错误或忽略的边界情况，但这是必要的优化和重构。
| CAST: 'CAST'; | ||
| AS: 'AS'; | ||
| KEY_SEQ: 'KEY_SEQ'; | ||
| PK_NAME: 'PK_NAME'; | ||
|
|
||
| fragment SEARCH_STRING_ESCAPE: '\'' '\\' '\''; | ||
|
|
There was a problem hiding this comment.
这段代码看起来只是添加了一些常量，并没有直接运行的代码。因此无法确定是否存在bug风险。但是，以下是一些建议：
- 为每个常量添加注释，以便其他开发人员更好地理解这些常量的用途和工作原理。
- 根据项目的编程规范，统一命名常量名称的格式。
另外，你可以在将来的代码提交中提供更多上下文信息，以便提供更全面的反馈。
| 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 语句的字符串。其中第一部分会从 SYSTEM_JDBC_COLUMNS 表中选择特定的列，并根据过滤器（tableCatalogFilter、tableSchemaFilter、tableNameFilter、colNameFilter）进行查询。第二部分则是从任何字符串中捕获所有字符。
根据这个补丁的内容，可以看出作者在数据库的获取和操作上进行了优化，新增了查询主键的功能。同时这个代码片段看上去没有明显的错误，但是如果整块代码给出会更容易进行完整的 review。
| 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.
这段代码的变化是将原来操作Plan中将 GetPrimaryKeys（） 加入引用，执行某些SQL时获取主键。建议考虑以下几点改进和风险：
- 这段代码看起来不错，很难找到潜在错误或危险。
- GetPrimaryKeys实现总是返回空值，因此只是简单地创建操作句柄并设置为没有结果集。
- 您可以考虑添加日志记录和异常处理，以便更好地跟踪异常情况和出现问题的位置。这将有助于快速诊断问题。
- 最后，您还可以调查一下通过语音转换功能可以将代码转化成其他自然语言。
希望我的答复可以帮到您！
| override def visitGetPrimaryKeys(ctx: GetPrimaryKeysContext): KyuubiTreeNode = { | ||
| GetPrimaryKeys() | ||
| } | ||
|
|
||
| override def visitNullCatalog(ctx: NullCatalogContext): AnyRef = { | ||
| null | ||
| } |
There was a problem hiding this comment.
这段代码看起来是在引入一些Trino SQL查询计划的类，并重新实现了KyuubiTrinoFeAstBuilder这个访问者。根据代码变更和上下文，以下是我的一些建议：
- 可能会有风险：我没有看到可以明确识别已经存在的潜在
bug，这段代码可能需要进行额外测试以确保其正确性。 - 建议改进：如果你只是想添加新的函数功能，那么这份补丁很不错。 在重构代码之前，考虑添加必要的注释，以便其他开发人员更容易阅读新的Trino支持代码。 如果要添加更多的语法支持，可以考虑将每个解析器功能分离成单独的方法，以提高可读性。
希望我的建议能对你有所帮助！
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