Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/12248 )
Change subject: IMPALA-8095: Detailed expression cardinality tests ...................................................................... Patch Set 5: (5 comments) Just some clarifying questions. Overall makes sense to me. Since this is test-only code, we can merge this and keep improving it as and when we find new bugs or need more fixture specific improvements for writing new tests. 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: See : * also {@link ExprNdvTest}, {@link CardinalityTest}. There seems to be overlap in all of these. Should we consolidate them? Others might find it confusing while figuring out where to add new tests. http://gerrit.cloudera.org:8080/#/c/12248/5/fe/src/test/java/org/apache/impala/analysis/ExprCardinalityTest.java@110 PS5, Line 110: Bug Does it make sense to tag the jiras for all the bugs here? I know you raised a bunch of them, probably we should maintain an epic (cardinality mis-estimates or something) and link them there. Many of them make good fe ramp-up tasks :-) ** I know that is a lot of work, don't want that to block this patch, just some thought. http://gerrit.cloudera.org:8080/#/c/12248/5/fe/src/test/java/org/apache/impala/analysis/ExprCardinalityTest.java@111 PS5, Line 111: // 2 in the shell with SHOW COLUMN STATS alltypes : //verifyTableCol(allTypes, "year", 2, 0); : // Bug: Unit test in Eclipse see the above, unit tests run from the : // command line see the below. Disabling to avoid a flaky test, : // here and below Not super clear what is happening here, clarify may be? 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. 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 by a temporary HMS created from scratch and we totally discard the HMS instance after test. That way we don't need to fake all the HMS table structures like in the methods below. -- 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: 5 Gerrit-Owner: Paul Rogers <prog...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Paul Rogers <prog...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Fri, 25 Jan 2019 19:25:44 +0000 Gerrit-HasComments: Yes
