Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/12248 )
Change subject: IMPALA-8095: Detailed expression cardinality tests ...................................................................... Patch Set 7: (5 comments) Thanks for the review Bharath. Addressed your comments. Also rebased on to latest master. Please take another look at your convenience. http://gerrit.cloudera.org:8080/#/c/12248/5/fe/src/test/java/org/apache/impala/analysis/ExprCardinalityTest.java File fe/src/test/java/org/apache/impala/analysis/ExprCardinalityTest.java: http://gerrit.cloudera.org:8080/#/c/12248/5/fe/src/test/java/org/apache/impala/analysis/ExprCardinalityTest.java@41 PS5, Line 41: : * > There seems to be overlap in all of these. Should we consolidate them? Othe Added comments to explain how the test classes relate to one another. http://gerrit.cloudera.org:8080/#/c/12248/5/fe/src/test/java/org/apache/impala/analysis/ExprCardinalityTest.java@110 PS5, Line 110: <Ta > Does it make sense to tag the jiras for all the bugs here? I know you raise Done http://gerrit.cloudera.org:8080/#/c/12248/5/fe/src/test/java/org/apache/impala/analysis/ExprCardinalityTest.java@111 PS5, Line 111: new TableName("functional", "alltypes"), : new TableName("functional", "nullrows"), : new TableName("functional", "manynulls")); : mdLoader.loadTables(tables); : > Not super clear what is happening here, clarify may be? Done http://gerrit.cloudera.org:8080/#/c/12248/5/fe/src/test/java/org/apache/impala/analysis/ExprNdvTest.java File fe/src/test/java/org/apache/impala/analysis/ExprNdvTest.java: http://gerrit.cloudera.org:8080/#/c/12248/5/fe/src/test/java/org/apache/impala/analysis/ExprNdvTest.java@164 PS5, Line 164: After IMPALA-7659, Impala computes a null count, : * when gathering stats, but the NDV does not include nulls (except for Boolean : * columns) if stats are computed by Impala, but does include nulls if stats are : * computed by Hive. > Haha, this is indeed bizarre. Added bug number and fixed typos. http://gerrit.cloudera.org:8080/#/c/12248/5/fe/src/test/java/org/apache/impala/common/FrontendFixture.java File fe/src/test/java/org/apache/impala/common/FrontendFixture.java: http://gerrit.cloudera.org:8080/#/c/12248/5/fe/src/test/java/org/apache/impala/common/FrontendFixture.java@164 PS5, Line 164: protected void clearTestDbs() { : for (Db testDb: testDbs_) { : catalog_.removeDb(testDb.getName()); : } : } > I did something similar in the testcase patch, except that this is backed b Great. Once both patches are in, we can come back and unify the approach. Will be very helpful to be able to easily create test tables for planner tests without having to create real tables in the functional DB. -- To view, visit http://gerrit.cloudera.org:8080/12248 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3da58ee9b0beebeffb170b9430bd36d20dcd2401 Gerrit-Change-Number: 12248 Gerrit-PatchSet: 7 Gerrit-Owner: Paul Rogers <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Paul Rogers <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Thu, 07 Feb 2019 01:27:55 +0000 Gerrit-HasComments: Yes
