Till Westmann has posted comments on this change. Change subject: ASTERIXDB-1281 - Interval format update to AQL and ADM ......................................................................
Patch Set 6: (13 comments) Looks good. There are a few small points, but there's one big question about creating intervals from a start point and a duration. It seems that this change removes some test coverage in that area and I'm wondering if a) we're also losing the functionality and b) if that's intended. 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 duration? 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? 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? 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? 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 the same here? Line 671: throws IOException { It seems that the line break is a little early. Line 715: throws IOException { It seems that the line break is a little early. Line 718: token = admLexer.next(); Single line would be enough :) Line 730: return end; Challenge: I can write this method in 9 lines without changing semantics or violating the code style. (Ok, this is not an issue that needs addressing :) .) Line 768: throw new ParseException("Interval argument properly constructed."); "not"? 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? if (typetag == ATypeTag.SERIALIZED_DATE_TYPE_TAG) { timeInstancePrinter = ADatePrinter.INSTANCE; } else if (typetag == ATypeTag.SERIALIZED_TIME_TYPE_TAG) { timeInstancePrinter = ATimePrinter.INSTANCE; } else if (typetag == ATypeTag.SERIALIZED_DATETIME_TYPE_TAG) { timeInstancePrinter = ADateTimePrinter.INSTANCE; } else { throw new AlgebricksException("Unsupport internal time types in interval: " + typetag); } timeInstancePrinter.print(b, startOffset, startSize, ps); ps.print(", "); timeInstancePrinter.print(b, endOffset, endSize, ps); 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? 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? -- 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
