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 4: (6 comments) Thanks for updating the patch! I just have concern on adding the new flag, since it exposes unrelated internal info to the impala-parser user. Adding more reviewers to get their ideas. http://gerrit.cloudera.org:8080/#/c/19023/4/common/thrift/BackendGflags.thrift File common/thrift/BackendGflags.thrift: http://gerrit.cloudera.org:8080/#/c/19023/4/common/thrift/BackendGflags.thrift@233 PS4, Line 233: skip_lookup_symbol nit: "skip_symbol_lookup_in_builtins" is more clear for me 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> > This profile is generally used by other projects. When building a package, I'm thinking about adding an example/test like this https://github.com/stiga-huang/impala-parser-helloworld . So would need it be generated by default. The example/test can avoid future changes break the basic usage of this jar. What's your opinion? http://gerrit.cloudera.org:8080/#/c/19023/4/fe/pom.xml File fe/pom.xml: http://gerrit.cloudera.org:8080/#/c/19023/4/fe/pom.xml@1134 PS4, Line 1134: impala-minimal-hive-exec Could you explain why we need impala-minimal-hive-exec in parsing a query string? It'd be nice to add a comment for it. http://gerrit.cloudera.org:8080/#/c/19023/4/fe/pom.xml@1136 PS4, Line 1136: </artifactSet> Can we exclude some classes that are not used in query parsing? E.g. I tried adding these <filters> <filter> <artifact>org.apache.impala</artifact> <excludes> <exclude>org/apache/impala/catalog/events/**</exclude> <exclude>org/apache/impala/catalog/local/**</exclude> <exclude>org/apache/impala/catalog/metastore/**</exclude> <exclude>org/apache/impala/catalog/monitor/**</exclude> <exclude>org/apache/impala/planner/**</exclude> </excludes> </filter> </filters> The parser jar is still usable in my hello-world project: https://github.com/stiga-huang/impala-parser-helloworld http://gerrit.cloudera.org:8080/#/c/19023/4/fe/pom.xml@1137 PS4, Line 1137: 4.2.0-RELEASE We should not hard-code the version string here. Let's replaced it with ${project.version} 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()) { > The createBuiltin method will call the be code through the local method to I see. Share the stacktrace I got without this skip BTW: at org.apache.impala.util.NativeLibUtil.loadLibrary (NativeLibUtil.java:66) at org.apache.impala.service.FeSupport.loadLibrary (FeSupport.java:491) at org.apache.impala.service.FeSupport.LookupSymbol (FeSupport.java:213) at org.apache.impala.service.FeSupport.LookupSymbol (FeSupport.java:222) at org.apache.impala.catalog.Function.lookupSymbol (Function.java:475) at org.apache.impala.catalog.ScalarFunction.createBuiltin (ScalarFunction.java:198) at org.apache.impala.catalog.ScalarFunction.createBuiltinOperator (ScalarFunction.java:180) at org.apache.impala.catalog.ScalarFunction.createBuiltinOperator (ScalarFunction.java:175) at org.apache.impala.analysis.ArithmeticExpr.initBuiltins (ArithmeticExpr.java:105) at org.apache.impala.catalog.BuiltinsDb.initBuiltins (BuiltinsDb.java:105) at org.apache.impala.catalog.BuiltinsDb.<init> (BuiltinsDb.java:94) at org.apache.impala.catalog.BuiltinsDb.getInstance (BuiltinsDb.java:79) at org.apache.impala.analysis.SqlScanner.init (SqlScanner.java:750) at org.apache.impala.analysis.SqlScanner.<clinit> (SqlScanner.java:766) at org.apache.impala.analysis.Parser.parse (Parser.java:59) at org.apache.impala.analysis.Parser.parse (Parser.java:42) However, this seems a hack for me. It's not that maintainable since we might need more flags like this if we found other logics should be skipped in the future. We do have some flags for external frontend. But it seems none of them can skip this. Generally, resolving the function names is part of the analyze phase. We should not init the builtin symbols when initializing the parser. But I don't have a better solution. Let's see if other reviewers have more ideas. -- 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: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jian Zhang <[email protected]> Gerrit-Reviewer: John Sherman <[email protected]> Gerrit-Reviewer: Minghui Zhu <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Steve Carlin <[email protected]> Gerrit-Reviewer: Xiang Yang <[email protected]> Gerrit-Comment-Date: Tue, 06 Dec 2022 09:34:51 +0000 Gerrit-HasComments: Yes
