Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/17306 )
Change subject: IMPALA-10445: Adjust NDV's scale with query option ...................................................................... Patch Set 15: Code-Review+1 (5 comments) http://gerrit.cloudera.org:8080/#/c/17306/15/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java: http://gerrit.cloudera.org:8080/#/c/17306/15/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java@181 PS15, Line 181: nit: redudant space http://gerrit.cloudera.org:8080/#/c/17306/15/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java@184 PS15, Line 184: should nit: "callers should" http://gerrit.cloudera.org:8080/#/c/17306/15/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java@188 PS15, Line 188: * expStr: origin expr : * rules: list of rewrite rules : * expectedExprStr: expected expr : * return: Expr nit: our code style use param comments like these: /** * .... * * @param exprStr: original expr * @param rules: list of rewrite rules * @param expectedExprStr: expected expr * @return the rewritten Expr */ http://gerrit.cloudera.org:8080/#/c/17306/15/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java@794 PS15, Line 794: session.options().setAppx_count_distinct(true); : RewritesOk("count(distinct bool_col)", rules, "ndv(bool_col)"); : } : : @Test : public void testDefaultNdvScaleRule() throws ImpalaException { : List<ExprRewriteRule> rules = Lists.newArrayList( : org.apache.impala.rewrite.DefaultNdvScaleRule.INSTANCE : ); : session.options().setDefault_ndv_scale(10); : RewritesOk("ndv(bool_col)", rules, "ndv(bool_col, 10)"); : } : : @Test : public void testDefaultNdvScaleRuleNotSet() throws ImpalaException { : List<ExprRewriteRule> rules = Lists.newArrayList( : org.apache.impala.rewrite.DefaultNdvScaleRule.INSTANCE : ); : RewritesOk("ndv(bool_col)", rules, null); : } : : @Test : public void testDefaultNdvScaleRuleSetDefault() throws ImpalaException { : List<ExprRewriteRule> rules = Lists.newArrayList( : org.apache.impala.rewrite.DefaultNdvScaleRule.INSTANCE : ); : session.options().setDefault_ndv_scale(2); : RewritesOk("ndv(bool_col)", rules, null); nit: Just like other tests in this file, you can merge these into one test method since they share the same rule. http://gerrit.cloudera.org:8080/#/c/17306/15/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java@822 PS15, Line 822: nit: redundant blank line -- To view, visit http://gerrit.cloudera.org:8080/17306 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1669858a6e8252e167b464586e8d0b6cb0d0bd50 Gerrit-Change-Number: 17306 Gerrit-PatchSet: 15 Gerrit-Owner: fifteencai <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Qifan Chen <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: fifteencai <[email protected]> Gerrit-Comment-Date: Thu, 15 Apr 2021 13:08:20 +0000 Gerrit-HasComments: Yes
