github-actions[bot] commented on code in PR #65782:
URL: https://github.com/apache/doris/pull/65782#discussion_r3609600659
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/helper/IcebergWriterHelper.java:
##########
@@ -61,6 +69,8 @@ public static WriteResult convertToWriterResult(
// Get table specification information
PartitionSpec spec = table.spec();
FileFormat fileFormat = IcebergUtils.getFileFormat(table);
+ MetricsConfig metricsConfig = MetricsConfig.forTable(table);
Review Comment:
[P2] Skip footer extraction when all metrics are disabled
This policy is first applied on FE after every BE has already populated
`TIcebergColumnStats`. For an unsorted table with
`write.metadata.metrics.default=none` and no column override,
`VIcebergPartitionWriter` still invokes both collectors for every output file;
the ORC collector even calls `_fs->open_file` and creates a new reader for the
just-written file before these filters discard every entry. Partition-heavy
writes to remote storage therefore pay a guaranteed-unused reopen/footer read
plus serialization per file. Please derive/pass an all-disabled decision
(accounting for overrides and sort promotion) to the sink and skip BE
collection in that case, with an ORC test that verifies no post-close reader is
opened.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/helper/IcebergWriterHelper.java:
##########
@@ -178,8 +189,65 @@ private static Metrics buildDataFileMetrics(Table table,
FileFormat fileFormat,
}
}
- return new Metrics(commitData.getRowCount(), columnSizes, valueCounts,
- nullValueCounts, null, lowerBounds, upperBounds);
+ // Physical file stats may contain every column, but manifest metrics
must honor the table's metadata policy.
+ return new Metrics(commitData.getRowCount(),
+ filterDisabledMetrics(columnSizes, schema, metricsConfig),
+ filterDisabledMetrics(valueCounts, schema, metricsConfig),
+ filterDisabledMetrics(nullValueCounts, schema, metricsConfig),
+ null,
+ filterBounds(lowerBounds, schema, metricsConfig, true),
+ filterBounds(upperBounds, schema, metricsConfig, false));
+ }
+
+ private static <T> Map<Integer, T> filterDisabledMetrics(
+ Map<Integer, T> metrics, Schema schema, MetricsConfig
metricsConfig) {
+ Map<Integer, T> filteredMetrics = new HashMap<>();
+ metrics.forEach((fieldId, value) -> {
+ if (MetricsUtil.metricsMode(schema, metricsConfig, fieldId) !=
MetricsModes.None.get()) {
+ filteredMetrics.put(fieldId, value);
+ }
+ });
+ return filteredMetrics;
+ }
+
+ private static Map<Integer, ByteBuffer> filterBounds(
+ Map<Integer, ByteBuffer> bounds, Schema schema, MetricsConfig
metricsConfig, boolean lowerBound) {
+ Map<Integer, ByteBuffer> filteredBounds = new HashMap<>();
+ bounds.forEach((fieldId, value) -> {
+ MetricsModes.MetricsMode mode = MetricsUtil.metricsMode(schema,
metricsConfig, fieldId);
+ if (mode == MetricsModes.None.get() || mode ==
MetricsModes.Counts.get()) {
+ return;
+ }
+
+ ByteBuffer filteredValue = value;
+ if (mode instanceof MetricsModes.Truncate) {
+ Type type = schema.findType(fieldId);
Review Comment:
[P1] Handle v3 row-lineage IDs before type lookup
For a default-config v3 `rewrite_data_files`, the sink appends `_row_id` and
`_last_updated_sequence_number` to the schema sent to BE, and both footer
collectors return bounds keyed by those reserved IDs. The commit path calls
this helper with `transaction.table().schema()`, which does not contain the
metadata fields. `MetricsUtil.metricsMode` therefore falls back to the default
`truncate(16)` mode, this lookup returns null, and `truncateBound` dereferences
it at `type.typeId()`. The existing Parquet/ORC v3 rewrite path will fail after
writing the replacement files. Please either apply the policy with the exact
write schema or intentionally exclude these metadata IDs before type lookup,
and cover the real row-lineage rewrite caller.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/helper/IcebergWriterHelper.java:
##########
@@ -178,8 +189,65 @@ private static Metrics buildDataFileMetrics(Table table,
FileFormat fileFormat,
}
}
- return new Metrics(commitData.getRowCount(), columnSizes, valueCounts,
- nullValueCounts, null, lowerBounds, upperBounds);
+ // Physical file stats may contain every column, but manifest metrics
must honor the table's metadata policy.
+ return new Metrics(commitData.getRowCount(),
+ filterDisabledMetrics(columnSizes, schema, metricsConfig),
+ filterDisabledMetrics(valueCounts, schema, metricsConfig),
+ filterDisabledMetrics(nullValueCounts, schema, metricsConfig),
+ null,
+ filterBounds(lowerBounds, schema, metricsConfig, true),
+ filterBounds(upperBounds, schema, metricsConfig, false));
+ }
+
+ private static <T> Map<Integer, T> filterDisabledMetrics(
+ Map<Integer, T> metrics, Schema schema, MetricsConfig
metricsConfig) {
+ Map<Integer, T> filteredMetrics = new HashMap<>();
+ metrics.forEach((fieldId, value) -> {
+ if (MetricsUtil.metricsMode(schema, metricsConfig, fieldId) !=
MetricsModes.None.get()) {
Review Comment:
[P2] Suppress logical metrics below repeated fields
Separately from the `counts`/`truncate` handling in the existing thread,
this condition keeps value/null maps for every non-`none` leaf ID, and
`filterBounds` does the same for bounds. Doris assigns IDs to Parquet list
elements and map keys/values, so the BE footer collector sends those physical
leaf statistics here. Iceberg 1.10.1 deliberately makes
`ParquetMetrics.MetricsVisitor.list/map` return no descendant value/null/bound
metrics (while retaining physical column sizes): definition-level placeholders
for a null or empty container cannot be interpreted as logical element nulls.
For rows such as `NULL`, `[]`, and `[1, NULL]`, Doris would publish misleading
element counts in `$files`/`readable_metrics`. Please index schema ancestry and
omit list/map-descendant counts and bounds while retaining their column sizes,
with null-container/empty-container/null-element tests.
--
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]
