Dmitry Lychagin has posted comments on this change. Change subject: [ASTERIXDB-2476][COMP] Array slicing parser syntax ......................................................................
Patch Set 6: (9 comments) https://asterix-gerrit.ics.uci.edu/#/c/3046/6/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/SqlppExpressionToPlanTranslator.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/SqlppExpressionToPlanTranslator.java: Line 1113: langExprToAlgExpression(expression.getStartIndexExpression(), tupSource); I think you need to give it expressionPair.second instead of tupSource. Seems like IndexAccessor translation has a bug that it does not do that. We should fix that too (and may be do all IndexAccessor fixes in a separate change?). Try giving subqueries as start index / end index exprs, to verify this. Line 1119: endIndexPair = langExprToAlgExpression(expression.getEndIndexExpression(), tupSource); should be startIndexPair.second instead of tupSource Line 1137: a.getInputs().add(expressionPair.second); the assign input should either be startIndexPair.second or endIndexPair.second if end index exists. https://asterix-gerrit.ics.uci.edu/#/c/3046/6/asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/expression/ListSliceExpression.java File asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/expression/ListSliceExpression.java: Line 19: package org.apache.asterix.lang.sqlpp.expression; Let's move it to org.apache.asterix.lang.common.expression package, because FieldAccessor and IndexAccessor are there already. (so as ListConstructor). Line 72: return 31 * super.hashCode() + Objects.hash(startIndexExpression) + Objects.hash(endIndexExpression); Why not "Objects.hash(startIndexExpression, endIndexExpression)" ? https://asterix-gerrit.ics.uci.edu/#/c/3046/6/asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/rewrites/SqlppQueryRewriter.java File asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/rewrites/SqlppQueryRewriter.java: Line 412: public Void visit(ListSliceExpression expression, Void arg) throws CompilationException { let's also fix parent GatherFunctionCallsVisitor to visit IndexAccessor's index expression https://asterix-gerrit.ics.uci.edu/#/c/3046/6/asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/visitor/CheckSql92AggregateVisitor.java File asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/visitor/CheckSql92AggregateVisitor.java: Line 128: return expression.getExpr().accept(this, parentSelectBlock); Need to visit both start and end index expressions too here. Let's fix visit(IndexAccessor) above too, so it visits it's index expression if it exists https://asterix-gerrit.ics.uci.edu/#/c/3046/6/asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/visitor/base/ISqlppVisitor.java File asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/visitor/base/ISqlppVisitor.java: Line 72: R visit(ListSliceExpression ria, T arg) throws CompilationException; Once we move ListSliceExpression to the common package this method will also need to be moved into ILangVisitor https://asterix-gerrit.ics.uci.edu/#/c/3046/6/asterixdb/asterix-lang-sqlpp/src/main/javacc/SQLPP.jj File asterixdb/asterix-lang-sqlpp/src/main/javacc/SQLPP.jj: Line 2301: LOOKAHEAD(ListSliceExpression(accessor != null ? accessor : expr)) we don't need an expensive syntactic LOOKAHEAD here if we go back to the previous version of the grammar where we extended IndexAccessor production to handle slicing. We just need to fix the issue that Ali mentioned there (I confirmed it with him). So the IndexAccessor production should become: “[” first_expr ( “:” second_expr?) ? “]” and return AbstractAccessor type. Once you see ":" set a boolean flag indicating that we need to return a ListSliceExpression. That expression may or may not have a second_expr. If we did not see ":" then return IndexAccessor -- 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: 6 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Hussain Towaileb <[email protected]> Gerrit-Reviewer: Ali Alsuliman <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Dmitry Lychagin <[email protected]> Gerrit-Reviewer: Hussain Towaileb <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-HasComments: Yes
