gaborkaszab commented on code in PR #15979:
URL: https://github.com/apache/iceberg/pull/15979#discussion_r3093387568
##########
core/src/main/java/org/apache/iceberg/PartitionStatsHandler.java:
##########
@@ -559,6 +564,75 @@ private static List<PartitionStatistics>
sortStatsByPartition(
return entries;
}
+ /**
+ * Populates {@code total_record_count} for partitions where it can be
reliably computed from
+ * metadata without scanning data files.
+ *
+ * <p>Per the spec, {@code total_record_count} should stay optional (NULL
means unknown). It can
+ * be computed when a partition has no equality deletes and no V2 position
delete files — i.e.,
+ * deletes are tracked exclusively via deletion vectors (DVs) or the
partition has no deletes at
+ * all. In that case, the total is: {@code data_record_count -
position_delete_record_count}.
+ *
+ * <p>DV cardinalities are exact (each position maps to one deleted row), so
the subtraction
+ * yields a precise total without reading any data files.
+ *
+ * @param stats the computed partition statistics to enrich
+ */
+ @VisibleForTesting
+ static List<PartitionStatistics>
populateTotalRecordCount(Collection<PartitionStatistics> stats) {
+ List<PartitionStatistics> result =
Lists.newArrayListWithExpectedSize(stats.size());
+ for (PartitionStatistics partitionStats : stats) {
+ boolean hasEqualityDeletes =
+ partitionStats.equalityDeleteRecordCount() != null
+ && partitionStats.equalityDeleteRecordCount() > 0;
+ boolean hasV2PositionDeleteFiles =
+ partitionStats.positionDeleteFileCount() != null
+ && partitionStats.positionDeleteFileCount() > 0;
+
Review Comment:
Why not skip the rest of the loop here if there are eq or v2 pos deletes?
##########
core/src/main/java/org/apache/iceberg/PartitionStatsHandler.java:
##########
@@ -608,7 +682,9 @@ private static void liveEntry(PartitionStatistics stats,
ContentFile<?> file, Sn
updateSnapshotInfo(stats, snapshot.snapshotId(),
snapshot.timestampMillis());
}
- // Note: Not computing the `TOTAL_RECORD_COUNT` for now as it needs
scanning the data.
+ // Note: total_record_count is populated by populateTotalRecordCount()
after all
Review Comment:
Can there be a way to calculate this stat as part of `computeStats` and/or
`computeAndMergeStatsIncremental`?
Anyway these comments aren't that useful here, at least for me.
##########
core/src/main/java/org/apache/iceberg/PartitionStatsHandler.java:
##########
@@ -559,6 +564,75 @@ private static List<PartitionStatistics>
sortStatsByPartition(
return entries;
}
+ /**
+ * Populates {@code total_record_count} for partitions where it can be
reliably computed from
+ * metadata without scanning data files.
+ *
+ * <p>Per the spec, {@code total_record_count} should stay optional (NULL
means unknown). It can
+ * be computed when a partition has no equality deletes and no V2 position
delete files — i.e.,
+ * deletes are tracked exclusively via deletion vectors (DVs) or the
partition has no deletes at
+ * all. In that case, the total is: {@code data_record_count -
position_delete_record_count}.
+ *
+ * <p>DV cardinalities are exact (each position maps to one deleted row), so
the subtraction
+ * yields a precise total without reading any data files.
+ *
+ * @param stats the computed partition statistics to enrich
+ */
+ @VisibleForTesting
+ static List<PartitionStatistics>
populateTotalRecordCount(Collection<PartitionStatistics> stats) {
+ List<PartitionStatistics> result =
Lists.newArrayListWithExpectedSize(stats.size());
+ for (PartitionStatistics partitionStats : stats) {
Review Comment:
I wonder how useful such a stat can be that is populated for some partitions
but might not be populated for others.
##########
core/src/main/java/org/apache/iceberg/PartitionStatsHandler.java:
##########
@@ -559,6 +564,75 @@ private static List<PartitionStatistics>
sortStatsByPartition(
return entries;
}
+ /**
+ * Populates {@code total_record_count} for partitions where it can be
reliably computed from
+ * metadata without scanning data files.
+ *
+ * <p>Per the spec, {@code total_record_count} should stay optional (NULL
means unknown). It can
+ * be computed when a partition has no equality deletes and no V2 position
delete files — i.e.,
+ * deletes are tracked exclusively via deletion vectors (DVs) or the
partition has no deletes at
+ * all. In that case, the total is: {@code data_record_count -
position_delete_record_count}.
+ *
+ * <p>DV cardinalities are exact (each position maps to one deleted row), so
the subtraction
+ * yields a precise total without reading any data files.
+ *
+ * @param stats the computed partition statistics to enrich
+ */
+ @VisibleForTesting
+ static List<PartitionStatistics>
populateTotalRecordCount(Collection<PartitionStatistics> stats) {
+ List<PartitionStatistics> result =
Lists.newArrayListWithExpectedSize(stats.size());
+ for (PartitionStatistics partitionStats : stats) {
+ boolean hasEqualityDeletes =
+ partitionStats.equalityDeleteRecordCount() != null
+ && partitionStats.equalityDeleteRecordCount() > 0;
+ boolean hasV2PositionDeleteFiles =
+ partitionStats.positionDeleteFileCount() != null
+ && partitionStats.positionDeleteFileCount() > 0;
+
+ // Create a fresh BasePartitionStatistics to avoid projection issues on
read-back objects
Review Comment:
I'm not sure I get this. Could you explain more why the copy is needed?
--
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]
