asterixdb-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dmitry Lychagin (Code Review)" <do-not-re...@asterixdb.incubator.apache.org>
Subject Change in asterixdb[master]: [ASTERIXDB-2170][SQL] Fix resolution order of implicit field...
Date Wed, 13 Dec 2017 01:38:53 GMT
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 <dmitry.lychagin@couchbase.com>
Gerrit-Reviewer: Anon. E. Moose #1000171
Gerrit-Reviewer: Dmitry Lychagin <dmitry.lychagin@couchbase.com>
Gerrit-Reviewer: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Steven Jacobs <sjaco002@ucr.edu>
Gerrit-Reviewer: Till Westmann <tillw@apache.org>
Gerrit-HasComments: Yes

Mime
View raw message