Ali Alsuliman has posted comments on this change. Change subject: [ASTERIXDB-2476][COMP] Array slicing parser syntax ......................................................................
Patch Set 3: (10 comments) https://asterix-gerrit.ics.uci.edu/#/c/3046/3/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/LangExpressionToPlanTranslator.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/LangExpressionToPlanTranslator.java: PS3, Line 760: ria.isAny() I don't think we're using "any" (i.e. ?). This should be removed. PS3, Line 765: leftIndexPair Better to call it startIndexPair PS3, Line 767: rightIndexPair Need to check if the end position is supplied first and then translate it. Better to call it endIndexPair PS3, Line 770: ARRAY_SLICE_WITH_END_POSITION Need to make the right choice whether it's WITH or WITHOUT function based on rightIndexPair PS3, Line 773: f.getArguments().add(new MutableObject<>(rightIndexPair.first)); Depends on whether it WITH or WITHOUT function https://asterix-gerrit.ics.uci.edu/#/c/3046/3/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/list/list-slice_01/list-slice_01.3.query.sqlpp File asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/list/list-slice_01/list-slice_01.3.query.sqlpp: PS3, Line 20: use test; Can we add a test case without end position? Also, let's try all possible variations: 1. function calls in place of literals for position expressions. 2. the position expression is a subquery (need to write a good one that returns an integer. You should also check what happens if the subquery does not return an integer, i.e. double, array, object, ... etc) 3. the input array is coming from tuples (i.e. slice an array field of a tuple) 4. the input array is coming from a result produced by a query. https://asterix-gerrit.ics.uci.edu/#/c/3046/3/asterixdb/asterix-lang-aql/src/main/java/org/apache/asterix/lang/aql/visitor/base/AbstractAqlSimpleExpressionVisitor.java File asterixdb/asterix-lang-aql/src/main/java/org/apache/asterix/lang/aql/visitor/base/AbstractAqlSimpleExpressionVisitor.java: PS3, Line 56: AbstractAqlQueryExpressionVisitor I believe slice syntax is only for SQL++. Check WindowExpression and see how it's handling that. https://asterix-gerrit.ics.uci.edu/#/c/3046/3/asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/expression/RangeIndexAccessor.java File asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/expression/RangeIndexAccessor.java: PS3, Line 28: private boolean isAny; I believe this should be removed based on the slice syntax. We're returning a definite range that is determined by a definite start index (and possibly end index) PS3, Line 34: startIndexExpression startIndexExpression cannot be null. We have to have it. As a reference, check WindowExpression. https://asterix-gerrit.ics.uci.edu/#/c/3046/3/asterixdb/asterix-lang-sqlpp/src/main/javacc/SQLPP.jj File asterixdb/asterix-lang-sqlpp/src/main/javacc/SQLPP.jj: PS3, Line 2340: (<COLON> : ( secondExpression = Expression() : { : if (secondExpression.getKind() == Expression.Kind.LITERAL_EXPRESSION) : { : Literal lit = ((LiteralExpr)secondExpression).getValue(); : if (lit.getLiteralType() != Literal.Type.INTEGER && : lit.getLiteralType() != Literal.Type.LONG) { : throw new SqlppParseException(secondExpression.getSourceLocation(), "Index should be an INTEGER"); : } : } : } : ) : )? I think there is a problem in the grammar now. With this, the slice syntax would always require an end position expression. You need to LOOKAHEAD and see if <COLON> is present and then commit to the decision that it's a range index expression, and then check if it has a second expression (the end expression?). Otherwise (if no COLON), it's the usual index accessor. -- To view, visit https://asterix-gerrit.ics.uci.edu/3046 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie83283bfd0a04257b59b573de3dab6b3e47de1bf Gerrit-PatchSet: 3 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Hussain Towaileb <hussai...@gmail.com> Gerrit-Reviewer: Ali Alsuliman <ali.al.solai...@gmail.com> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Gerrit-HasComments: Yes
