Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/19023 )
Change subject: IMPALA-11566: Provide SQL parsing capabilities to other applications in the form of jar packages ...................................................................... Patch Set 2: (5 comments) Thanks for working on this! I think this will benefit a lot for the community. http://gerrit.cloudera.org:8080/#/c/19023/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19023/2//COMMIT_MSG@9 PS2, Line 9: We have a requirement to rewrite the parameters of a function in SQL, and there will be more requirements related to SQL parsing in the future. Before, we used jsqlParser for parsing. In order to be more compatible with impala sql, we want to use impala's parser. : : Not all statements currently support the toSql method, so to provide complete parsing capabilities, it is also necessary to support the toSql method of other statements, such as AlterTableStmt. But in our scenario, there are only select and use statements, which are already supported. So all we need to do is to analyze all the functions in sql and type out the parser package. nit: please wrap each line to have 72 or fewer characters if possible. https://cwiki.apache.org/confluence/display/IMPALA/Contributing+to+Impala http://gerrit.cloudera.org:8080/#/c/19023/2//COMMIT_MSG@25 PS2, Line 25: FunctionCallExprVisitor functionCallExprVisitor = new FunctionCallExprVisitor(); : node.accept(functionCallExprVisitor); : List<FunctionCallExpr> funcCalls = functionCallExprVisitor.getFuncCalls(); : // do something with funcCalla I prefer using the existing ExprRewriter for customized expr rewrites. Here is the entrance: https://github.com/apache/impala/blob/d47d305bf42f963ba6fac18c014e96c734e9e9ff/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java#L561 The pros is each statement already implements the rewriteExprs() interface that gets a ExprRewriter: https://github.com/apache/impala/blob/3554b0752d4be142dabb82b6b751323cde5323d8/fe/src/main/java/org/apache/impala/analysis/StatementBase.java#L128 If so, the code looks like public String parse(String sql) { StatementBase stmt = Parser.parse(sql); ExprRewriter exprRewriter = new MyExprRewritter(); stmt.rewriteExprs(exprRewriter); return stmt.toSql(); } We will need to remove some preconditions like "Preconditions.checkState(isAnalyzed())" https://github.com/apache/impala/blob/b91aa065377a1d154cd9eb5f5cd9ffb6da919b65/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java#L1382 http://gerrit.cloudera.org:8080/#/c/19023/2/fe/pom.xml File fe/pom.xml: http://gerrit.cloudera.org:8080/#/c/19023/2/fe/pom.xml@1130 PS2, Line 1130: <id>impala-parser</id> Should we activate this by default? <activation> <activeByDefault>true</activeByDefault> </activation> http://gerrit.cloudera.org:8080/#/c/19023/2/fe/pom.xml@1149 PS2, Line 1149: impala-parser Can we append the impala version at the end? Something like impala-parser-4.2.0-RELEASE.jar makes more sense to me. http://gerrit.cloudera.org:8080/#/c/19023/2/fe/src/main/java/org/apache/impala/catalog/ScalarFunction.java File fe/src/main/java/org/apache/impala/catalog/ScalarFunction.java: http://gerrit.cloudera.org:8080/#/c/19023/2/fe/src/main/java/org/apache/impala/catalog/ScalarFunction.java@82 PS2, Line 82: if (BackendConfig.INSTANCE != null && BackendConfig.INSTANCE.skipLookupSymbol()) { > what is skipLookupSymbol() for? Same question from me. If we do need such a feature, we can add new methods like createBuiltinWithoutResolvingSymbols individually. Or do we need this method being used without resolving the symbols? -- To view, visit http://gerrit.cloudera.org:8080/19023 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I246b088310f25b809d9e598c0ecc6613d9749ee3 Gerrit-Change-Number: 19023 Gerrit-PatchSet: 2 Gerrit-Owner: Minghui Zhu <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jian Zhang <[email protected]> Gerrit-Reviewer: Minghui Zhu <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Xiang Yang <[email protected]> Gerrit-Comment-Date: Tue, 25 Oct 2022 07:38:45 +0000 Gerrit-HasComments: Yes
