impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Zoltan Borok-Nagy (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-6314: Add run time scalar subquery check for uncorrelated subqueries
Date Fri, 12 Jan 2018 16:36:14 GMT
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/9005
)

Change subject: IMPALA-6314: Add run time scalar subquery check for uncorrelated subqueries
......................................................................


Patch Set 1:

(15 comments)

Thanks for all the comments!

http://gerrit.cloudera.org:8080/#/c/9005/1/be/src/exec/scalar-check-node.h
File be/src/exec/scalar-check-node.h:

http://gerrit.cloudera.org:8080/#/c/9005/1/be/src/exec/scalar-check-node.h@44
PS1, Line 44:   boost::scoped_ptr<RowBatch> child_row_batch_;
> If I understand correctly, having child_row_batch_ as member is usually nee
You are right, but I still keep it because I've changed the implementation and now we need
to have a member. I renamed it to scalar_row_batch_ btw.

ScalarCheckNode is now a blocking node and deep-copies its child's row in Open(), then passes
it to its parent in GetNext().


http://gerrit.cloudera.org:8080/#/c/9005/1/be/src/exec/scalar-check-node.h@47
PS1, Line 47: child_row_idx_
> This member is not used anywhere. Probably it should be removed.
Done


http://gerrit.cloudera.org:8080/#/c/9005/1/be/src/exec/scalar-check-node.cc
File be/src/exec/scalar-check-node.cc:

http://gerrit.cloudera.org:8080/#/c/9005/1/be/src/exec/scalar-check-node.cc@31
PS1, Line 31: NULL
> Prefer nullptr in new code.
Removed it from here since it is a boost::scoped_ptr with a default constructor. But maybe
I can add "= nullptr" in the class definition to make the initialization explicit.


http://gerrit.cloudera.org:8080/#/c/9005/1/be/src/exec/scalar-check-node.cc@32
PS1, Line 32:       child_row_idx_(0),
> We've mostly been switching to initialising member variables to constants i
Removed those members.


http://gerrit.cloudera.org:8080/#/c/9005/1/be/src/exec/scalar-check-node.cc@56
PS1, Line 56:   if (child_row_batch_->num_rows() == 0) {
> I think this needs to be a while loop that resets the batch after each iter
I followed this logic and moved this part to Open() since now it is a blocking node and we
want to call Close() on the child asap.


http://gerrit.cloudera.org:8080/#/c/9005/1/be/src/exec/scalar-check-node.cc@63
PS1, Line 63: one
> "zero or one" or "maximum one" would be more exact
Done


http://gerrit.cloudera.org:8080/#/c/9005/1/be/src/exec/scalar-check-node.cc@78
PS1, Line 78:   child_row_batch_->TransferResourceOwnership(row_batch);
> I think it's the start of a good idea. I originally suggested modelling thi
Thanks for the idea, I followed this logic. So I deep copy the child's row in Open(), or raise
an error if there are more rows. Also, I close the child right after reading its row. Then,
deep copy the single scalar row in GetNext() into the parent's row batch.


http://gerrit.cloudera.org:8080/#/c/9005/1/fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java
File fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java:

http://gerrit.cloudera.org:8080/#/c/9005/1/fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java@178
PS1, Line 178:           // Runtime check needed to check if subquery returns scalar
> This is not just a check, but can be a transformation, as array types are r
Replaced the comment to make it more informative.


http://gerrit.cloudera.org:8080/#/c/9005/1/fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java
File fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java:

http://gerrit.cloudera.org:8080/#/c/9005/1/fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java@179
PS1, Line 179:           // Runtime check needed to check if subquery returns scalar
> This is not just a check, but can be a transformation, as array types are r
Replaced the comment.


http://gerrit.cloudera.org:8080/#/c/9005/1/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java
File fe/src/main/java/org/apache/impala/analysis/QueryStmt.java:

http://gerrit.cloudera.org:8080/#/c/9005/1/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java@90
PS1, Line 90:   // If true, a ScalarCheckNode is needed to check this statement's result at
runtime
> nit: missing .
Done


http://gerrit.cloudera.org:8080/#/c/9005/1/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java
File fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java:

http://gerrit.cloudera.org:8080/#/c/9005/1/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java@735
PS1, Line 735:    * @throws ImpalaException
> We don't generally have @throws declarations in Java comments
Oops, eclipse added it automagically.


http://gerrit.cloudera.org:8080/#/c/9005/1/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java@746
PS1, Line 746: selectNode
> scalarCheckNode?
Yes!


http://gerrit.cloudera.org:8080/#/c/9005/1/fe/src/main/java/org/apache/impala/planner/ScalarCheckNode.java
File fe/src/main/java/org/apache/impala/planner/ScalarCheckNode.java:

http://gerrit.cloudera.org:8080/#/c/9005/1/fe/src/main/java/org/apache/impala/planner/ScalarCheckNode.java@32
PS1, Line 32:     // LIMIT 2 is enough to check if the child returns a scalar value
            :     // Also, an optimized plan is generated with LIMIT 2
> nit: 2 missing .
Done


http://gerrit.cloudera.org:8080/#/c/9005/1/testdata/workloads/functional-query/queries/QueryTest/subquery.test
File testdata/workloads/functional-query/queries/QueryTest/subquery.test:

http://gerrit.cloudera.org:8080/#/c/9005/1/testdata/workloads/functional-query/queries/QueryTest/subquery.test@839
PS1, Line 839: ---- QUERY
> Maybe add a test that sets num_nodes=1 to make sure that the single-node ve
Done


http://gerrit.cloudera.org:8080/#/c/9005/1/testdata/workloads/functional-query/queries/QueryTest/subquery.test@932
PS1, Line 932: ====
> The case when the subquery returns 0 row could be tested too.
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/9005
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0f52b93a60eeacedd242a2f17fa6b99c4fc38e06
Gerrit-Change-Number: 9005
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy <boroknagyz@cloudera.com>
Gerrit-Reviewer: Attila Jeges <attilaj@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <csringhofer@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <boroknagyz@cloudera.com>
Gerrit-Comment-Date: Fri, 12 Jan 2018 16:36:14 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message