Xikui Wang has posted comments on this change. Change subject: [NO ISSUE][EXT] Java UDF framework refactoring ......................................................................
Patch Set 6: (9 comments) Addressed the comments. Thank you Till. You are the best! 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? Done 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" i I moved everything out and put all datatypes under the base directory to be consistent with AObjects. 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? This method removes line breaks and non-ascii characters, which I don't think makes much sense... Why would we want to filter data out... PS6, Line 163: JUnorderedListAccessor > Why do we have unordered lists, but not ordered lists or records? Done. Composite values are handled differently in JObjectPointableVisitor. 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? Done 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 Done 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? Done. Fixed for other classes as well. 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 Done. Good idea. Fixed 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? AMutableLine and ALine don't support set each point individually, so... -- 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 <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-Reviewer: abdullah alamoudi <bamou...@gmail.com> Gerrit-HasComments: Yes
