Doris-Breakwater commented on issue #68035: URL: https://github.com/apache/doris/issues/68035#issuecomment-5685311907
Breakwater-GitHub-Analysis-Slot: slot_4acf752c01c8 ## Initial triage **Assessment: confirmed and actionable at `28577df9f6d`; the existing labels `kind/fix` and `area/iceberg` are appropriate.** This should be treated as a high-priority correctness issue for Iceberg REST catalogs, because the failure mode can be a successful query with rows that the server-planned tasks would have filtered, rather than only a misleading error. Restoring the existing fail-closed behavior should be separated from full server-side planning support in #68036. I reviewed the exact Doris sources and the Iceberg 1.11.0 implementation. I did not independently rerun the Docker fixture, so the end-to-end row and request counts below remain reporter-supplied evidence; the code mechanism itself is source-verified. ### Verified facts - Both snapshot paths replace a `BaseTable` subclass with a new plain `BaseTable` while retaining only snapshot-pinned operations and metadata: [`IcebergStatementScope.snapshotReadTable`](https://github.com/apache/doris/blob/28577df9f6df3446f7fd1d0c90032fb7d85c7733/fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergStatementScope.java#L257-L267) and [`IcebergTableCache.TableOwner.snapshotReadTable`](https://github.com/apache/doris/blob/28577df9f6df3446f7fd1d0c90032fb7d85c7733/fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergTableCache.java#L284-L295). - Iceberg 1.11.0's `RESTTable` overrides `newScan()` to create `RESTTableScan` and overrides `allowDistributedPlanning()` to return `false`. Reconstructing it as `BaseTable` therefore changes both scan behavior and the capability value. - Small wording correction: Iceberg 1.11.0's plain `BaseTable` **does implement** `SupportsDistributedScanPlanning`; it inherits the interface default `allowDistributedPlanning() == true`. Thus the wrapper loses the REST-specific `false` override, not the marker interface itself. The guard still returns normally, so this correction does not change the reported diagnosis. - [`resolveTable`](https://github.com/apache/doris/blob/28577df9f6df3446f7fd1d0c90032fb7d85c7733/fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergScanPlanProvider.java#L3082-L3123) and the metadata-table base resolution perform the unsupported-mode check only after the statement/cache acquisition may have produced the plain `BaseTable`. The later `streamingSplitEstimate()` call reads `snapshot.dataManifests(table.io())`, matching the reported local-manifest/403 stack. - The existing `serverPlannedTable` test helper is a dynamic proxy, not a `BaseTable`. Both snapshot methods therefore return it unchanged, so those tests cannot detect this regression with an actual SDK `RESTTable`. - The affected acquisition branches are not all identical: cached borrows and direct/unscoped snapshot conversion lose the override, while a statement-owned tracked-table branch currently returns its raw table. This makes a shared boundary check preferable to relying on a later scan-only check. The supplied controls are strong: the raw-table guard succeeds, both wrapped-table guards miss, the SDK reader issues `/plan` and enforces the injected residual, Doris issues no `/plan`, and the restricted-storage stack identifies local manifest access. Together with the source trace, this is sufficient to accept the issue without requesting a query profile or more general environment logs. ### Evidence limits / information still useful No additional information is blocking initial triage. Two items should be resolved during implementation validation: 1. Record explicit cache settings and cover cache enabled, cache disabled, and statement-scope `NONE`, since the acquisition branches differ. 2. Verify the weight-bounded cache path before choosing check placement. `TableOwner` may serialize/estimate metadata before `snapshotReadTable()` is called. The issue correctly identifies this as an audit target, but the supplied reproduction does not establish that this serialization itself reads a manifest. Treat that as a risk to test, not yet a verified cause. There is no evidence here for asynchronous planning, delete-file handling, credential vending, or any production catalog's authorization policy. Those should not be inferred from the controlled residual-filter proxy. ### Recommended next steps 1. Accept a focused PR that restores rejection. Extract one package-level helper for the `SupportsDistributedScanPlanning && !allowDistributedPlanning()` check and apply it to the original SDK table before any capability-destroying snapshot conversion or possible local-manifest-dependent cache work. 2. Preserve ownership on rejection. In the cache path, ensure any acquired load guard, catalog lease, table cleanup/FileIO ownership, and statement borrower are released if the new early check throws. Keep the existing scan-entry guard as defense in depth. 3. Preserve current snapshot isolation for client-planned tables; returning the mutable raw `RESTTable` unconditionally would change statement consistency and is not an adequate fix. 4. Add regression coverage using tables loaded through a real Iceberg REST catalog, not only an interface proxy. Cover client-configured server mode and server table override, cache enabled/disabled, populated and empty tables, metadata-table base resolution, and cleanup on rejection. Keep client-planned and snapshot-isolation tests unchanged. 5. Re-run both end-to-end controls. For this bug-fix PR, both should fail with the explicit unsupported-mode error before local manifest access; the policy proxy should receive no `/plan`. Successful consumption of server tasks, residuals, and scan-scoped credentials belongs to #68036. Recommended disposition: confirm the bug and invite the reporter's offered PR, with the capability wording correction and lifecycle/cache-ordering checks above included in review. -- 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]
