Clp integration mysql metadata #1
Conversation
| catch (SQLException e) { | ||
| log.error(e, "Failed to connect to metadata database"); | ||
| return ImmutableList.of(); | ||
| } | ||
| finally { | ||
| // Closing the connection | ||
| try { | ||
| if (connection != null) { | ||
| connection.close(); | ||
| } | ||
| } | ||
| catch (SQLException ex) { | ||
| log.warn(ex, "Failed to close metadata database connection"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Seems such try-catch is heavily used when doing SQL operations. Can we wrap it into a function or class (say MetadataQueryRunner), for example, which takes a function call as parameter.
There was a problem hiding this comment.
This finally code block was removed
| private String getVariableName(VariableReferenceExpression variable) | ||
| { | ||
| String variableName = ((ClpColumnHandle) assignments.get(variable)).getColumnName(); | ||
| if (variableName.endsWith("_bigint") || variableName.endsWith("_double") || |
There was a problem hiding this comment.
Shall we move these constant strings "_bigint" etc. into ClpColumnHandle. Actually, I figure we can move this function too ClpColumnHandle, make it a member method of it
| return variableName; | ||
| } | ||
|
|
||
| private ClpExpression handleNot(CallExpression node) |
There was a problem hiding this comment.
Shall we add a TODO comment in each of these handle* functions? Say we will support more complicated logic expression in the future.
There was a problem hiding this comment.
Can you show an example of more complicated logic? I don't think KQL supports more
There was a problem hiding this comment.
for example more logic operators can work in a row (A AND B AND C OR D ...). If KQL doesn't not support that shall we mention that in the TODO, because this expects to be one of the basic feature of regular SQL
There was a problem hiding this comment.
I think it supports such logic. It's basically AND and OR, no new operators.
|
It’s supported.
…On Mon, Mar 17, 2025 at 11:32 Xiaochong(Eddy) Wei ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In presto-clp/src/main/java/com/yscope/presto/ClpFilterToKqlConverter.java
<#1 (comment)>:
> + return ((Slice) literal.getValue()).toStringUtf8();
+ }
+ return literal.toString();
+ }
+
+ private String getVariableName(VariableReferenceExpression variable)
+ {
+ String variableName = ((ClpColumnHandle) assignments.get(variable)).getColumnName();
+ if (variableName.endsWith("_bigint") || variableName.endsWith("_double") ||
+ variableName.endsWith("_varchar") || variableName.endsWith("_boolean")) {
+ return variableName.substring(0, variableName.lastIndexOf('_'));
+ }
+ return variableName;
+ }
+
+ private ClpExpression handleNot(CallExpression node)
for example more logic operators can work in a row (A AND B AND C OR D
...). If KQL doesn't not support that shall we mention that in the TODO,
because this expects to be one of the basic feature of regular SQL
—
Reply to this email directly, view it on GitHub
<#1 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AI4LBM3OK2Z4DQ2TUIRVUC32U3THPAVCNFSM6AAAAABXOXCXRGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDMOJRGAZTSMJQGE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Description
Motivation and Context
Impact
Test Plan
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.
If release note is NOT required, use: