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 <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: abdullah alamoudi <bamou...@gmail.com>
Gerrit-HasComments: Yes

Reply via email to