>From Dmitry Lychagin <[email protected]>: Dmitry Lychagin has posted comments on this change. ( https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9565 )
Change subject: [NO ISSUE] UDF API improvements ...................................................................... Patch Set 19: (11 comments) https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9565/19/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/AbstractNCUdfServlet.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/AbstractNCUdfServlet.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9565/19/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/AbstractNCUdfServlet.java@91 PS19, Line 91: this.dataverse = DataverseName.createFromCanonicalForm(dataverse.getValue()); we shouldn't be using dataverse's canonical form in http parameters. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9565/19/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/AbstractNCUdfServlet.java@101 PS19, Line 101: this.dataverse = DataverseName.createFromCanonicalForm(dataverse.getValue()); we shouldn't be using dataverse's canonical form in http parameters. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9565/19/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/NCUdfApiServlet.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/NCUdfApiServlet.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9565/19/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/NCUdfApiServlet.java@175 PS19, Line 175: if (localPath(request).equals("/") | localPath(request).equals("")) { let's compute localPath(request) once, assign it to a variable and use that variable. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9565/19/asterixdb/asterix-app/src/test/java/org/apache/asterix/app/external/ExternalUDFLibrarian.java File asterixdb/asterix-app/src/test/java/org/apache/asterix/app/external/ExternalUDFLibrarian.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9565/19/asterixdb/asterix-app/src/test/java/org/apache/asterix/app/external/ExternalUDFLibrarian.java@64 PS19, Line 64: entity.addTextBody(dataverseKey, dataverse.toString()); DataverseName.toString() produces the display form. We need to discuss this. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9565/19/asterixdb/asterix-app/src/test/java/org/apache/asterix/app/external/ExternalUDFLibrarian.java@79 PS19, Line 79: entity.addTextBody("dataverse", dataverse.toString()); DataverseName.toString() produces the display form. We need to discuss this. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9565/19/asterixdb/asterix-app/src/test/java/org/apache/asterix/test/common/TestExecutor.java File asterixdb/asterix-app/src/test/java/org/apache/asterix/test/common/TestExecutor.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9565/19/asterixdb/asterix-app/src/test/java/org/apache/asterix/test/common/TestExecutor.java@1252 PS19, Line 1252: librarian.install(path, "dataverse", DataverseName.createSinglePartName(dataverse), library, it'd be nice to support multi-part dataverse names too. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9565/19/asterixdb/asterix-app/src/test/java/org/apache/asterix/test/common/TestExecutor.java@1263 PS19, Line 1263: librarian.uninstall(path, DataverseName.createSinglePartName(dataverse), library, it'd be nice to support multi-part dataverse names too. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9565/19/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/external-library/create-or-replace-function-1/create-or-replace-function-1.2.lib.sqlpp File asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/external-library/create-or-replace-function-1/create-or-replace-function-1.2.lib.sqlpp: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9565/19/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/external-library/create-or-replace-function-1/create-or-replace-function-1.2.lib.sqlpp@19 PS19, Line 19: install externallibtest testlib java admin admin target/data/externallib/asterix-external-data-testlib.zip it's getting harder to understand install command's argument because there are too many now. At some point we should may be consider using a json object instead for them. e.g.: install { dataverse: ..., library: "...", type: "java", .... } https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9565/19/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/external-library/invalid_library_requests/invalid_library_requests.1.lib.sqlpp File asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/external-library/invalid_library_requests/invalid_library_requests.1.lib.sqlpp: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9565/19/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/external-library/invalid_library_requests/invalid_library_requests.1.lib.sqlpp@19 PS19, Line 19: test admin admin dataverse:Default:string name:testlib:string type:badType:string data:target/data/externallib/asterix-external-data-testlib.zip:file what does 'test' command do? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9565/19/asterixdb/asterix-app/src/test/resources/runtimets/testsuite_it_sqlpp.xml File asterixdb/asterix-app/src/test/resources/runtimets/testsuite_it_sqlpp.xml: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9565/19/asterixdb/asterix-app/src/test/resources/runtimets/testsuite_it_sqlpp.xml@67 PS19, Line 67: <expected-error>{ Can we have a single-line expected error message? e.g.: ASX3042: UDF of kind badType not supported. and let the test code verify that it got a proper JSON with "error" field in the error case? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9565/19/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/util/ExternalLibraryUtils.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/util/ExternalLibraryUtils.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9565/19/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/util/ExternalLibraryUtils.java@37 PS19, Line 37: protected static final ObjectMapper OBJECT_MAPPER = new ObjectMapper(); is this used anywhere? -- To view, visit https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9565 To unsubscribe, or for help writing mail filters, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-Project: asterixdb Gerrit-Branch: cheshire-cat Gerrit-Change-Id: Ib6e6ce7debc9c2e07d24163542c1f98886792165 Gerrit-Change-Number: 9565 Gerrit-PatchSet: 19 Gerrit-Owner: Ian Maxon <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Dmitry Lychagin <[email protected]> Gerrit-Reviewer: Ian Maxon <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Michael Blow <[email protected]> Gerrit-Comment-Date: Wed, 03 Mar 2021 02:37:11 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
