Copilot commented on code in PR #6662:
URL: https://github.com/apache/hive/pull/6662#discussion_r3690445177
##########
ql/src/java/org/apache/hadoop/hive/ql/stats/StatsUtils.java:
##########
@@ -207,38 +197,75 @@ private static Statistics collectStatistics(HiveConf
conf, PrunedPartitionList p
public static long getNumRows(HiveConf conf, List<ColumnInfo> schema, Table
table, PrunedPartitionList partitionList,
AtomicInteger noColsMissingStats) {
- List<Partish> inputs = new ArrayList<>();
- if (table.isPartitioned()) {
- for (Partition part : partitionList.getNotDeniedPartns()) {
- inputs.add(Partish.buildFor(table, part));
- }
- } else {
- inputs.add(Partish.buildFor(table));
- }
-
Factory basicStatsFactory = new BasicStats.Factory();
if (HiveConf.getBoolVar(conf, ConfVars.HIVE_STATS_ESTIMATE_STATS)) {
basicStatsFactory.addEnhancer(new BasicStats.DataSizeEstimator(conf));
basicStatsFactory.addEnhancer(new
BasicStats.RowNumEstimator(estimateRowSizeFromSchema(conf, schema)));
}
- for (Partish pi : inputs) {
- BasicStats bStats = new BasicStats(pi);
- long nr = bStats.getNumRows();
- // FIXME: this point will be lost after the factory; check that it's
really a warning....cleanup/etc
- if (nr <= 0) {
- // log warning if row count is missing
- noColsMissingStats.getAndIncrement();
- }
- }
- List<BasicStats> results = basicStatsFactory.buildAll(conf, inputs);
+ List<BasicStats> results;
+ if (table.isPartitioned() && checkCanProvidePartitionStats(table)) {
+ List<Partish> inputs = partitionList.getNotDeniedPartns().stream()
+ .map(part -> Partish.buildFor(table, part))
+ .toList();
+ results = buildBasicStats(conf, table, partitionList, inputs,
basicStatsFactory);
+ } else {
+ // partition-level statistics are unavailable (e.g. non-native table
with an external stats source):
+ // fall back to the table-level statistics rather than per-partition
minimums
+ results = List.of(buildBasicStats(table, basicStatsFactory));
+ }
+ // count the partishes with missing row counts (estimated rows do not
count as provided)
+ noColsMissingStats.addAndGet((int) results.stream()
+ .filter(bStats -> bStats.getRawNumRows() <= 0)
+ .count());
BasicStats aggregateStat = BasicStats.buildFrom(results);
aggregateStat.apply(new BasicStats.SetMinRowNumber());
return aggregateStat.getNumRows();
}
+ /**
+ * Builds the per-partish basic stats. For non-native tables the stats are
sourced from the storage handler in a
+ * single batched read, mirroring the aggregate column statistics retrieval
(see
+ * {@link HiveStorageHandler#getAggrBasicStatsFor}): one read serves the
whole partition list. Otherwise falls
+ * back to per-partish collection (see {@link BasicStats.Factory#buildAll}).
+ */
+ private static List<BasicStats> buildBasicStats(HiveConf conf, Table table,
PrunedPartitionList partList,
+ List<Partish> inputs, BasicStats.Factory factory) {
+ HiveStorageHandler storageHandler = table.isNonNative() ?
table.getStorageHandler() : null;
+ if (storageHandler != null && storageHandler.canProvideBasicStatistics()) {
+ if (partList != null && partList.getReferredPartCols().isEmpty()) {
Review Comment:
`PrunedPartitionList#getReferredPartCols()` can be empty both when there is
no partition predicate *and* when the pruner expression is constant-false (see
the class-level comment). In the constant-false case, `inputs` will be empty,
but this branch currently short-circuits to table-level statistics, producing a
large row estimate for a scan that actually reads no partitions. Add a guard so
the shortcut only applies when there are partitions in scope.
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/StatsOptimizer.java:
##########
@@ -933,29 +934,60 @@ private Collection<List<ColumnStatisticsObj>>
verifyAndGetPartColumnStats(
}
private Long getRowCnt(TableScanOperator tsOp, Table tbl) throws
HiveException {
- long rowCnt = 0L;
+ if (tbl.isNonNative()) {
+ return getRowCntFromStorageHandler(tsOp, tbl);
+ }
final List<Partish> partishList;
- if (tbl.isPartitioned() &&
StatsUtils.checkCanProvidePartitionStats(tbl)) {
+ if (tbl.isPartitioned()) {
partishList = pctx.getPrunedPartitions(tsOp.getConf().getAlias(),
tsOp).getPartitions().stream()
.map(Partish::buildFor)
- .collect(Collectors.toList());
+ .toList();
} else {
partishList = Lists.newArrayList(Partish.buildFor(tbl));
}
+ long rowCnt = 0L;
for (Partish partish : partishList) {
Map<String, String> basicStats = partish.getPartParameters();
- if (tbl.isNonNative()) {
- if (!tbl.getStorageHandler().canComputeQueryUsingStats(partish)) {
- return null;
- }
- basicStats = tbl.getStorageHandler().getBasicStatistics(partish);
- } else if
(!StatsUtils.areBasicStatsUptoDateForQueryAnswering(partish.getTable(),
partish.getPartParameters())) {
+ if
(!StatsUtils.areBasicStatsUptoDateForQueryAnswering(partish.getTable(),
basicStats)) {
return null;
}
- long partRowCnt =
Long.parseLong(basicStats.get(StatsSetupConst.ROW_COUNT));
- rowCnt += partRowCnt;
+ rowCnt += Long.parseLong(basicStats.get(StatsSetupConst.ROW_COUNT));
}
return rowCnt;
}
+
+ private Long getRowCntFromStorageHandler(TableScanOperator tsOp, Table
tbl) throws HiveException {
+ if (tbl.getMetaTable() != null) {
+ // metadata table scans cannot be answered from the data table's
statistics
+ return null;
+ }
+ HiveStorageHandler storageHandler = tbl.getStorageHandler();
+ if (tbl.isPartitioned()) {
+ PrunedPartitionList prunedList =
pctx.getPrunedPartitions(tsOp.getConf().getAlias(), tsOp);
+ if (!prunedList.getReferredPartCols().isEmpty()) {
Review Comment:
When `PrunedPartitionList#getReferredPartCols()` is empty, that can also
mean the pruner expression is constant-false (and the partition set is empty).
In that case this method currently falls through to `getRowCount(tbl)` and may
rewrite `count(*)` using the table-level row count, yielding an incorrect
non-zero result for a scan that reads no partitions. Handle the
empty-partition-list case explicitly before using table-level stats.
##########
common/src/java/org/apache/hadoop/hive/ql/ErrorMsg.java:
##########
@@ -313,6 +313,7 @@ public enum ErrorMsg {
"Not all clauses are supported with mapjoin hint. Please remove mapjoin
hint."),
ANALYZE_TABLE_NOSCAN_NON_NATIVE(10228, "ANALYZE TABLE NOSCAN cannot be used
for " + "a non-native table"),
+ ANALYZE_PARTITION_NON_NATIVE(10449, "ANALYZE TABLE with a PARTITION clause
cannot be used for a non-native table"),
Review Comment:
This error is thrown only when `tbl.hasNonNativePartitionSupport()` is true,
not for every non-native table. The current wording (“non-native table”) is
broader than the actual condition and can mislead users into thinking
partition-scoped ANALYZE is universally unsupported for all non-native tables.
Consider rewording to match the real restriction.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
