impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Csaba Ringhofer (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-6314: Add run time scalar subquery check for uncorrelated subqueries
Date Thu, 11 Jan 2018 16:42:33 GMT
Csaba Ringhofer 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:

(8 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 needed because the
row_batch in GetNext() can reach its capacity, so the remaining fetched rows will be copied
in the next GetNext() call(s). Is this possible here? That would mean that the GetNext() was
called with an already full row batch. 
If this is not possible, then this member could be replaced with a local variable in GetNext().


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@56
PS1, Line 56:   if (child_row_batch_->num_rows() == 0) {
Is it possible/ok for this condition to be false? See my comment in scalar-check-node.h


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


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);
Just an idea, not sure if a good one: it may be better to deep copy the row, which would make
it possible to release the resources referenced by child_row_batch_ earlier.


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 replaced. This should
be mentioned in the comment.
+nit: missing .


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 replaced. This should
be mentioned in the comment.
+nit: missing .


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 .


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 .



-- 
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-Comment-Date: Thu, 11 Jan 2018 16:42:33 +0000
Gerrit-HasComments: Yes

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