Alex Behm has posted comments on this change. ( )

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

Patch Set 13:

File fe/src/test/java/org/apache/impala/planner/
PS13, Line 509:   public void testCardinalityOverflow() throws ImpalaException {
Please clean up / condense the SQL to make it more readable. I pointing out 
specific improvements below.
PS13, Line 510:     String tblStr = "l_orderkey BIGINT, "
Would be great to have individual clauses as building blocks to make the CREATE 
TABLE more readable. Example:

String colDefs = "(
l_orderkey BIGINT,
l_partkey BIGINT,

String tblPropsTempate = "TBLPROPERTIES('numRows'='%s')";

String tblProps = String.format(tblPropsTemplate, 
// Stitch them together
addTestTable(String.format("CREATE TABLE ... %s %s",
 colDefs, tblProps);
clause not needed
PS13, Line 518:         + "WITH SERDEPROPERTIES ('field.delim'=' ', 
'serialization.format'='|') "
clause not needed
PS13, Line 519:         + "STORED AS TEXTFILE "
clause not needed
PS13, Line 521:         + "TBLPROPERTIES ('COLUMN_STATS_ACCURATE'='false', "
For TBLPROPERTIES, we only care about the 'numRows'. All other keys can be 
PS13, Line 524:        + tblStr + "'numFiles'='1', 'numRows'= '"
numFiles property not needed
PS13, Line 530:         + "tpch.cardinality_overflow b, 
tpch.cardinality_overflow c ";
extra space at the end
PS13, Line 541:     query = "select tpch.cardinality_overflow.l_shipmode from 
tpch.cardinality_overflow "
use unqualified column references where possible:

tpch.cardinality_overflow.l_shipmode -> l_shipmode
PS13, Line 550:         + "< b.l_orderkey";
fits in previous line?
File fe/src/test/java/org/apache/impala/planner/
PS13, Line 788:   protected void checkCardinality(String query, long min, long 
Would be great to run this for all existing planner tests list like 

Please move this function under checkLimitCardinality() so the 
refactoring/combination opportunity is visible (for future changes).
PS13, Line 799:         return;
style: indent 2
PS13, Line 805:           if(node.estimated_stats == null) {
style: space after if
PS13, Line 809:           if(cardinality < min || cardinality > max){
* in general a cardinality of -1 means "unknown" and is also valid; I think it 
makes most sense to deal with that -1 specially; we should document in the 
function comment cardinalities between min/max and -1 are valid
* style: space after if

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: 13
Gerrit-Owner: Xinran Tinney <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Jim Apple <>
Gerrit-Reviewer: Lars Volker <>
Gerrit-Reviewer: Vuk Ercegovac <>
Gerrit-Reviewer: Xinran Tinney <>
Gerrit-Comment-Date: Sat, 10 Feb 2018 01:57:49 +0000
Gerrit-HasComments: Yes

Reply via email to