>From Ian Maxon <[email protected]>: Ian Maxon has posted comments on this change. ( https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9565 )
Change subject: [NO ISSUE] UDF API improvements ...................................................................... Patch Set 20: (10 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. Done 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. Done 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. Done 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. converted to use multiparts 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. converted to use multiparts 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. done, not in a particularly robust way, but hopefully it suffices 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. […] yeah it is obtuse at the moment 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? this takes each field to be a piece of a multipart form request. this makes it so i can test all the combinations of invalid parameters to be sure they fail with an understandable message. i kind of wanted to add this in a different way but it ended up being cleaner to do it this way, because a lot of our api/query test harness kind of assumes the body and parameters are all strings 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? ah no sorry not anymore, moved it in some last minute tweaks -- 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: 20 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 23:49:28 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Dmitry Lychagin <[email protected]> Gerrit-MessageType: comment
