Till Westmann has posted comments on this change. Change subject: [NO ISSUE][EXT] Java UDF framework refactoring ......................................................................
Patch Set 6: (9 comments) Here are a few "reading comments" (i.e. I didn't check this out or test it). In addition to the specific comments, I think that all the subclasses of Object should look * look as similar as possible (have one structure and one order of methods that is repeated in all classes) and * be as small as possible (pull shared or derived functionality into JObject - unless it's too expensive). https://asterix-gerrit.ics.uci.edu/#/c/2405/6/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/api/IJObject.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/api/IJObject.java: PS6, Line 24: ATypeTag Do we still need this? https://asterix-gerrit.ics.uci.edu/#/c/2405/6/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/library/JTypeObjectFactory.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/library/JTypeObjectFactory.java: PS6, Line 35: JOrderedList This seems to be "builtin" as well. Should the package be called "atomic" instead of "builtin"? Or is there another distinction between the atomic and the composite types? https://asterix-gerrit.ics.uci.edu/#/c/2405/6/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/library/java/JObjectAccessors.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/library/java/JObjectAccessors.java: PS6, Line 281: What did this do? PS6, Line 163: JUnorderedListAccessor Why do we have unordered lists, but not ordered lists or records? https://asterix-gerrit.ics.uci.edu/#/c/2405/6/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/library/java/base/JOrderedList.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/library/java/base/JOrderedList.java: PS6, Line 65: UnorderedListBuilder Should this be an ordered list? https://asterix-gerrit.ics.uci.edu/#/c/2405/6/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/library/java/base/builtin/JBuiltinType.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/library/java/base/builtin/JBuiltinType.java: PS6, Line 148: AINT8 AINT16 https://asterix-gerrit.ics.uci.edu/#/c/2405/6/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/library/java/base/builtin/JByte.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/library/java/base/builtin/JByte.java: PS6, Line 41: getValue Why do we have a getter here but not in JCircle? PS6, Line 47: if (writeTypeTag) { : try { : dataOutput.writeByte(value.getType().getTypeTag().serialize()); : } catch (IOException e) { : throw new HyracksDataException(e); : } : } Pull this into a method in JObject? It seems that this code should work for all types. https://asterix-gerrit.ics.uci.edu/#/c/2405/6/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/library/java/base/builtin/JLine.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/library/java/base/builtin/JLine.java: PS6, Line 42: setValue 2 different setters? -- To view, visit https://asterix-gerrit.ics.uci.edu/2405 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3b648191b73fe4aad4f2a6ba1c2066c872fa16a9 Gerrit-PatchSet: 6 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Xikui Wang <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-Reviewer: abdullah alamoudi <[email protected]> Gerrit-HasComments: Yes
