Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/9139 )
Change subject: IMPALA-6228: Control stats extrapolation via tbl prop. ...................................................................... Patch Set 1: (6 comments) looks good. mostly nit comments and one clarifying question for the e2e test. http://gerrit.cloudera.org:8080/#/c/9139/1/fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java File fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java: http://gerrit.cloudera.org:8080/#/c/9139/1/fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java@549 PS1, Line 549: this nit: remove "this" http://gerrit.cloudera.org:8080/#/c/9139/1/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java: http://gerrit.cloudera.org:8080/#/c/9139/1/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@130 PS1, Line 130: // setting for a specific table. By default, tables do not have property set and rely nit: ... the property ... http://gerrit.cloudera.org:8080/#/c/9139/1/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@1971 PS1, Line 1971: * Returns true stats extrapolation is enabled for this table, false otherwise. nit: if http://gerrit.cloudera.org:8080/#/c/9139/1/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@1975 PS1, Line 1975: enableStatsExtrapolation Current name sounds like it modifies state instead of tests state. how about: isStatsExtrapolationEnabled() ? http://gerrit.cloudera.org:8080/#/c/9139/1/fe/src/test/java/org/apache/impala/planner/StatsExtrapolationTest.java File fe/src/test/java/org/apache/impala/planner/StatsExtrapolationTest.java: http://gerrit.cloudera.org:8080/#/c/9139/1/fe/src/test/java/org/apache/impala/planner/StatsExtrapolationTest.java@73 PS1, Line 73: actualExrtapNumRows nit: spelling http://gerrit.cloudera.org:8080/#/c/9139/1/tests/metadata/test_stats_extrapolation.py File tests/metadata/test_stats_extrapolation.py: http://gerrit.cloudera.org:8080/#/c/9139/1/tests/metadata/test_stats_extrapolation.py@25 PS1, Line 25: class TestStatsExtrapolation(ImpalaTestSuite): perhaps I've misread the diff, but is the impalad flag for extrapolation still tested with an e2e test with this change? -- To view, visit http://gerrit.cloudera.org:8080/9139 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie49597bf1b93b7572106abc620d91f199cba0cfd Gerrit-Change-Number: 9139 Gerrit-PatchSet: 1 Gerrit-Owner: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Comment-Date: Tue, 30 Jan 2018 18:33:15 +0000 Gerrit-HasComments: Yes
