Preston Carman has posted comments on this change. Change subject: ASTERIXDB-1281 - Interval format update to AQL and ADM ......................................................................
Patch Set 2: (11 comments) Follow up comment and a new patch. https://asterix-gerrit.ics.uci.edu/#/c/602/2/asterix-external-data/src/main/java/org/apache/asterix/external/parser/ADMDataParser.java File asterix-external-data/src/main/java/org/apache/asterix/external/parser/ADMDataParser.java: Line 705: if (token == AdmLexer.TOKEN_CONSTRUCTOR_OPEN) { > Could we replace these ifs with a switch? Take a look at the new version and see if you think we need more changes. Line 711: if (token == AdmLexer.TOKEN_COMMA) { > I think that this if-statement should be replaceable with a small method th I created a second argument parser. Line 717: + AdmLexer.tokenKindToString(startToken) + " != " + AdmLexer.tokenKindToString(token)); > It seems that startToken is an "(", if we are here. That's probably not int Good catch. startToken is supposed to be the first argument type. Line 780: if (tag == ATypeTag.DATE) { > Can we replace this with a switch? Done Line 782: chrononTimeInMs += (parseDatePart(arg, 0, arg.length() - 1) > This code exists somewhere else as well, right? Can we reuse that? I am reusing the date parsing logic, but the divide statement is propagated only for interval date. Line 784: } catch (Exception e) { > Can we move the exception handling code outside the switch (if we need it)? I was able to remove exception after looking at the function throwing this error. Line 785: throw new HyracksDataException(e); > Why would we throw a HyracksDataException here? Shouldn't this be a ParseEx Done Line 797: throw new AlgebricksException( > Why is this an AlgebricksException? This is happening at runtime, right? Done https://asterix-gerrit.ics.uci.edu/#/c/602/2/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/serde/AOrderedListSerializerDeserializer.java File asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/serde/AOrderedListSerializerDeserializer.java: Line 81: fixedSize = NonTaggedFormatUtil.isFixedSizedCollection(typeTag); > Yes! Done https://asterix-gerrit.ics.uci.edu/#/c/602/2/asterix-om/src/main/java/org/apache/asterix/om/util/NonTaggedFormatUtil.java File asterix-om/src/main/java/org/apache/asterix/om/util/NonTaggedFormatUtil.java: Line 71: public static final boolean isFixedSizedCollection(ATypeTag type) { > Why do we have a different result here than for the other method of the sam The arguments are slightly different. IAType has additional methods while the tag is only available for specific cases. I updated the functions to share more of the switch statement. https://asterix-gerrit.ics.uci.edu/#/c/602/2/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/constructors/AIntervalConstructorDescriptor.java File asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/constructors/AIntervalConstructorDescriptor.java: Line 116: if (argOut0.getByteArray()[0] == SER_DATE_TYPE_TAG) { > Could this be a switch? Seems like it could be. I get the following error message "case expressions must be constant expressions". Do you know a way to define the SER_DATE_TYPE_TAG to make a constant expression at compile time? -- To view, visit https://asterix-gerrit.ics.uci.edu/602 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I009c71b7a445d141e228ba15d56d0b6cf3c8a3f5 Gerrit-PatchSet: 2 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Preston Carman <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Preston Carman <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-HasComments: Yes
