Vuk Ercegovac has posted comments on this change. ( )

Change subject: IMPALA-5440 Add planner tests with extreme statistics values

Patch Set 12:


couple of proposals for simplifying code/comments. otherwise, looks fine.
File fe/src/test/java/org/apache/impala/planner/
PS12, Line 527: two
nit: you have three tables in the query.
PS12, Line 527: when the two tables both have Long.MAX_VALUE as
              :     // the row number, the CROSS JOIN should have 
              :     // and this tests for the cardinality estimate overflow
perhaps the following template is easier? if you think its better, pls use it 
for the others.

"tests that multiplying the input cardinalities does not overflow the 
cross-join's estimated cardinality."

you can replace "cross-join " and "multiply" with add where applicable.
PS12, Line 550:     // row number, the JOIN (especially unequal JOIN) should 
have >Long.MAX_VALUE rows
nit: space
PS12, Line 564: rows
nit: row count
File fe/src/test/java/org/apache/impala/planner/
PS12, Line 783: the query
replace with: 'query'

(to refer to the parameter)
PS12, Line 784:    * fails if the cardinality is not within the specified 
add: ['min', 'max']
PS12, Line 785: There are three parameters: query is the request that estimated
              :    * cardinality is calculated based on and min/max indicates 
the bounds
              :    * of the cardinality.
remove this and inline it in the decscription above.
PS12, Line 793: TExecRequest execRequest
move the declaration and assignment to L796 (no need to split this over two 
PS12, Line 798: execRequest == null ||
from my reading of createExecRequest, this will never be null, so remove this 
PS12, Line 825: protected void checkCardinalityBounds(
after the simplifications, I think it makes sense to inline this function at 
L807. at this point, its complicating this more than needed (extra repeated 
comment, awkward parameters) for a basic bounds check and error message.

To view, visit
To unsubscribe, visit

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I86dec47cf1438882cafaec53e97864ccfcdff3cb
Gerrit-Change-Number: 9065
Gerrit-PatchSet: 12
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Jim Apple <>
Gerrit-Reviewer: Lars Volker <>
Gerrit-Reviewer: Vuk Ercegovac <>
Gerrit-Comment-Date: Fri, 09 Feb 2018 17:22:16 +0000
Gerrit-HasComments: Yes

Reply via email to