Preston Carman has posted comments on this change. Change subject: ASTERIXDB-1281 - Interval format update to AQL and ADM ......................................................................
Patch Set 6: (13 comments) https://asterix-gerrit.ics.uci.edu/#/c/602/6/asterix-app/src/test/resources/runtimets/queries/constructor/interval/interval.3.query.aql File asterix-app/src/test/resources/runtimets/queries/constructor/interval/interval.3.query.aql: Line 32: return {"interval71": $itv71, "interval72": $itv72, "interval73": $itv73, "interval74": $itv74, "interval75": $itv75, "interval76": $itv76, "interval77": $itv77, "interval78": $itv78, "interval79": $itv79} > Did we lose the ability to create an interval from a start point and a dura I guess I got a little over zealous with me clean up. Those functions functions should have been left here. https://asterix-gerrit.ics.uci.edu/#/c/602/6/asterix-app/src/test/resources/runtimets/testsuite.xml File asterix-app/src/test/resources/runtimets/testsuite.xml: Line 6300: <test-case FilePath="temporal"> > Which tests did we remove here? I found this test was being run twice. Once for the constructor group and then again in the temporal group. I just removed it from the temporal group. https://asterix-gerrit.ics.uci.edu/#/c/602/6/asterix-app/src/test/resources/runtimets/testsuite_sqlpp.xml File asterix-app/src/test/resources/runtimets/testsuite_sqlpp.xml: Line 6126: </compilation-unit> > Which tests did we remove here? Duplicated test. https://asterix-gerrit.ics.uci.edu/#/c/602/6/asterix-app/src/test/resources/runtimets/testsuite_sqlpp_parser.xml File asterix-app/src/test/resources/runtimets/testsuite_sqlpp_parser.xml: Line 6350: </compilation-unit> > Which tests did we remove here? Duplicated test. https://asterix-gerrit.ics.uci.edu/#/c/602/6/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 341: throw new ParseException(mismatchErrorMessage + objectType.getTypeTag()); > For the other types we use the type name, not the type tag. Should we do th Done. Also fixed a few other type messages. Line 671: throws IOException { > It seems that the line break is a little early. Done Line 715: throws IOException { > It seems that the line break is a little early. Done Line 718: token = admLexer.next(); > Single line would be enough :) Done Line 730: return end; > Challenge: I can write this method in 9 lines without changing semantics or I see a way to do it, but my idea breaks from the way the rest of the class has been written. I think this is clean and similar to the rest of the methods in this file. Line 768: throw new ParseException("Interval argument properly constructed."); > "not"? Done https://asterix-gerrit.ics.uci.edu/#/c/602/6/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/printers/adm/AIntervalPrinter.java File asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/printers/adm/AIntervalPrinter.java: Line 71: } > Couldn't we just do this? Done, also used a switch statement. https://asterix-gerrit.ics.uci.edu/#/c/602/6/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/printers/json/clean/AIntervalPrinter.java File asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/printers/json/clean/AIntervalPrinter.java: Line 74: } > Same as adm/AIntervalPrinter.java? Done https://asterix-gerrit.ics.uci.edu/#/c/602/6/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/printers/json/lossless/AIntervalPrinter.java File asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/printers/json/lossless/AIntervalPrinter.java: Line 74: } > Same as adm/AIntervalPrinter.java? Done -- 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: 6 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
