Minghui Zhu 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 4: (9 comments) http://gerrit.cloudera.org:8080/#/c/19023/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19023/2//COMMIT_MSG@1 PS2, Line 1: Parent: 2e6bbbba (IMPALA-11767: Ignore exceptions for invalid paths in Hudi search) > can you add some UT? The visitor code has been deleted, and the code logic has not been modified, but a parser package has been built, so there is no need to add a unit test, or do you have any suggestions for adding a unit test? 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 > nit: please wrap each line to have 72 or fewer characters if possible. Done http://gerrit.cloudera.org:8080/#/c/19023/2//COMMIT_MSG@25 PS2, Line 25: <artifactId>impala-parser</artifactId> : <version>${impala.version}</version> : </dependency> : > I prefer using the existing ExprRewriter for customized expr rewrites. Here I have replaced it with ExprRewriter, which is not universal, so it has not been submitted, and the commit information has been updated. 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: </goals> > Should we activate this by default? This profile is generally used by other projects. When building a package, we can use the command: mvn clean package -DskipTests -Pimpala-parser to manually specify. What is the intention to activate by default here? http://gerrit.cloudera.org:8080/#/c/19023/2/fe/pom.xml@1149 PS2, Line 1149: > Can we append the impala version at the end? Something like impala-parser-4 Done http://gerrit.cloudera.org:8080/#/c/19023/2/fe/src/main/java/org/apache/impala/analysis/StmtNode.java File fe/src/main/java/org/apache/impala/analysis/StmtNode.java: http://gerrit.cloudera.org:8080/#/c/19023/2/fe/src/main/java/org/apache/impala/analysis/StmtNode.java@49 PS2, Line 49: > why use @SuppressWarnings here? This method will be deleted. 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()) { > Same question from me. If we do need such a feature, we can add new methods If we use the createBuiltinWithoutResolvingSymbols method, then we need to add a skip conditional judgment before the createBuiltin method call, which will involve modifications in many places. 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? The createBuiltin method will call the be code through the local method to find the symbol, which requires libfesupport.so when building the parser package. However, it is impossible for the symbol not to be found, so just skip it. http://gerrit.cloudera.org:8080/#/c/19023/2/fe/src/main/java/org/apache/impala/util/StmtVisitor.java File fe/src/main/java/org/apache/impala/util/StmtVisitor.java: http://gerrit.cloudera.org:8080/#/c/19023/2/fe/src/main/java/org/apache/impala/util/StmtVisitor.java@26 PS2, Line 26: > Why don't you use or extend org.apache.impala.util.Visitor? This interface will be deleted. -- 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: 4 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: Mon, 05 Dec 2022 07:50:33 +0000 Gerrit-HasComments: Yes
