924060929 commented on PR #66529:
URL: https://github.com/apache/doris/pull/66529#issuecomment-5211750317
Thanks for the thorough PR — I reviewed the FE side end-to-end (scan plan
provider, metadata pinning, and the nested-partition write path). Overall the
changes are correct and the conservatism is deliberate and test-backed; I
verified the FE→BE protocol consistency for `source_field_path` (Doris children
order == `DataTypeStruct` element order == Iceberg struct field order, with
BE-side bounds checks), the 3-arg `getColumnHandles` fallback matching
`getTableSchema`/`pinnedSchema`, and the retry-loop scope reset semantics. One
issue I'd rate P1:
## P1: `applicableEqualityDeleteFieldIds` enumerates every delete manifest
on every query — no pruning, no early exit, wider failure surface
`IcebergScanPlanProvider.getScanNodeProperties` →
`applicableEqualityDeleteFieldIds` now runs on **every** non-system scan of any
table whose snapshot summary `total-equality-deletes` is non-zero:
```java
for (ManifestFile manifest : snapshot.deleteManifests(table.io())) {
...
try (ManifestReader<DeleteFile> reader =
ManifestFiles.readDeleteManifest(
manifest, table.io(), table.specs())) {
for (DeleteFile deleteFile : reader) { // fully read, no early exit
if (deleteFile.content() == FileContent.EQUALITY_DELETES) {
fieldIds.addAll(deleteFile.equalityFieldIds());
}
}
} catch (IOException e) { throw ... }
}
```
Compared to the old `scan.planFiles()` walk (partition-pruned, lazy, returns
at the **first** equality delete), this:
1. **Per-query planning cost**: opens and fully reads *every* delete
manifest of the snapshot on every query (regardless of filter/partition
pruning), with the manifest cache default-off
(`meta.cache.iceberg.manifest.enable`), so no caching mitigates it. For
delete-heavy tables (many small delete batches → thousands of delete
manifests), that's thousands of remote manifest file reads added to every
query's planning, including `count(*)` and narrowly-filtered scans that never
touch them.
2. **Wider failure surface**: any unreadable/corrupt delete manifest — even
one in a fully-pruned partition irrelevant to the query — now fails the whole
scan with `DorisConnectorException`; the old code only read manifests for
planned tasks.
3. **Widened upgrade fence**: `hasApplicableEqualityDeletes` changed
semantics from "the planned tasks carry equality deletes" to "any live equality
delete exists in the snapshot", so `REQUIRED_CURRENT_BACKEND_SEMANTICS` now
gates every query on such tables during rolling upgrades (tests confirm this is
intentional, but the availability cost is not quantified).
Suggested fixes (any one):
- **Early exit**: the carrier only needs historical fields when an equality
field id is missing from `scanSchema` — once every id seen so far is present in
`scanSchema`, stop reading manifests (carrier = `scanSchema.columns()`).
- Or route the delete-manifest reads through the manifest-cache path.
- At minimum, degrade gracefully instead of failing the whole query when an
irrelevant delete manifest is unreadable.
## P2 (worth addressing)
- **`resolveSourceFieldPath` silently treats `uniqueId < 0` as top-level**
(`PhysicalExternalRowLevelMergeSink`): for a nested source, a top-level column
without a carried field id yields an empty path — the whole struct becomes the
partition source, and the BE would write garbage partition values with no
error. Unreachable for Iceberg today (ids are always stamped via
`ConnectorColumnConverter.applyNestedFieldIds`), but this is a
silent-wrong-data channel if the column tree is ever built by a path that skips
id stamping. Suggest hard-failing there (as the `findSourceFieldPath` miss
does) instead of assuming top-level.
- **Nested partition sources on the MERGE/UPDATE path still hard-fail**:
`columnIdToExprId` only contains top-level column ids, so a nested source id
lookup fails → random fallback. Safe (data stays correct), but the routing fix
only applies to the INSERT-style branch — worth confirming whether that's
intended and documenting it.
- **The requiredness fence no longer proves snapshot ancestry**
(`selectedHistoryRequiresMissingRequiredFieldRejection`): any historical schema
with the hazard now gates the selected snapshot. No false negatives, but the
false-positive surface is much larger than the old lineage-proving walk; the
rolling-upgrade availability trade-off could be quantified in the PR
description.
Everything else I checked held up: the 3-arg
`getColumnHandles`/`getTableSchema`/`pinnedSchema` fallbacks stay mutually
consistent (the same-name-different-id corner is guarded only by name, but
schema ids never expire in Iceberg so it's not reachable), `requestsColumn`'s
empty-list→fence is conservative, `toThrift` only sets the new optional path
when non-empty (BE `__isset`-gated), and the retry-loop
`resetConnectorStatementScope` is a no-op on the first attempt and only closes
already-discarded attempt state.
--
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]
