Till Westmann has posted comments on this change. Change subject: Add record reader and parser for CAP messages ......................................................................
Patch Set 13: (8 comments) Submitting some comments on a older patchset - sorry. https://asterix-gerrit.ics.uci.edu/#/c/1269/13/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/input/record/reader/stream/CAPMessageRecordReader.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/input/record/reader/stream/CAPMessageRecordReader.java: Line 36: if (collection != null) { Could we replace this by this.recordLvl = Boolean.parseBoolean(collection) ? 1 : 0; ? Line 52: final int IN_SCHEMA_DEFINITION = 7; These are really helpful, but I think that they could be static final class members (or you could us an enum if you don't need the fact that they are ints). Line 82: if (state == IN_START_OF_ELEMENT_NAME) { A nested switch might be better. Line 88: } else if (state == START_OF_PROLOG) { Should this be "IN_PROLOG"? Line 114: if (state == START_OF_ELEMENT_NAME) { A nested switch might be better. Line 129: return newRecordFormed; This is always true, right? https://asterix-gerrit.ics.uci.edu/#/c/1269/13/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/parser/CAPMessageParser.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/parser/CAPMessageParser.java: Line 101: private void resetPools() { Inline reset pools? Line 171: parentPath = String.join(".", curPathStack); As it's only used inside this method, parentPath could be a local variable. However, another concern is that String.join will probably allocate a few objects for every element. It would be interesting to see, if parsing many of these messages will lead to a lot of garbage collection. -- To view, visit https://asterix-gerrit.ics.uci.edu/1269 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia36101a0761973a9edb96b42d3dcc117661301da Gerrit-PatchSet: 13 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Xikui Wang <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-Reviewer: Xikui Wang <[email protected]> Gerrit-HasComments: Yes
