>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 40: (7 comments) https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9565/40/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/40/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/server/NCUdfApiServlet.java@184 PS40, Line 184: libraryEntry.put(getDataverseParameter(), dvAndLibs.getKey()); dvAndLibs.getKey() is a canonical dataverse name. We should not be returning it as a string. Let's change ExternalLibraryUtils.produceLibraryListing() to return Map<DataverseName, Map<String, String>> and then convert DataverseName it into an array of parts in the output JSON. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9565/40/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/library/ExternalLibraryManager.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/library/ExternalLibraryManager.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9565/40/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/library/ExternalLibraryManager.java@244 PS40, Line 244: public List<Pair<DataverseName, String>> getLibraryListing() throws IOException { minor. Can we consider changing this method to return library hashes too? We already call findLibraryRevDir() in line 284. Might as well read the library descriptor at that point. Then we could eliminate ExternalLibraryUtils.produceLibraryListing() because it's just converting one data structure into another. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9565/40/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/40/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/util/ExternalLibraryUtils.java@45 PS40, Line 45: public static Map<String, Map<String, String>> produceLibraryListing(ILibraryManager libraryManager) See my other comments regarding eliminating this method. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9565/40/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/util/ExternalLibraryUtils.java@48 PS40, Line 48: Map<String, Map<String, String>> dvToLibHashes = new HashMap<>(); Consider using TreeMap instead of HashMap, so we'd get them sorted by the dataverse name. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9565/40/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/util/ExternalLibraryUtils.java@50 PS40, Line 50: dvToLibHashes.computeIfAbsent(lib.first.getCanonicalForm(), h -> new HashMap<>()).put(lib.getSecond(), Consider using TreeMap instead of HashMap, so we'd get them sorted by the library name. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9565/40/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/util/ExternalLibraryUtils.java@51 PS40, Line 51: libraryManager.getLibraryHash(lib.first, lib.second)); getLibraryHash() can theoretically return 'null', but we're not handling it here. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9565/40/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-common/src/main/java/org/apache/hyracks/control/common/controllers/NCConfig.java File hyracks-fullstack/hyracks/hyracks-control/hyracks-control-common/src/main/java/org/apache/hyracks/control/common/controllers/NCConfig.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/9565/40/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-common/src/main/java/org/apache/hyracks/control/common/controllers/NCConfig.java@97 PS40, Line 97: FIND_PYTHON(BOOLEAN, false), Let's consider changing "find.python" to something that starts with "python." to remain aligned with other python options below. Maybe "python.cmd.autodetect" (or "python.cmd.auto" if you think "python.cmd.autodetect" is too long). -- 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: 40 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-Reviewer: Till Westmann <[email protected]> Gerrit-Comment-Date: Tue, 16 Mar 2021 19:45:22 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
