Dmitry Lychagin has submitted this change and it was merged. Change subject: [ASTERIXDB-2248][SQLPP] Disallow use of column aliases in GBY/HAVING ......................................................................
[ASTERIXDB-2248][SQLPP] Disallow use of column aliases in GBY/HAVING - user model changes: yes - storage format changes: no - interface changes: no Details: - Column aliases defined by SELECT clause should not be referenceable from GROUP BY/HAVING clauses because aliases are not variables. References from ORDER BY clause are still allowed. Change-Id: Ieb734f87904e6bf307a04b2328a2fd109151f70f Reviewed-on: https://asterix-gerrit.ics.uci.edu/2294 Sonar-Qube: Jenkins <[email protected]> Tested-by: Jenkins <[email protected]> Integration-Tests: Jenkins <[email protected]> Reviewed-by: Till Westmann <[email protected]> Contrib: Till Westmann <[email protected]> --- M asterixdb/asterix-app/src/test/resources/parserts/queries_sqlpp/columnalias.sqlpp M asterixdb/asterix-app/src/test/resources/parserts/queries_sqlpp/columnalias3.sqlpp M asterixdb/asterix-app/src/test/resources/parserts/results_parser_sqlpp/columnalias.ast M asterixdb/asterix-app/src/test/resources/parserts/results_parser_sqlpp/columnalias3.ast M asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/tpcds/q24a/q24a.3.query.sqlpp M asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/tpcds/q24b/q24b.3.query.sqlpp M asterixdb/asterix-app/src/test/resources/runtimets/testsuite_sqlpp.xml M asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/rewrites/visitor/InlineColumnAliasVisitor.java 8 files changed, 130 insertions(+), 73 deletions(-) Approvals: Till Westmann: Looks good to me, approved; Jenkins: Verified; No violations found; Verified Objections: Anon. E. Moose #1000171: Violations found diff --git a/asterixdb/asterix-app/src/test/resources/parserts/queries_sqlpp/columnalias.sqlpp b/asterixdb/asterix-app/src/test/resources/parserts/queries_sqlpp/columnalias.sqlpp index 72eb1d9..9d1af80 100644 --- a/asterixdb/asterix-app/src/test/resources/parserts/queries_sqlpp/columnalias.sqlpp +++ b/asterixdb/asterix-app/src/test/resources/parserts/queries_sqlpp/columnalias.sqlpp @@ -17,8 +17,6 @@ * under the License. */ -SELECT SQRT(t.a*t.b) AS root FROM tbl_name t -GROUP BY root -WITH u AS root -HAVING root > 0 -ORDER BY u; \ No newline at end of file +SELECT sum(t.a*t.b) AS root FROM tbl_name t +GROUP BY t.id +ORDER BY root; \ No newline at end of file diff --git a/asterixdb/asterix-app/src/test/resources/parserts/queries_sqlpp/columnalias3.sqlpp b/asterixdb/asterix-app/src/test/resources/parserts/queries_sqlpp/columnalias3.sqlpp index 8226362..d95b42d 100644 --- a/asterixdb/asterix-app/src/test/resources/parserts/queries_sqlpp/columnalias3.sqlpp +++ b/asterixdb/asterix-app/src/test/resources/parserts/queries_sqlpp/columnalias3.sqlpp @@ -17,8 +17,6 @@ * under the License. */ -SELECT ELEMENT {'root': SQRT(t.a*t.b)} FROM tbl_name t -GROUP BY root -WITH u AS root -HAVING root > 0 -ORDER BY u; \ No newline at end of file +SELECT ELEMENT {'root': SUM(t.a*t.b)} FROM tbl_name t +GROUP BY t.id +ORDER BY root; \ No newline at end of file diff --git a/asterixdb/asterix-app/src/test/resources/parserts/results_parser_sqlpp/columnalias.ast b/asterixdb/asterix-app/src/test/resources/parserts/results_parser_sqlpp/columnalias.ast index 9c5aaf5..1e0efba 100644 --- a/asterixdb/asterix-app/src/test/resources/parserts/results_parser_sqlpp/columnalias.ast +++ b/asterixdb/asterix-app/src/test/resources/parserts/results_parser_sqlpp/columnalias.ast @@ -1,6 +1,31 @@ Query: SELECT [ -Variable [ Name=$root ] +FunctionCall asterix.sql-sum@1[ + ( + SELECT ELEMENT [ + OperatorExpr [ + FieldAccessor [ + FieldAccessor [ + Variable [ Name=#3 ] + Field=t + ] + Field=a + ] + * + FieldAccessor [ + FieldAccessor [ + Variable [ Name=#3 ] + Field=t + ] + Field=b + ] + ] + ] + FROM [ Variable [ Name=#1 ] + AS Variable [ Name=#3 ] + ] + ) +] root ] FROM [ FunctionCall asterix.dataset@1[ @@ -9,36 +34,43 @@ AS Variable [ Name=$t ] ] Groupby - Variable [ Name=$root ] + Variable [ Name=$id ] := - FunctionCall null.sqrt@1[ - OperatorExpr [ - FieldAccessor [ - Variable [ Name=$t ] - Field=a - ] - * - FieldAccessor [ - Variable [ Name=$t ] - Field=b - ] - ] + FieldAccessor [ + Variable [ Name=$t ] + Field=id ] GROUP AS Variable [ Name=#1 ] ( t:=Variable [ Name=$t ] ) -Let Variable [ Name=$u ] - := - Variable [ Name=$root ] - HAVING - OperatorExpr [ - Variable [ Name=$root ] - > - LiteralExpr [LONG] [0] - ] Orderby - Variable [ Name=$u ] + FunctionCall asterix.sql-sum@1[ + ( + SELECT ELEMENT [ + OperatorExpr [ + FieldAccessor [ + FieldAccessor [ + Variable [ Name=#2 ] + Field=t + ] + Field=a + ] + * + FieldAccessor [ + FieldAccessor [ + Variable [ Name=#2 ] + Field=t + ] + Field=b + ] + ] + ] + FROM [ Variable [ Name=#1 ] + AS Variable [ Name=#2 ] + ] + ) + ] ASC diff --git a/asterixdb/asterix-app/src/test/resources/parserts/results_parser_sqlpp/columnalias3.ast b/asterixdb/asterix-app/src/test/resources/parserts/results_parser_sqlpp/columnalias3.ast index 6db92c9..7deb117 100644 --- a/asterixdb/asterix-app/src/test/resources/parserts/results_parser_sqlpp/columnalias3.ast +++ b/asterixdb/asterix-app/src/test/resources/parserts/results_parser_sqlpp/columnalias3.ast @@ -4,7 +4,32 @@ ( LiteralExpr [STRING] [root] : - Variable [ Name=$root ] + FunctionCall asterix.sql-sum@1[ + ( + SELECT ELEMENT [ + OperatorExpr [ + FieldAccessor [ + FieldAccessor [ + Variable [ Name=#3 ] + Field=t + ] + Field=a + ] + * + FieldAccessor [ + FieldAccessor [ + Variable [ Name=#3 ] + Field=t + ] + Field=b + ] + ] + ] + FROM [ Variable [ Name=#1 ] + AS Variable [ Name=#3 ] + ] + ) + ] ) ] ] @@ -14,36 +39,43 @@ AS Variable [ Name=$t ] ] Groupby - Variable [ Name=$root ] + Variable [ Name=$id ] := - FunctionCall null.sqrt@1[ - OperatorExpr [ - FieldAccessor [ - Variable [ Name=$t ] - Field=a - ] - * - FieldAccessor [ - Variable [ Name=$t ] - Field=b - ] - ] + FieldAccessor [ + Variable [ Name=$t ] + Field=id ] GROUP AS Variable [ Name=#1 ] ( t:=Variable [ Name=$t ] ) -Let Variable [ Name=$u ] - := - Variable [ Name=$root ] - HAVING - OperatorExpr [ - Variable [ Name=$root ] - > - LiteralExpr [LONG] [0] - ] Orderby - Variable [ Name=$u ] + FunctionCall asterix.sql-sum@1[ + ( + SELECT ELEMENT [ + OperatorExpr [ + FieldAccessor [ + FieldAccessor [ + Variable [ Name=#2 ] + Field=t + ] + Field=a + ] + * + FieldAccessor [ + FieldAccessor [ + Variable [ Name=#2 ] + Field=t + ] + Field=b + ] + ] + ] + FROM [ Variable [ Name=#1 ] + AS Variable [ Name=#2 ] + ] + ) + ] ASC diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/tpcds/q24a/q24a.3.query.sqlpp b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/tpcds/q24a/q24a.3.query.sqlpp index c24c10e..95e4e8c 100644 --- a/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/tpcds/q24a/q24a.3.query.sqlpp +++ b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/tpcds/q24a/q24a.3.query.sqlpp @@ -59,12 +59,13 @@ SELECT c_last_name ,c_first_name ,s_store_name - ,SUM(netpaid) paid + ,paid FROM ssales WHERE i_color = 'orchid' GROUP BY c_last_name ,c_first_name ,s_store_name GROUP AS g +LET paid = SUM(netpaid) HAVING paid > (SELECT value 0.05*avg(g.ssales.netpaid) FROM g)[0] ; \ No newline at end of file diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/tpcds/q24b/q24b.3.query.sqlpp b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/tpcds/q24b/q24b.3.query.sqlpp index 86ffa7b..3a1590d 100644 --- a/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/tpcds/q24b/q24b.3.query.sqlpp +++ b/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/tpcds/q24b/q24b.3.query.sqlpp @@ -59,12 +59,13 @@ SELECT c_last_name ,c_first_name ,s_store_name - ,SUM(netpaid) paid + ,paid FROM ssales WHERE i_color = 'chiffon' GROUP BY c_last_name ,c_first_name ,s_store_name GROUP AS g +LET paid = SUM(netpaid) HAVING paid > (SELECT value 0.05*avg(g.ssales.netpaid) FROM g)[0] ; \ No newline at end of file diff --git a/asterixdb/asterix-app/src/test/resources/runtimets/testsuite_sqlpp.xml b/asterixdb/asterix-app/src/test/resources/runtimets/testsuite_sqlpp.xml index 0b8fd8c..16fa334 100644 --- a/asterixdb/asterix-app/src/test/resources/runtimets/testsuite_sqlpp.xml +++ b/asterixdb/asterix-app/src/test/resources/runtimets/testsuite_sqlpp.xml @@ -1563,6 +1563,7 @@ <test-case FilePath="dapd"> <compilation-unit name="q2-3"> <output-dir compare="Text">q2</output-dir> + <expected-error>Cannot resolve ambiguous alias reference for undefined identifier sig_id</expected-error> </compilation-unit> </test-case> <test-case FilePath="dapd"> @@ -2901,11 +2902,13 @@ <test-case FilePath="group-by"> <compilation-unit name="sugar-02"> <output-dir compare="Text">core-02</output-dir> + <expected-error>Cannot resolve ambiguous alias reference for undefined identifier deptId</expected-error> </compilation-unit> </test-case> <test-case FilePath="group-by"> <compilation-unit name="sugar-02-2"> <output-dir compare="Text">core-02</output-dir> + <expected-error>Cannot resolve ambiguous alias reference for undefined identifier deptId</expected-error> </compilation-unit> </test-case> <test-case FilePath="group-by"> diff --git a/asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/rewrites/visitor/InlineColumnAliasVisitor.java b/asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/rewrites/visitor/InlineColumnAliasVisitor.java index 628bf57..270b366 100644 --- a/asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/rewrites/visitor/InlineColumnAliasVisitor.java +++ b/asterixdb/asterix-lang-sqlpp/src/main/java/org/apache/asterix/lang/sqlpp/rewrites/visitor/InlineColumnAliasVisitor.java @@ -28,7 +28,6 @@ import org.apache.asterix.lang.common.base.Expression.Kind; import org.apache.asterix.lang.common.base.ILangExpression; import org.apache.asterix.lang.common.base.Literal; -import org.apache.asterix.lang.common.clause.LetClause; import org.apache.asterix.lang.common.expression.FieldBinding; import org.apache.asterix.lang.common.expression.LiteralExpr; import org.apache.asterix.lang.common.expression.RecordConstructor; @@ -45,6 +44,11 @@ import org.apache.asterix.lang.sqlpp.visitor.SqlppSubstituteExpressionVisitor; import org.apache.asterix.lang.sqlpp.visitor.base.AbstractSqlppExpressionScopingVisitor; +/** + * Syntactic sugar rewriting: inlines column aliases defines in SELECT clause into ORDER BY and LIMIT clauses. <br/> + * Note: column aliases are not cosidered new variables, but they can be referenced from ORDER BY and LIMIT clauses + * because of this rewriting (like in SQL) + */ public class InlineColumnAliasVisitor extends AbstractSqlppExpressionScopingVisitor { public InlineColumnAliasVisitor(LangRewritingContext context) { @@ -67,18 +71,6 @@ // Creates a substitution visitor. SqlppSubstituteExpressionVisitor visitor = new SqlppSubstituteExpressionVisitor(context, map); - // Rewrites GROUP BY/LET/HAVING clauses. - if (selectBlock.hasGroupbyClause()) { - selectBlock.getGroupbyClause().accept(visitor, arg); - } - if (selectBlock.hasLetClausesAfterGroupby()) { - for (LetClause letClause : selectBlock.getLetListAfterGroupby()) { - letClause.accept(visitor, arg); - } - } - if (selectBlock.hasHavingClause()) { - selectBlock.getHavingClause().accept(visitor, arg); - } SelectExpression selectExpression = (SelectExpression) arg; // For SET operation queries, column aliases will not substitute ORDER BY nor LIMIT expressions. -- To view, visit https://asterix-gerrit.ics.uci.edu/2294 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: merged Gerrit-Change-Id: Ieb734f87904e6bf307a04b2328a2fd109151f70f Gerrit-PatchSet: 3 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Dmitry Lychagin <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Dmitry Lychagin <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Michael Blow <[email protected]> Gerrit-Reviewer: Murtadha Hubail <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-Reviewer: abdullah alamoudi <[email protected]>
