Conversation
27fc967 to
d0e8fea
Compare
d0e8fea to
be100ec
Compare
be100ec to
efa65c1
Compare
efa65c1 to
8228b0b
Compare
8228b0b to
14823b9
Compare
14823b9 to
aa1cfb7
Compare
a7fa8b2 to
4173058
Compare
4173058 to
27b2198
Compare
27b2198 to
fc3051a
Compare
fc3051a to
9fb4799
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Four unresolved review findings include two critical correctness issues and two moderate issues.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR enables Iceberg-backed MIN, MAX, and COUNT answers using snapshot-aware column and row statistics.
Changes:
- Adds handler APIs for freshness, row counts, and aggregate statistics.
- Extends optimizer logic for statistics-based aggregate folding.
- Adds Iceberg statistics storage, reading, validation, and test coverage.
File summaries
| File | Summary |
|---|---|
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/common/StatsSetupConst.java |
Adds batched column-stat freshness checks. |
ql/src/test/results/clientpositive/llap/stats_part.q.out |
Updates expected empty-table aggregate output. |
ql/src/test/queries/clientpositive/stats_part.q |
Adds empty-partition MAX coverage. |
ql/src/java/org/apache/hadoop/hive/ql/stats/StatsUtils.java |
Integrates handler row counts and statistics freshness. |
ql/src/java/org/apache/hadoop/hive/ql/plan/ColStatistics.java |
Tracks partial aggregate state. |
ql/src/java/org/apache/hadoop/hive/ql/optimizer/StatsOptimizer.java |
Adds aggregate folding. Critical, 1 vote: negative unknown null counts can produce incorrect COUNT. Moderate, 1 vote: loading all partition statistics can cause excessive memory use and transfer. |
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveReduceExpressionsWithStatsRule.java |
Avoids folding partial statistics. |
ql/src/java/org/apache/hadoop/hive/ql/metadata/HiveStorageHandler.java |
Defines handler statistics APIs. |
iceberg/iceberg-handler/src/test/results/positive/iceberg_part_colstats.q.out |
Updates partition-statistics query results. |
iceberg/iceberg-handler/src/test/results/positive/col_stats.q.out |
Updates column-statistics output. |
iceberg/iceberg-handler/src/test/queries/positive/iceberg_part_colstats.q |
Adds partition-statistics query coverage. |
iceberg/iceberg-handler/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStatistics.java |
Tests snapshot and aggregate behavior. |
iceberg/iceberg-handler/src/test/java/org/apache/iceberg/mr/hive/test/utils/HiveIcebergTestUtils.java |
Adds statistics test helpers. |
iceberg/iceberg-handler/src/test/java/org/apache/iceberg/mr/hive/stats/TestIcebergColStatsFormat.java |
Tests statistics encoding. |
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/stats/IcebergStoredStats.java |
Tracks statistics freshness. Moderate, 1 vote: the change-set key can reuse stale statistics across recreated or cross-catalog tables. |
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/stats/IcebergColStatsWriter.java |
Writes and merges statistics. |
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/stats/IcebergColStatsWritePolicy.java |
Supports statistics write policies. |
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/stats/IcebergColStatsReader.java |
Reads and aggregates statistics. |
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/IcebergTableUtil.java |
Supports snapshot resolution and statistics granularity. |
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/HiveIcebergStorageHandler.java |
Supplies Iceberg statistics. Critical, 1 vote: metadata-table scans can receive base-table row counts. |
Review details
Suppressed comments (2)
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/stats/IcebergStoredStats.java:252
CHANGED_PARTITIONSis process-wide, but its key identifies a table only bytable.name()and snapshot IDs. If a table is dropped and recreated with the same name (or the same name is used by another catalog) and the snapshot IDs overlap, a cached change set from the old table can be reused;upToDateColStatscan then accept stale partition statistics and return incorrect MIN/MAX/COUNT results. Include an immutable table identity such as the table location/catalog in this key.
int snapshotLookback = bounded ?
HiveConf.getIntVar(conf, ConfVars.HIVE_ICEBERG_STATS_MAX_SNAPSHOT_LOOKBACK) : Integer.MAX_VALUE;
// the walk reads manifests, and one query can ask it more than once: a table scanned twice
// over, or a DESC that asks column by column
ql/src/java/org/apache/hadoop/hive/ql/optimizer/StatsOptimizer.java:723
- This replaces the metastore's bounded aggregate response with a full
ColumnStatisticsObjlist for every requested partition, then materializes another list ofColumnStatisticsobjects before folding. Planning aMIN/MAX/COUNT(col)on a highly partitioned native table can therefore consume O(partitions × columns) client memory and transfer, causing timeouts or OOMs where the previous aggregate path returned O(columns). Preserve the exact-partition guarantee with an exact server-side aggregate API or a bounded/streaming approach rather than loading every partition's stats at once.
private AggrStats exactAggrColStats(List<String> partNames) throws HiveException, MetaException {
Map<String, List<ColumnStatisticsObj>> statsByPart = hive.getPartitionColumnStatistics(
tbl.getDbName(), tbl.getTableName(), partNames, colNames, true);
List<ColumnStatistics> partStats = new ArrayList<>();
- Files reviewed: 20/20 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| } else if (!HiveMetaHook.ICEBERG.equals(getStatsSource()) && !quickStats && | ||
| hmsTable.getSnapshotRef() == null) { | ||
| // the metastore parameters describe the table, not a branch: use the snapshot's counters | ||
| hmsTable.getQualifier().isEmpty()) { |
| if (result.size() != parts.size()) { | ||
| Logger.debug("Received " + result.size() + " stats for " + parts.size() + " partitions"); | ||
| Long nullCnt = getNullCountFor(type, statData); | ||
| if (nullCnt == null) { |
There was a problem hiding this comment.
master does the identical rowCnt -= nullCnt with no guard
…umn statistics MIN, MAX and COUNT over a column are facts the stored statistics already state, so a query asking only for them is answered from what a storage handler holds rather than by reading the rows. The statistics of every aggregate in a query are fetched at once, and a partitioned table is answered only from statistics that describe the partitions, the columns and the snapshot the scan asks about - a partition whose statistics do not cover every asked column, or which a live delete of no named partition may have changed, is not answered for. A table's size comes from its storage handler rather than from listing what its location holds, and whether a join can be a sort-merge is decided before its big table is elected, so an election made on the handler's numbers is not undone by one made on the listing's.
Entries in a partition blob answer by their field id even when a read decodes the whole blob, so what a dropped column left behind is stepped over rather than served under a namesake added since. And values aggregated from only some of the partitions a scan reads carry that mark, so the filter-reduction rule keeps estimating from them but never folds a predicate to a constant over them.
…gregates A partition-level file holds the table-level aggregates at its tail, so a session reading at table level answers from them rather than finding nothing at its own granularity - but only while the file states the table, which its registered entry marks. A gather over every partition states it, and a merge does while the file it carried from did; a partition-scoped gather with nothing to carry describes its partition alone and marks nothing. The write side stays strict: a table-level increment merges only into a file gathered as one, and a partition-level increment only into a partition-level file, so the leniency is a read-side courtesy over what is physically already there, never a cross-granularity merge.
The reduce rule folds IS NULL and IS NOT NULL when a column's null count equals the row count. The null count comes from the snapshot the scan reads - a branch or as-of, resolved through the handler - so the row count must come from the same snapshot, not the current table's metastore parameters. A branch null count read against the main row count folds IS NOT NULL to false and drops the branch's non-null rows.
The metastore holds one unversioned set of parameters describing the current table, so a branch, a tag or a point in time cannot be answered from it. getBasicStatistics guarded only the named-ref case and read the current parameters for an as-of scan; guarding on the whole qualifier, as the row count and the column-stats freshness already do, sends every versioned scan to its own snapshot summary.
- the schema resolves the id to the column's current name, lower cased - an entry whose field the schema dropped is left out, not renamed - a read resolves the asked columns to field ids once, not per entry
There was a problem hiding this comment.
🟡 Changes recommended
Correctness issues remain for unknown partitions and unknown null counts.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
ql/src/java/org/apache/hadoop/hive/ql/optimizer/StatsOptimizer.java:784
numNullsuses a negative value (notably-1) for an unknown null count, but this subtraction treats it as a real count. A handler/native statistic with unknown nulls would therefore producerowCnt + 1forCOUNT(column)and the optimizer would return an incorrect answer; decline the rewrite whenevernullCnt < 0.
return rowCnt - nullCnt;
ql/src/java/org/apache/hadoop/hive/ql/optimizer/StatsOptimizer.java:800
- The row-count path also builds
partishListfrom the retained partitions without checkingPrunedPartitionList.hasUnknownPartitions(). When pruning is not exact, this sum includes partitions the scan may discard, so the new constant-answer rewrite can return an incorrectCOUNT(*)for a partitioned native table. Require an exact pruned list before summing these partition counts, as the table-statistics path does.
for (Partish partish : partishList) {
- Files reviewed: 22/22 changed files
- Comments generated: 2
- Review effort level: Lite
|
|
||
| /** What the scan's partitions hold for every column asked about, or null to decline. */ | ||
| private Map<String, ColumnStatisticsObj> partitionColStats() throws HiveException { | ||
| Set<Partition> parts = prunedList.getPartitions(); |
There was a problem hiding this comment.
pruned list is exactly what the scan read. the rule pattern has no FIL, and PCR removes the filter only when it proves the predicate for every kept partition
| if (aggrStats != null && aggrStats.getPartsFound() != partNames.size() && stats.getColumnStatsState() != State.NONE) { | ||
| stats.updateColumnStatsState(State.PARTIAL); | ||
| // values aggregated from a subset of the scanned partitions estimate, but never | ||
| // answer; a partition column's stats come from the pruned values and stay exact | ||
| aggregatedStats.forEach(colStats -> colStats.setPartialAggregate(true)); |
There was a problem hiding this comment.
candidates are a superset of the scanned rows, so the bounds are wider
- the caller answers from nothing less, so folding a subset is work it discards
- a snapshot written without a summary names no operation
- a count or a length the rest of the blob cannot hold is refused outright - the codec frames a partition blob whole, so nothing outside it writes one - each name says the level it reads and whether it goes to the stream
…artition - the blob type says the file holds partitions, so the name of one is not copied up - a partition names itself on its own blob, in the file's own footer
- what the snapshot counts is the rows of the table the metadata describes
|



What changes were proposed in this pull request?
Answer MIN, MAX and COUNT from the handler's column statistic
Why are the changes needed?
Performance optimization
Does this PR introduce any user-facing change?
No
How was this patch tested?
TestIcebergColStatsFormat.java
TestHiveIcebergStatistics.java
iceberg_part_colstats.q
col_stats.q
depends on #6707