Dmitry Lychagin has posted comments on this change. Change subject: [ASTERIXDB-2170][SQL] Fix resolution order of implicit field access ......................................................................
Patch Set 7: (9 comments) https://asterix-gerrit.ics.uci.edu/#/c/2207/6/asterixdb/asterix-app/src/test/resources/optimizerts/queries_sqlpp/count-tweets.sqlpp File asterixdb/asterix-app/src/test/resources/optimizerts/queries_sqlpp/count-tweets.sqlpp: PS6, Line 42: > Do we need "t" here? You're right. It's not necessary. Will fix https://asterix-gerrit.ics.uci.edu/#/c/2207/6/asterixdb/asterix-app/src/test/resources/optimizerts/results_parser_sqlpp/fj-dblp-csx.ast File asterixdb/asterix-app/src/test/resources/optimizerts/results_parser_sqlpp/fj-dblp-csx.ast: PS6, Line 95: > Huh? will fix https://asterix-gerrit.ics.uci.edu/#/c/2207/6/asterixdb/asterix-app/src/test/resources/parserts/queries_sqlpp/columnalias2.sqlpp File asterixdb/asterix-app/src/test/resources/parserts/queries_sqlpp/columnalias2.sqlpp: PS6, Line 20: sum > This is confusing, what did this query do before? I'm puzzled too. This query belongs to the SmokeParserTest which only parses queries and runs first chunk of the AST rewrite rules. Previously it'd run rules until (including) first invocation of VariableCheckAndRewriteVisitor. First invocation of that visitor would introduce resolve() function and ignore anything that could not be unresolved at that point. With my new change this rule fails if an identifier cannot be resolved, so the query had to be modified. https://asterix-gerrit.ics.uci.edu/#/c/2207/6/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/resolution/groupby_rename_with_sugar/groupby_rename_with_sugar.1.ddl.sqlpp File asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/resolution/groupby_rename_with_sugar/groupby_rename_with_sugar.1.ddl.sqlpp: PS6, Line 24: UNION ALL > What happened to the union_negative_2 queries? it is no longer failing so I renamed it to union_order_by_5. https://asterix-gerrit.ics.uci.edu/#/c/2207/6/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/union/union_order_by_5/union_orderby_5.3.query.sqlpp It used to fail because it would not find field 't' in the output schema of the union. However now the union is handled through the sugar rewrite: a union b union c order by x -> from ( a union b union c ) select ... order by x. So 'x' is resolved as a field access on the single variable iterating over the union. https://asterix-gerrit.ics.uci.edu/#/c/2207/6/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/tpch-with-index/q13_customer_distribution/q13_customer_distribution.3.query.sqlpp File asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/tpch-with-index/q13_customer_distribution/q13_customer_distribution.3.query.sqlpp: PS6, Line 27: COL > The support for sugars in different places seems to have changed. Do the su Sugars work in both places. I'll revert this fragment (lined 27-30) back to COLL_SUM. Line 44 no longer works though. It should either be 'count(gco)' or 'coll_count((select value gco from g))'. I'll change it to 'count(gco)' to preserve SQL-92 aggregate function as it was before https://asterix-gerrit.ics.uci.edu/#/c/2207/6/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/tpch/query-issue562/query-issue562.3.query.sqlpp File asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/tpch/query-issue562/query-issue562.3.query.sqlpp: PS6, Line 37: : : : : : > Why did this go? it's a dead code. nobody uses this variable. I can bring this back though. So the query would remain the same as before. https://asterix-gerrit.ics.uci.edu/#/c/2207/6/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/union/union_order_by_5/union_orderby_5.3.query.sqlpp File asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/union/union_order_by_5/union_orderby_5.3.query.sqlpp: PS6, Line 30: t.id, id > What do these resolve to? field access on the result of the union. (union sugar rewrite). so it'd be: from ( from ... union ... union ...) as $1 select $1.t.id order by $1.t.id, $1.id https://asterix-gerrit.ics.uci.edu/#/c/2207/6/asterixdb/asterix-app/src/test/resources/runtimets/testsuite_sqlpp.xml File asterixdb/asterix-app/src/test/resources/runtimets/testsuite_sqlpp.xml: PS6, Line 2790: function field-access-by-name > Unrelated to this change, but I think that "field-access-by-name" is nothin Agree. Although it'd require some thought. The problem is that this error is thrown by the function type check code which is generic for all functions. https://asterix-gerrit.ics.uci.edu/#/c/2207/6/asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/statement/InsertStatement.java File asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/statement/InsertStatement.java: Line 119: return null; > Sounds like a good suggestion. Is that feasible in this context? I'm not sure at this point. I'd revisit external variables approach as we see more of them in the future. May be in statement parameters, or stored procedures -- To view, visit https://asterix-gerrit.ics.uci.edu/2207 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I50bc823ff53da06507a5454b30f4f500b862d4bf Gerrit-PatchSet: 7 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: Steven Jacobs <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-HasComments: Yes
