You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Alex Behm (Code Review)" <ge...@cloudera.org> on 2017/11/21 08:10:08 UTC
[Impala-ASF-CR] IMPALA-5310: Add COMPUTE STATS TABLESAMPLE.
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/8136 )
Change subject: IMPALA-5310: Add COMPUTE STATS TABLESAMPLE.
......................................................................
Patch Set 1:
(44 comments)
http://gerrit.cloudera.org:8080/#/c/8136/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/8136/1//COMMIT_MSG@24
PS1, Line 24: Limitations
> add: only for HDFS tables
Done
http://gerrit.cloudera.org:8080/#/c/8136/1//COMMIT_MSG@25
PS1, Line 25: --enable_stats_extrapolation=true
> Is this really required to be a startup option, wouldn't a query option (SE
* That startup controls more than this command and is not introduced in this patch.
* It is technically not required to be a startup option, but it reduces the number of ways users can shoot themselves in the foot. For example, first running COMPUTE STATS TABLESAMPLE on a table T and then running a query against table T without stats extrapolation does not make sense and will not work well. If that was not clear to you, then it's probably not clear to users either. Yes, that can be solved with warnings etc., but preventing non-sensical combinations seems better until we have strong evidence against that conservative approach.
* This feature is strictly experimental for now, so I don't think this question needs to be addressed immediately in this patch. I filed IMPALA-6228 - we can continue the discussion there.
http://gerrit.cloudera.org:8080/#/c/8136/1/be/src/exec/catalog-op-executor.cc
File be/src/exec/catalog-op-executor.cc:
http://gerrit.cloudera.org:8080/#/c/8136/1/be/src/exec/catalog-op-executor.cc@218
PS1, Line 218: // The first column is the COUNT(*) expr of the original query.
> Can you put a similar comment above L206. I had a hard time figuring out wh
Done
http://gerrit.cloudera.org:8080/#/c/8136/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/8136/1/fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java@55
PS1, Line 55: >)]
> I was expecting to see a mention of REPEATABLE here (per change description
Done
http://gerrit.cloudera.org:8080/#/c/8136/1/fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java@58
PS1, Line 58: Existing partition-level row counts are not modified.
> Mention what happens to the partitions that don't have the row count set.
Reworded slightly to clarify.
Partition objects are not touched at all regardless of whether they have row counts or not.
http://gerrit.cloudera.org:8080/#/c/8136/1/fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java@58
PS1, Line 58: partition-level row counts are not modified.
> does this allow for the state of table vs. partition stats to be inconsiste
Yes.
Though with --enable_state_extrapolation the inconsistency is not relevant because in that mode we only rely on table-level stats and do not modify or inspect per-partition row counts. The way this works is explained in HdfsScanNode.computeCardinalities()
http://gerrit.cloudera.org:8080/#/c/8136/1/fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java@60
PS1, Line 60: unmodified
> remove
Done
http://gerrit.cloudera.org:8080/#/c/8136/1/fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java@61
PS1, Line 61: extrapolated
> Maybe add some details about how extrapolation is computed. I haven't looke
Done
http://gerrit.cloudera.org:8080/#/c/8136/1/fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java@62
PS1, Line 62: so as not to confuse other engines like Hive/SparkSQL which may rely on
: * the shared HMS fields being accurate.
> That part may be confusing to someone with no background knowledge. For ins
Fair point. Reworded to clarify. I think it's important to mention that Impala/Hive/SparkSQL interop is a goal here.
http://gerrit.cloudera.org:8080/#/c/8136/1/fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java@64
PS1, Line 64: Independently, row-count extrapolation is also done during planning based on the
: * numRows / totalSize ratio because the table contents may have changed since the
: * last time COMPUTE STATS was run.
> Remove and put it near the relevant code (planning).
Already have that in HdfsScanNode.computeCardinalities(). Removed here.
http://gerrit.cloudera.org:8080/#/c/8136/1/fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java@67
PS1, Line 67: Stats extrapolation disabled
> How would Impala behave if stats are computed with TABLESAMPLE, and then st
Impala falls back to whatever row counts are available, in this case only the table-level row counts, regardless of how many partitions are scanned.
With --enable_stats_extrapolation per-partition row counts are not computed at all.
http://gerrit.cloudera.org:8080/#/c/8136/1/fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java@77
PS1, Line 77: Not supported for now to keep the logic/code simple.
> Ha, this is the opposite in the COMPUTE STATS case.
Yea, gotta start somewhere! Probably don't want to encourage incremental stats until it's fixed.
http://gerrit.cloudera.org:8080/#/c/8136/1/fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java@106
PS1, Line 106: totalFileBytes_
> Can't you use the HdfsTable.totalFileBytes_ instead?
Yes. Done.
http://gerrit.cloudera.org:8080/#/c/8136/1/fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java@108
PS1, Line 108: Only set when
: // TABLESAMPLE was specified. Set to -1 for non-HDFS tables
> Maybe "Set to -1 for non-HDFS tables or when TABLESAMPLE is not specified"?
Done
http://gerrit.cloudera.org:8080/#/c/8136/1/fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java@305
PS1, Line 305: expectAllPartitions_ = !updateTableStatsOnly()
> So if I do not update table stats only I should expect to compute stats on
I reworked/moved this
http://gerrit.cloudera.org:8080/#/c/8136/1/fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java@344
PS1, Line 344: expectAllPartitions_ = false;
> Why is this needed? Isn't L305 sufficient?
L305 is not sufficient because you may compute incremental stats on a subset of partitions which means expectAllPartitions should be false.
http://gerrit.cloudera.org:8080/#/c/8136/1/fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java@440
PS1, Line 440: // Only add group by clause for HdfsTables.
> Comment doesn't seem relevant to the code that follows.
Done
http://gerrit.cloudera.org:8080/#/c/8136/1/fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java@442
PS1, Line 442: !updateTableStatsOnly()
> expectAllPartitions_?
That doesn't work for incremental stats on a subset of partitions.
http://gerrit.cloudera.org:8080/#/c/8136/1/fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java@452
PS1, Line 452: !updateTableStatsOnly()
> expectAllPartitions_?
That doesn't work for incremental stats on a subset of partitions.
http://gerrit.cloudera.org:8080/#/c/8136/1/fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java@506
PS1, Line 506: sets 'expectAllPartitions_' to false
> Hm, I don't see that in the code.
Stale comment. Fixed.
http://gerrit.cloudera.org:8080/#/c/8136/1/fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java@511
PS1, Line 511: base
> remove?
Done
http://gerrit.cloudera.org:8080/#/c/8136/1/fe/src/main/java/org/apache/impala/catalog/ColumnStats.java
File fe/src/main/java/org/apache/impala/catalog/ColumnStats.java:
http://gerrit.cloudera.org:8080/#/c/8136/1/fe/src/main/java/org/apache/impala/catalog/ColumnStats.java@34
PS1, Line 34: import org.apache.impala.service.BackendConfig;
> I don't think this is needed.
Done
http://gerrit.cloudera.org:8080/#/c/8136/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/8136/1/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@1877
PS1, Line 1877: public Map<Long, List<FileDescriptor>> getFilesSample(Collection<HdfsPartition> inputParts,
> nit: long line
Done
http://gerrit.cloudera.org:8080/#/c/8136/1/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
File fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java:
http://gerrit.cloudera.org:8080/#/c/8136/1/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@161
PS1, Line 161: .
> pls clarify if these should never be valid at the same time.
Done
http://gerrit.cloudera.org:8080/#/c/8136/1/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@184
PS1, Line 184: partition
> nit: partitions
Done
http://gerrit.cloudera.org:8080/#/c/8136/1/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@184
PS1, Line 184: partition
> nit: partitions
Done
http://gerrit.cloudera.org:8080/#/c/8136/1/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@185
PS1, Line 185: numPartitionsWithNumRows
> nit: alternatively numPartitionsWithRowCount_
We use numRows in a lot of places including HdfsTable and some thrift structs. Ok to defer if we really want to rename? Would be nice to be consistent everywhere.
http://gerrit.cloudera.org:8080/#/c/8136/1/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@777
PS1, Line 777: if (totalBytes_ == 0) {
> totalBytes_ doesn't get a new value in the above section, why not make this
The goal is to always call getStatsNumRows() to collect stats info reported in the explain plan.
Added comment and reworked code.
http://gerrit.cloudera.org:8080/#/c/8136/1/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@780
PS1, Line 780: cardinality_
> it would be clearer if this is named outputCardinality_ (the comment for th
This is a big rename in several places because cardinality_ comes from PlanNode and is set/used in all plan nodes.
Ok to defer?
http://gerrit.cloudera.org:8080/#/c/8136/1/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@830
PS1, Line 830: getNumRows
> used at 4 call sites in this for-loop. assign it to a variable.
Good point. Done.
http://gerrit.cloudera.org:8080/#/c/8136/1/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@1122
PS1, Line 1122: tbl_.getNumClusteringCols() > 0
> this is used in several places-- it would be clearer if it was wrapped in s
Trying to minimize surface area of this already big patch. Ok to defer? Like you said, affects many places.
http://gerrit.cloudera.org:8080/#/c/8136/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:
http://gerrit.cloudera.org:8080/#/c/8136/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@691
PS1, Line 691: trace
> I wonder if trace is the right logging level. Maybe info? It seems kind of
Makes sense. I slightly improved the msg also.
http://gerrit.cloudera.org:8080/#/c/8136/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@699
PS1, Line 699: try {
> Do we need the nested try here?
Nope. Done.
http://gerrit.cloudera.org:8080/#/c/8136/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@735
PS1, Line 735: numUpdatedColumns.setRef(Long.valueOf(0));
: if (colStats != null) {
: numUpdatedColumns.setRef(Long.valueOf(colStats.getStatsObjSize()));
: }
> Move after L705?
Made the changes. My intention was to update the return values in the same place with similarly looking code. Works for me either way.
http://gerrit.cloudera.org:8080/#/c/8136/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@744
PS1, Line 744: Impala
> got confused here since previous line mentions Hive and Impala tables (why
Updated comment since the HMS table is not actually updated in this function.
There's no fundamental reason for us to work with Impala partitions and not HMS partitions. HMS partitions tend to be unwieldy due to the long class name.
Would you prefer to modify and return HMS partitions instead?
http://gerrit.cloudera.org:8080/#/c/8136/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@795
PS1, Line 795: Hive table
> given previous comment referencing Hive and Impala tables, I'm unclear abou
We're updating the Hive table because that's what needs to be sent in the alterTable() RPC to the HMS.
For the partitions case above we ultimately also send an HMS partition in the alterPartition() RPC.
The main difference is that for partitions we have a HdfsPartition.toHmsPartition() function whereas there is no table equivalent due to the trickness of that conversion.
If you prefer, we can switch updatePartitionStats() to modify and return HMS partitions. Then updateTableStats() and updatePartitionStats() are at least somewhat consistent.
I think we should not change updateTabeStats() to work on an Impala table and then introduce a toHmsTable() because creating completely correct HMS table metadata is hard and error prone and we might break something.
http://gerrit.cloudera.org:8080/#/c/8136/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@857
PS1, Line 857: sampleFileBytes
> Could that be 0?
Yes. Added Preconditions check.
http://gerrit.cloudera.org:8080/#/c/8136/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@860
PS1, Line 860: totalFileBytes / sampleFileBytes
> is returned result guaranteed to be >= val?
I think so. Added comment.
http://gerrit.cloudera.org:8080/#/c/8136/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@862
PS1, Line 862: Long.MAX_VALUE
> Hm, that's a really big number :) Are you sure about that?
This means the multiplication above overflowed. In other places we also cap at MAX_VALUE when overflowing cardinality/rows calculations. Do you have an alternative?
Simplified this code (round() already does that check).
http://gerrit.cloudera.org:8080/#/c/8136/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@866
PS1, Line 866: private static ColumnStatisticsData createHiveColStatsData(TAlterTableUpdateStatsParams params,
> nit: long line
Done
http://gerrit.cloudera.org:8080/#/c/8136/1/fe/src/test/java/org/apache/impala/hive/executor/UdfExecutorTest.java
File fe/src/test/java/org/apache/impala/hive/executor/UdfExecutorTest.java:
http://gerrit.cloudera.org:8080/#/c/8136/1/fe/src/test/java/org/apache/impala/hive/executor/UdfExecutorTest.java@74
PS1, Line 74: import com.google.common.base.Joiner;
> Is it needed?
It is now after rebasing
http://gerrit.cloudera.org:8080/#/c/8136/1/testdata/workloads/functional-planner/queries/PlannerTest/max-row-size.test
File testdata/workloads/functional-planner/queries/PlannerTest/max-row-size.test:
http://gerrit.cloudera.org:8080/#/c/8136/1/testdata/workloads/functional-planner/queries/PlannerTest/max-row-size.test@37
PS1, Line 37: 0
> Is this correct?
No. Updated.
http://gerrit.cloudera.org:8080/#/c/8136/1/testdata/workloads/functional-planner/queries/PlannerTest/resource-requirements.test
File testdata/workloads/functional-planner/queries/PlannerTest/resource-requirements.test:
http://gerrit.cloudera.org:8080/#/c/8136/1/testdata/workloads/functional-planner/queries/PlannerTest/resource-requirements.test@38
PS1, Line 38: 71
> There are a bunch of second digit changes in most of the tests. Are these o
The size comparison is ignored in tests because there are always every-so-slight variances depending on the version of Hive/Impala or similar things.
http://gerrit.cloudera.org:8080/#/c/8136/1/tests/metadata/test_explain.py
File tests/metadata/test_explain.py:
http://gerrit.cloudera.org:8080/#/c/8136/1/tests/metadata/test_explain.py@131
PS1, Line 131:
> nit: extra space
Done
--
To view, visit http://gerrit.cloudera.org:8080/8136
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7f3e72471ac563adada4a4156033a85852b7c8b7
Gerrit-Change-Number: 8136
Gerrit-PatchSet: 1
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Balazs Jeszenszky <je...@gmail.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 21 Nov 2017 08:10:08 +0000
Gerrit-HasComments: Yes