Till Westmann has posted comments on this change. Change subject: [ASTERIXDB-2148][FUN] Add init parameter for external UDF ......................................................................
Patch Set 15: (8 comments) https://asterix-gerrit.ics.uci.edu/#/c/2107/15/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/external/ExternalLibraryUtils.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/external/ExternalLibraryUtils.java: PS15, Line 230: function.getName().trim() Should we pull the trimming of strings out of these function calls and instead create a few well-named local variables? https://asterix-gerrit.ics.uci.edu/#/c/2107/15/asterixdb/asterix-app/src/test/resources/runtimets/queries/external-library/keyword_detector/keyword_detector.5.query.aql File asterixdb/asterix-app/src/test/resources/runtimets/queries/external-library/keyword_detector/keyword_detector.5.query.aql: PS15, Line 24: = true remove? PS15, Line 24: = true remove? https://asterix-gerrit.ics.uci.edu/#/c/2107/15/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/external-library/keyword_detector/keyword_detector.5.query.sqlpp File asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/external-library/keyword_detector/keyword_detector.5.query.sqlpp: PS15, Line 24: = true remove? PS15, Line 24: = true remove? https://asterix-gerrit.ics.uci.edu/#/c/2107/15/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/library/ILibraryManager.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/library/ILibraryManager.java: PS15, Line 62: addFunctionParameters Are we adding one function parameter or more than one? If it's one, it should be called addFunctionParameter. If it's more than one the javadodc (which should be added anyway) should describe how multiple parameters are encoded. https://asterix-gerrit.ics.uci.edu/#/c/2107/15/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/api/IFunctionHelper.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/api/IFunctionHelper.java: PS15, Line 38: getParameters Again, is thin one or more parameters? https://asterix-gerrit.ics.uci.edu/#/c/2107/15/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/library/JavaFunctionHelper.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/library/JavaFunctionHelper.java: PS15, Line 59: parameters One or more parameters? -- To view, visit https://asterix-gerrit.ics.uci.edu/2107 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I567ce0bcac288267595b2565e53fea61e16fbd65 Gerrit-PatchSet: 15 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Xikui Wang <xkk...@gmail.com> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Gerrit-Reviewer: Till Westmann <ti...@apache.org> Gerrit-Reviewer: Xikui Wang <xkk...@gmail.com> Gerrit-HasComments: Yes
