[fix](iceberg) Honor disabled write metrics#65782
Open
Gabriel39 wants to merge 9 commits into
Open
Conversation
Jira: DORIS-27023 Filter backend-collected file statistics using the Iceberg table metrics configuration before writing manifest metadata.
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Gabriel39
marked this pull request as ready for review
July 18, 2026 11:59
Contributor
TPC-H: Total hot run time: 29537 ms |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
I found two issues that should be addressed before merge.
Checkpoint conclusions:
- Goal and data correctness: the new effective-
nonefiltering matches Iceberg 1.10.1 for all five populated metric maps and is shared by insert/overwrite, merge/row-delta, and rewrite commits. However, Doris's enabled HMS Iceberg statistics-fetch path cannot consume the newly absent maps and falls back toUNKNOWNthrough repeated exceptions. - Scope and parallel paths: the production change is focused, and both BE Parquet and ORC payloads converge on this helper. The downstream statistics consumer is the only direct Java
ContentFilemetrics consumer found with an absent-map assumption. - Concurrency and lifecycle: conversion is synchronous; Iceberg's transaction table reads one non-refreshing metadata snapshot, so there is no cross-file schema/property race or lock-order issue. That same stability makes rebuilding the table policy for every file unnecessary.
- Configuration and compatibility: no Doris config, FE-BE protocol, function symbol, or storage-format field is added. Omitting optional manifest maps for effective
noneis standard Iceberg behavior, butenable_fetch_iceberg_statsneeds an explicit missing-metrics fallback. - Transactions, persistence, and writes: append, replace, row-delta, and rewrite transaction mechanics are unchanged; manifest persistence is the intended behavior. No EditLog path applies to this external Iceberg commit.
- Testing: the new unit test covers the direct global-
nonehelper result but not the write-to-statistics consumer failure. Per the review-runner contract I did not build locally. CheckStyle passes. The current FE UT job is red because Maven received a truncatedpaimon-s3dependency beforefe-core; 293 earlier tests passed, so this is infrastructure failure and the changed test did not execute in that job. The author-reported targeted 7-test result was not independently rerun here. - Observability: existing cache-loader logging exposes the exception but does not make the configured statistics feature work; no separate metrics/logging change otherwise appears necessary.
- User focus: no additional focus points were supplied.
Please address the two inline comments and add the missing absent-metrics coverage.
|
|
||
| return new Metrics(commitData.getRowCount(), columnSizes, valueCounts, | ||
| nullValueCounts, null, lowerBounds, upperBounds); | ||
| MetricsConfig metricsConfig = MetricsConfig.forTable(table); |
Contributor
There was a problem hiding this comment.
[P2] Build the metrics policy once per file batch
convertToWriterResult invokes this method once for every TIcebergCommitData, so these new lines rebuild the same MetricsConfig and fetch the same schema for every output file. MetricsConfig.forTable walks schema field IDs/inferred defaults, sort columns, and properties; partition-heavy or heavily rolled writes therefore repeat fixed table work F times on the synchronous FE commit path. The transaction table's metadata is stable for this batch, so please capture the config/schema once before the loop and pass them into buildDataFileMetrics.
| Schema schema = table.schema(); | ||
| // 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), |
Contributor
There was a problem hiding this comment.
[P2] Handle disabled maps in Iceberg stats fetching
An effective none mode correctly makes both columnSizes() and nullValueCounts() absent, but StatisticsUtil.getIcebergColumnStats still unconditionally adds task.file().columnSizes().get(colId) and task.file().nullValueCounts().get(colId) at lines 608/610. After an HMS Iceberg table is written with this property, enabling enable_fetch_iceberg_stats therefore drives the stats-cache load into a null dereference/fallback for every affected column. Please update that consumer to recognize missing metrics (return unknown/empty rather than aggregating a fabricated zero) and cover the write-to-stats path.
Contributor
TPC-DS: Total hot run time: 178094 ms |
Contributor
ClickBench: Total hot run time: 25.15 s |
Build the Iceberg metrics policy once per commit batch and return unknown statistics when required file metrics are absent.\n\nJira: DORIS-27023
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
TPC-H: Total hot run time: 29374 ms |
Contributor
TPC-DS: Total hot run time: 181069 ms |
Contributor
There was a problem hiding this comment.
I found one remaining correctness issue in the close-error path. The normal-path changes address the two earlier inline concerns: Iceberg's effective none policy is applied once per file batch, and missing required metrics now produce unknown statistics when the scan closes normally.
Checkpoint conclusions:
- Goal and data correctness: effective-
nonefiltering matches Iceberg 1.10.1 across global defaults, per-column overrides, wide-schema inferred defaults, and sorted-column promotion. However, the new earlyOptional.empty()result can still be replaced by partial or fabricated-zero statistics when closing the scan throws; see the inline comment. - Scope and parallel paths: the production change is focused. Append, overwrite, merge/row-delta, and rewrite data files all converge on
convertToWriterResult; HMS Iceberg statistics converge ongetIcebergColumnStats. No additional unmodified parallel path was found. - Concurrency and lifecycle: the conversion uses method-local state and adds no threads or locks. Iceberg's transaction table is non-refreshing, so policy/schema hoisting observes one transaction-local metadata snapshot. The remaining lifecycle defect is limited to the try-with-resources close-error completion path.
- Configuration and compatibility: no Doris config, Thrift field, function symbol, EditLog format, or storage-format field is added. Omitting optional manifest metric maps for effective
noneis standard Iceberg behavior, and mixed old/new files conservatively return unknown on the normal path. - Transactions, persistence, and writes: existing Iceberg commit atomicity, visibility, cleanup, and append/overwrite/merge/rewrite mechanics are unchanged; only
DataFilemanifest metrics are affected. - Error handling and observability: absent maps or entries correctly invalidate the whole aggregation on normal close, and the existing warning identifies close failures. The catch must also preserve the unavailable result instead of allowing accumulator state to escape.
- Performance:
MetricsConfigand schema work is now performed once per batch; per-file filtering is linear in supplied metric entries. No additional hot-path issue was substantiated. - Testing: the new JUnit tests cover global
none, once-per-batch policy construction, and the normal write-to-statistics fallback. They use a no-op close and therefore miss the accepted close-IOExceptionpath. Per the review-runner contract I did not build or run tests locally. - User focus: no additional focus points were supplied.
Please address the inline issue and add throwing-close coverage.
| Long nullValueCount = nullValueCounts == null ? null : nullValueCounts.get(colId); | ||
| // Iceberg can omit maps or entries for mode=none; partial aggregation would fabricate zero stats. | ||
| if (columnSize == null || nullValueCount == null) { | ||
| return Optional.empty(); |
Contributor
There was a problem hiding this comment.
[P2] Preserve unknown stats when closing the scan fails
This return runs inside try-with-resources. If CloseableIterable.close() throws IOException, Java abandons the pending Optional.empty() return, enters the catch below, and then builds a statistic from the current accumulators. For example, a complete file followed by a metrics-disabled file will expose partial totals if close also fails; if the disabled file is first, it exposes a fabricated zero statistic. Please return Optional.empty() from the IOException catch (or otherwise preserve the unavailable state) and cover a throwing-close iterable. This is distinct from the prior missing-map thread: normal close is fixed here, while the close-error path still escapes partial data.
Contributor
ClickBench: Total hot run time: 24.88 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
Return unknown Iceberg column statistics when closing the scan fails, preventing partial or zero accumulators from escaping. Jira: DORIS-27023
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Automated review: changes requested
The current head correctly omits metrics for effective none mode, builds the table policy once per batch, and returns unknown statistics when required file metrics are absent or scan close fails. One blocking configuration-semantics issue remains: counts and truncate(N) are still treated like full for bound maps (see inline comment).
Critical checkpoint conclusions:
- Goal and test proof: Partially achieved. The new tests cover
none, batch policy construction, missing metrics, and close failure, but do not covercountsortruncate(N), which exposes the remaining defect. - Scope and clarity: The four-file FE change is focused and reuses the central data-file conversion path.
- Concurrency and lifecycle: No new shared mutable state, locks, threads, static initialization, or special owner lifecycle;
CloseableIterableis managed by try-with-resources and close failure now preserves unknown statistics. - Configuration, compatibility, and persistence: No new config or Doris protocol field is introduced, and optional Iceberg manifest maps remain format-compatible. However, existing Iceberg metrics properties are not fully honored because raw full bounds are persisted for
countsand string/binarytruncate(N). - Parallel paths, transactions, and data writes: Insert/overwrite, merge/row-delta, and rewrite all converge on
convertToWriterResult; Parquet and ORC producers were traced. No separate atomicity, failover, or path-propagation issue was found. - Error handling, conditions, and observability: Missing maps/entries and checked close failures now fail conservatively to unknown statistics. Existing cache-loader/log boundaries are adequate; no additional metric is needed.
- Performance: Policy/schema construction is amortized per batch as intended. No other material hot-path regression was substantiated.
- Validation: Static review only, as required by the runner contract; no local build or test was run. The added JUnit counterfactuals for the already-known regressions were inspected, and GitHub CheckStyle was green during review.
- User focus: No additional user-provided focus was specified.
Review completeness: The complete authoritative diff, changed-file list, existing threads, producer/consumer paths, tests, and applicable Doris checkpoints were reviewed. Every candidate was verified and deduplicated; MAIN-1 is the only distinct issue to submit.
| 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()) { |
Contributor
There was a problem hiding this comment.
[P1] Apply the effective mode to each metric family
Every non-None mode passes through the same raw maps here. With write.metadata.metrics.default=counts, the BE-provided lower/upper bounds are still written even though Iceberg counts mode must omit them; with the normal truncate(16) default (and explicit or sort-promoted truncate modes), long string/binary bounds are copied in full instead of using Iceberg's safe lower/upper truncation. The BE producers emit complete footer extrema and DataFiles.Builder.withMetrics does not reapply MetricsConfig, so these full values reach manifests. Please make bound handling mode-aware—drop bounds for counts and decode/truncate/re-encode string/binary bounds for truncate(N)—and add counts/truncate tests.
Contributor
TPC-H: Total hot run time: 28867 ms |
Contributor
TPC-DS: Total hot run time: 178692 ms |
Contributor
ClickBench: Total hot run time: 24.83 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
Omit bounds for counts mode and safely truncate string and binary bounds for truncate mode before writing Iceberg manifests. Jira: DORIS-27023
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
TPC-H: Total hot run time: 29601 ms |
Contributor
There was a problem hiding this comment.
Automated review: changes requested
I found three additional issues on the current head: one P1 commit failure and two P2 manifest/performance gaps. I deduplicated them against all four existing inline threads; the new set is limited to the v3 row-lineage schema boundary, repeated-field logical metrics, and all-disabled BE extraction.
Checkpoint conclusions:
- Goal and proof: The patch now applies Iceberg metrics modes for ordinary fields and preserves unknown statistics, but it does not fully accomplish the goal because v3 row-lineage bounds can fail commit and repeated-field metrics still differ from Iceberg.
- Scope and focus: The four-file Java change is localized and clear, but the implementation stops after BE collection and uses the table schema where a larger physical write schema is required. No additional user focus was supplied.
- Concurrency: No new shared mutable state, locks, threads, or ordering hazards are introduced; schema/config objects are batch-local.
- Lifecycle: The
planFilestry-with-resources close-error path now preserves unknown state. No static/global lifecycle is involved. The remaining ORC reopen is a synchronous performance issue, not a leaked resource found in this diff. - Configuration: No Doris configuration item is added. Existing Iceberg metrics properties, per-column overrides, and sort-column promotion form the policy being interpreted.
- Compatibility: The current diff changes no symbol, storage format, or FE-BE wire field. Any upstream all-disabled optimization should preserve collect-by-default behavior for rolling FE/BE compatibility.
- Parallel paths: Append, overwrite, merge, and rewrite converge on this helper; v3 rewrite and matched mutation paths add writer-only row-lineage fields that the captured table schema omits.
- Conditions: The ordinary
none,counts, andtruncate(N)branches are mode-aware, but the filters lack the separate list/map-ancestor rule used by Iceberg. - Test coverage: Added JUnit cases cover ordinary none/counts/truncate behavior and one throwing-close path. Missing coverage includes real v3 Parquet/ORC row-lineage callers, null/empty nested list/map data, and proof that all-disabled writes skip BE collection.
- Test results: The new assertions are consistent with the covered ordinary cases; no generated result file is changed. I did not execute tests because the supplied review contract explicitly prohibits builds and test runs.
- Observability: No new logging or metrics requirement was identified for this policy transformation.
- Transactions and persistence: No Doris EditLog or persistence-format change is present. The P1 path is transactionally significant because replacement files are written before manifest conversion fails.
- Data writes: Manifest metric correctness is part of the Iceberg commit contract; MAIN-1 can abort a supported v3 commit and MAIN-3 publishes misleading repeated-field metadata. No additional write-concurrency issue was found.
- FE-BE propagation: No new variable is transmitted. That omission is exactly why an effective all-
nonepolicy cannot prevent guaranteed-unused BE footer collection. - Performance: Building the policy once per batch fixes the prior FE repetition, but all-disabled Parquet footer walks and per-file ORC remote reopens remain.
- Other issues: The
StatisticsUtilmissing-map and close-failure handling is now conservative. No further nonduplicate issue survived the final sweep.
Validation was static-only as required. The live head remained a91df699cc3a0c226f2e521f52535cfa1c3bb053, matching the authoritative review bundle, and all three convergence reviewers returned NO_NEW_VALUABLE_FINDINGS against this unchanged comment set.
| // Get table specification information | ||
| PartitionSpec spec = table.spec(); | ||
| FileFormat fileFormat = IcebergUtils.getFileFormat(table); | ||
| MetricsConfig metricsConfig = MetricsConfig.forTable(table); |
Contributor
There was a problem hiding this 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.
| 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()) { |
Contributor
There was a problem hiding this 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.
|
|
||
| ByteBuffer filteredValue = value; | ||
| if (mode instanceof MetricsModes.Truncate) { | ||
| Type type = schema.findType(fieldId); |
Contributor
There was a problem hiding this 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.
Contributor
Author
|
/review |
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
There was a problem hiding this comment.
Request changes: one new P2 inline issue remains on the current head.
Review conclusions
- Goal and correctness: The current head addresses the existing threads for none/counts/truncate/full filtering, repeated descendants, v3 type lookup, downstream unknown statistics, close failures, and avoiding unused footer reads. It does not yet fully honor the effective metrics policy because the new all-disabled gate evaluates an incomplete field set; the inline comment gives ORC-complex and v3-lineage triggers.
- Scope and clarity: The patch is focused on metrics policy, transport, and tests. The remaining fix should keep the decision aligned with the exact writer schema/collector rather than widen unrelated code.
- Concurrency and lifecycle: No new shared mutable state or lock-order risk. The per-writer flag is immutable after construction; mandatory commit-data fields are populated before the early return, and direct/merge/sorted-rollover paths preserve the flag.
- Configuration and compatibility: Iceberg default/column/sort modes otherwise flow through
MetricsConfig. The new optional Thrift fields are rolling-upgrade safe: unset old-FE payloads collect by default, while old BEs ignore the new field and continue collecting. No Doris dynamic configuration or storage-format compatibility change is introduced. - Parallel paths and FE/BE propagation: Direct table and merge sinks both set the field, merge forwards it into the inner table sink, and writer rollover reuses the same sink. That complete propagation is why the FE false negative suppresses both Parquet/ORC lineage metrics and the ORC complex-field case.
- Transactions, persistence, and data writes:
HasTableOperationscorrectly resolves v3 transaction metadata, and the helper uses the matching reserved lineage IDs/types. The change affects manifest metrics only; file contents, transaction atomicity, EditLog, and visible-version semantics are not otherwise changed. - Errors, observability, and performance: Missing/partial consumer maps and scan-close failures now return unknown rather than partial statistics. No new observability is needed. Skipping a guaranteed-unused ORC reopen is sound; the remaining per-file schema-index work is already covered by an existing performance thread and was not duplicated.
- Tests: The new FE/BE unit tests cover primitive none/counts/truncate modes, repeated descendants, v3 type lookup, unknown statistics, close failure, disabled footer work, and old-FE default behavior. Coverage is missing for the two accepted false-negative triggers. Per the review-runner contract, I performed static review only and did not run builds or tests.
- User focus: No additional user-provided focus was supplied.
| Schema schema = table.schema(); | ||
| MetricsConfig metricsConfig = MetricsConfig.forTable(table); | ||
| return TypeUtil.indexById(schema.asStruct()).values().stream() | ||
| .filter(field -> field.type().isPrimitiveType()) |
Contributor
There was a problem hiding this comment.
[P2] Evaluate collection against the actual writer field set
This gate can return false even though the writer has enabled metrics outside this primitive user-schema set. For ORC items list<int> with default none and column.items=counts, it drops the top-level items field and only checks items.element (still none), although both Doris and Iceberg ORC metrics publish top-level counts. Separately, for a v3 rewrite/merge with default counts and every user field overridden to none, this returns false before the binders append _row_id and _last_updated_sequence_number; those appended fields inherit counts, and the collectors/helper support their reserved IDs. In both cases BE skips the footer and requested manifest metrics disappear. Please evaluate the effective policy against the exact schema/field set sent to the selected writer (including v3 lineage and ORC top-level complex fields), with tests for both triggers.
Contributor
TPC-H: Total hot run time: 29738 ms |
Contributor
TPC-DS: Total hot run time: 178142 ms |
Contributor
ClickBench: Total hot run time: 25.77 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
There was a problem hiding this comment.
Automated review: changes requested (three-round cap reached)
One new P1 regression remains on head 53a14e38c9155eefaa1207531f3c7b1d278fe39c: the added string-bound conversion can decode a numeric ORC bound that the pre-existing collector mislabeled after a complex field. With invalid UTF-8 bytes, FE throws during convertToWriterResult after BE has produced the file, so insert, rewrite, and merge can fail. The trigger and fix direction are inline.
Review completion: all three permitted rounds were completed. Both normal final passes returned NO_NEW_VALUABLE_FINDINGS, but the final risk pass produced this accepted candidate. The workflow forbids a fourth convergence round, so this review is explicitly incomplete at the round limit; every currently known candidate nevertheless has a verified, deduplicated main-agent conclusion.
Critical checkpoint conclusions:
- Goal and proof: the aggregate diff otherwise applies Iceberg
none,counts,truncate(N), andfullpolicies to the correct metric families, handles repeated descendants and v3 lineage, skips disabled BE footer work, and makes missing read statistics unknown. The new cross-type ORC trigger means the goal is not yet correct for every writer schema, and no test covers it. - Scope and clarity: the patch is focused on metrics policy, propagation, footer skipping, and statistics fallback. All eight bundled prior inline concerns have corresponding fixes on the current head.
- Concurrency and lifecycle: no new thread, lock, shared mutable state, static initialization, or ownership cycle is introduced. The per-writer flag is immutable; the disabled path retains file identity, counts, size, content, and partition data before skipping footer extraction.
- Configuration and conditions: no Doris configuration item is added; existing Iceberg table properties drive the policy. The Parquet-leaf versus ORC-top-level condition matches the collectors' intended field sets, but the ORC collector's physical-ID assumption creates the accepted interaction.
- Compatibility, parallel paths, and FE-BE state: the optional thrift flag is sent by direct and merge sinks, preserves presence through merge nesting, reaches normal/sorted/rolled writers, defaults to collection for old FE/new BE, and is safely ignored by old BE/new FE. Insert, rewrite, and merge all share the affected FE conversion.
- Tests and results: unit coverage exercises modes, repeated ancestry, v3 lineage, disabled/unset collection, missing maps, and close failure, but lacks the nested-numeric-followed-by-string ORC case. Per the review contract, I ran no local build or tests. At submission, compile, Cloud UT, vault P0, format, and CheckStyle are green; macOS BE UT failed before compilation from the JDK 25 versus required JDK 17 runner mismatch; both coverage checks are red; main BE/FE UT and several regressions remain pending.
- Observability and errors: existing transaction logging will expose the failure, but observability cannot make the deterministic conversion safe. The
RuntimeIOExceptioncorrectly fails rather than silently continuing. - Transactions, persistence, and data writes: no EditLog or durable Doris metadata schema changes are involved. Existing Iceberg transaction boundaries remain, but the accepted failure happens during manifest-metric conversion after the BE data file is written and before commit.
- Performance: capturing schema/config once per batch and skipping unused footer extraction remove the intended fixed and remote-I/O work. Remaining per-file parent indexing is in the already-raised policy-construction performance family and was not duplicated.
- Other issues: three serious adjacent ORC producer defects were independently validated—shifted physical IDs after complex fields, sub-millisecond timestamp upper bounds, and negative-decimal sign encoding—but their original same-type/timestamp/decimal behavior predates this aggregate diff and is unchanged or less exposed, so they are noted as non-blocking follow-up risks rather than charged to this PR.
User focus: no additional review focus was provided; the complete PR was reviewed.
| private static ByteBuffer truncateBound(Type type, ByteBuffer value, int length, boolean lowerBound) { | ||
| switch (type.typeId()) { | ||
| case STRING: | ||
| String stringValue = Conversions.fromByteBuffer(type, value).toString(); |
Contributor
There was a problem hiding this comment.
[P1] Do not decode shifted ORC bounds as UTF-8
The ORC collector still indexes top-level fields as i + 1, so after a complex field it can attach a descendant's bytes to the following field. For s struct<a:int>, b string with s.a=-1, the second iteration reads a's integer statistics and stores eight 0xFF bytes under b's Iceberg ID. Before this diff those bytes were passed to Metrics without decoding; under the normal truncate(16) mode this new call decodes them as UTF-8, and Iceberg's strict decoder throws RuntimeIOException, causing insert/rewrite/merge to fail during FE commit conversion. Please resolve the actual ORC column ID before type-aware conversion (or otherwise guarantee the bound type matches the field), and cover a nested negative integer followed by a string in an ORC write test.
Contributor
TPC-H: Total hot run time: 29636 ms |
Contributor
TPC-DS: Total hot run time: 177572 ms |
Contributor
ClickBench: Total hot run time: 25.01 s |
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Automated review conclusion for head 471470f5375d9483cd9b7034008fcedc8289f50b: one new blocking build issue remains; see the inline comment on vorc_transformer.cpp. The current head otherwise addresses all nine existing inline review concerns without a new semantic correctness finding.
Checkpoint conclusions:
- Goal and proof: the FE metrics policy, BE collection gate, manifest normalization, ORC field-ID fix, and unavailable-statistics handling implement the stated behavior and have focused FE/BE tests, but the changed ORC collector currently fails compilation before those tests can run.
- Scope: changes stay within Iceberg metrics collection/transport/filtering, ORC footer identity, and the Iceberg statistics consumer; no unrelated production behavior was found.
- Concurrency and initialization: no new shared mutable state, locking, static initialization, or thread-lifecycle dependency is introduced.
- Resource lifecycle: disabled collection skips footer work only after required commit metadata is populated; enabled close/reopen behavior is otherwise unchanged.
- Configuration: no new runtime configuration item or validation boundary is introduced.
- Compatibility and parallel paths: direct insert/overwrite, rewrite, merge, partition rollover, sorted writes, Parquet/ORC, and v1/v2/v3 schemas were traced. The optional Thrift field preserves the supported old-FE/new-BE default, and merge propagation is present. The supported upgrade order is BE first, then FE.
- Tests: the added tests cover the policy flag/default, metric modes and truncation, repeated descendants, v3 lineage, checked close failure, and nested-ORC ID alignment. Local builds/tests were intentionally not run under the review contract; the authoritative compile job instead exposed the inline width-conversion failure.
- Observability and failures: missing metrics fail closed to unknown statistics; no new silent partial result was found. Existing logging/error boundaries are retained.
- Transactions, persistence, and data writes: transaction-local Iceberg metadata and v3 lineage schema resolution are consistent; no new EditLog, persistence, visibility, atomicity, or retry boundary is introduced.
- Performance: the disabled gate avoids unnecessary footer work; the remaining fixed-schema-per-file work is already covered by an existing review thread and was not duplicated.
- Other: all existing review threads were deduplicated before submission. No additional user-provided review focus was supplied.
Validation state at submission: CheckStyle, Clang Formatter, license, dependency, secret, large-file, and Cloud UT checks passed. TeamCity compile build 1000377 failed on the inline issue; BE/FE coverage checks then failed because that compile artifact was not published. The macOS BE UT failure is an independent runner JDK mismatch (JDK 25 versus required JDK 17). Remaining BE UT, FE UT, performance, and automated-review checks were still pending during this review. The local checkout lacks .worktree_initialized, thirdparty/installed, and protoc, so validation here was static plus exact-head CI-log inspection.
| continue; | ||
| } | ||
| // ORC IDs are depth-first, so top-level fields after a complex field are not i + 1. | ||
| uint64_t orc_col_id = orc_root_type.getSubtype(i)->getColumnId(); |
Contributor
There was a problem hiding this comment.
[P1] Keep the ORC statistics index in the API's accepted width
This changes orc_col_id to uint64_t, but the next call passes it to orc::Statistics::getColumnStatistics(uint32_t). The compile job for this exact head fails here under Doris's -Werror,-Wshorten-64-to-32 flags (implicit conversion loses integer precision), so no compile artifact is produced and both coverage jobs fail downstream. Please perform an explicit, range-justified conversion (or otherwise keep the value in the API's accepted width) before the call.
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
TPC-H: Total hot run time: 29983 ms |
Contributor
TPC-DS: Total hot run time: 177857 ms |
Contributor
ClickBench: Total hot run time: 24.96 s |
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
There was a problem hiding this comment.
Automated review summary
One correctness issue remains in ORC truncate-mode metric handling; see the inline comment.
- Goal and proof: The change now honors Iceberg metrics modes across FE filtering and BE footer collection, including disabled collection, counts/truncate behavior, repeated fields, v3 lineage, ORC IDs, and missing statistics. The added FE/BE unit cases cover those repaired paths, but not the accepted ORC no-successor edge case.
- Scope and clarity: The changes are focused on the required FE, BE, Thrift, statistics-consumer, and test boundaries and reuse Iceberg's policy/type utilities.
- Concurrency and lifecycle: No new shared mutable concurrency or lock ordering is introduced. Writer close/footer ordering is preserved; disabling metrics skips only optional footer extraction, including the ORC reopen. No static-initialization or ownership issue was found.
- Configuration, compatibility, and parallel paths: Existing Iceberg table properties remain the source of policy. The optional Thrift flag defaults to collection when absent, preserving rolling upgrades. Normal insert, rewrite, and merge paths propagate it, and v3 writer lineage is handled consistently.
- Conditions and data correctness: Missing file metrics remain unknown rather than becoming partial or fabricated statistics. Commit atomicity and persisted table formats are unchanged. The one remaining gap is the ORC-specific upper-bound fallback described inline.
- FE/BE propagation and observability: The flag crosses both table and merge sinks into every partition writer. No additional logs or runtime metrics are needed for this policy-only behavior.
- Performance: Disabled collection avoids unused serialization/footer work. Remaining repeated schema indexing is already covered by an existing review thread, so it was not duplicated.
- Tests and results: Full compile, CheckStyle, both clang-format checks, and vault_p0 pass. I did not run builds or tests locally under the supplied review contract. The current FE UT failure is a truncated dependency download and the macOS BE job stops on a runner JDK mismatch before relevant tests; several other suites are still pending.
- Other issues: After three convergence rounds, all other suspicious points were either dismissed with code evidence or deduplicated against existing threads.
User focus: No additional focus points were supplied; the complete PR was reviewed.
| String stringValue = Conversions.fromByteBuffer(type, value).toString(); | ||
| String truncatedString = lowerBound | ||
| ? UnicodeUtil.truncateStringMin(stringValue, length) | ||
| : UnicodeUtil.truncateStringMax(stringValue, length); |
Contributor
There was a problem hiding this comment.
[P2] Preserve ORC's no-successor upper-bound fallback
This shared helper now applies the Parquet null result to ORC as well. For an ORC string maximum whose first retained code point is U+10FFFF (for example U+10FFFF followed by another character with truncate(1)), UnicodeUtil.truncateStringMax cannot increment the prefix and returns null, so filterBounds drops the upper bound. Iceberg 1.10.1's ORC path deliberately falls back to the original full maximum in this case, unlike its Parquet path; otherwise ORC manifests lose the requested upper metric and pruning/readable-metrics information. Please pass the file format into the bound filtering and retain the original ORC upper string value when no safe truncated successor exists, with an ORC-backed edge test. This is distinct from the prior thread about not truncating bounds at all: it is the ORC-only fallback lost by the new unified fix.
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
nonecountsand safely truncate string/binary bounds fortruncate(N)Root cause
Doris collected column statistics in the backend and copied every statistics map into the Iceberg
DataFilemanifest in the frontend. The conversion never consulted the table'sMetricsConfig, so metadata for disabled columns was persisted even though the physical file statistics were available only as an implementation detail. The initial filtering also treated every non-nonemode likefull, retaining bounds forcountsand failing to safely truncate string/binary bounds fortruncate(N).After disabled metrics were correctly omitted, the downstream Iceberg statistics reader still assumed
columnSizesandnullValueCountsalways contained every column. It could therefore throw while loading statistics for a table using metrics modenone. A scan-close failure could also override an in-flight unknown-statistics return and expose partial or fabricated accumulators. The reader now treats missing required metrics and scan-close failures as unknown.Testing
IcebergWriterHelperTestandStatisticsUtilTest(21 tests passed)git diff --checkLinks