Repository navigation
[fix](fe) Skip table snapshot resolution for external views - #69836
Conversation
### What problem does this PR solve? Related PR: apache#69829 Problem Summary: External views resolve a table handle during statement snapshot loading before their SQL is expanded. A catalog can serve a view while rejecting table requests for the same name. Skip table snapshot materialization for known views and keep normal snapshot pinning for their base tables. ### Release note Avoid table metadata requests for external views during snapshot binding. ### Check List (For Author) - Test: 138 FE unit tests passed; Checkstyle passed for both changed Java files; Polaris E2E not run. - Behavior changed: Yes, known views skip table snapshot resolution. - Does this need documentation: No.
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
FE UT Coverage ReportIncrement line coverage |
TPC-H: Total hot run time: 28980 ms |
TPC-DS: Total hot run time: 151119 ms |
ClickBench: Total hot run time: 24.4 s |
FE Regression Coverage ReportIncrement line coverage |
|
/review |
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
/review |
There was a problem hiding this comment.
Reviewed PR 69836 at head cd2facafd8566957976f251b248cfc4d41dd0f62. The plain external-view path now avoids table-handle resolution before view expansion; the empty pin is safe for that path because BindRelation expands the view and schema warmup uses the view definition. No new lock, protocol, persistence, or lifecycle issue was found, and there are no existing inline blocking findings or additional user focus points.
One correctness issue is reported inline: relation-scoped @branch/@tag selectors are silently dropped for plugin views, so an Iceberg view reference can read the default/latest view body instead of rejecting or honoring the requested ref. The added unit test covers the three snapshot overloads but does not cover this selector path.
This was a static-only review; builds and tests were not run. Convergence is false at the round-3 cap because the required independent subagent did not return after repeated model-capacity failures; the main-agent coverage and verified finding are still delivered per the bounded contract.
Review did not converge after 3 rounds. This automated review cannot pass the review check.
Formal convergence could not be established because the required independent review subagent failed to return after repeated model-capacity errors. Main-agent coverage is complete through the round-3 final sweep; one verified inline finding is delivered.
| Optional<MvccSnapshot> latestSnapshotFence) { | ||
| // Views have no table snapshot. BindRelation expands their SQL and pins the base tables; | ||
| // resolving a table handle here would load the view name through the table API. | ||
| if (isView()) { |
There was a problem hiding this comment.
[P1] Preserve or reject view @branch/@tag selectors: BindRelation calls loadSnapshots before expanding a plugin view, and its view guard rejects only TableSnapshot. With this early return, Iceberg @branch/@tag never reaches resolveTimeTravel, and the returned LogicalSubQueryAlias carries no selector, so SELECT ... FROM view@branch('b') silently reads the default/latest view body. Reject these selectors for views (as for FOR VERSION/TIME) or propagate them to the expanded base relations; do not drop them.
FE Regression Coverage ReportIncrement line coverage |
### What problem does this PR solve? Related PR: #69836 Selecting an external Iceberg REST view can fail during query binding with `BadRequestException: ... is not an Iceberg compatible table`, even when the catalog supports reading the view definition. `BindRelation` loads statement snapshots before expanding the view SQL. The snapshot loader calls `PluginDrivenExternalTable.resolveConnectorTableHandle`, which unconditionally asks the connector for a table handle using the view name. A REST catalog can reject that table request with HTTP 400 instead of reporting a missing table. The Iceberg connector only converts `NoSuchTableException` to an empty handle, so the error escapes before the existing view-existence fallback can run. Resolve the object's view classification before requesting a table handle. Known views return no table handle and retain the live view-existence check, so a view deleted outside Doris still invalidates its cached lookup. Snapshot loading can then return an empty view pin and continue to view expansion; the referenced base tables keep their normal snapshot handling. Ordinary table handles and table-load errors retain their existing behavior. This addresses the shared handle-resolution boundary, including callers outside the snapshot loader. #69836 proposes skipping the snapshot-loader entry instead. ### Release note Fix external view queries failing when an Iceberg REST catalog rejects table metadata requests for view names. ### Check List (For Author) - Test - [ ] Regression test - [x] Unit Test - [ ] Manual test - [ ] No need to test or manual test. - Behavior changed: - [ ] No. - [x] Yes. External views no longer issue a table-handle request during resolution. - Does this need documentation? - [x] No. - [ ] Yes. Validation: - `run-fe-ut.sh --run` with `PluginDrivenExternalViewHandleTest`, `PluginDrivenExternalTableTest`, `PluginDrivenMvccExternalTableTest`, and `PluginDrivenMissingTableTest`: **164 tests passed**, with no failures, errors, or skips. - Before the fix, both view cases failed with the reproduced table-endpoint error; the ordinary table cases passed. - `mvn checkstyle:check -pl fe-core`: **0 violations**. - `git diff --check` passed. No live Databricks integration test was run.
### What problem does this PR solve? Related PR: apache#69836 Problem Summary: The external-view snapshot shortcut ignores relation scan parameters such as @Branch and @tag. BindRelation then expands the default view SQL without honoring or rejecting the requested selector. Reject scan parameters before returning the empty view snapshot, while retaining the ordinary-view shortcut and existing base-table pinning. ### Release note Reject unsupported scan parameters on external views instead of silently ignoring branch, tag, incremental-read, or options selectors. ### Check List (For Author) - Test: Checkstyle passed for both changed Java files. Negative unit coverage added for both loadSnapshot overloads and latest-fence calls; not run locally in accordance with the requested validation scope. - Behavior changed: Yes, unsupported scan parameters on views produce an error. - Does this need documentation: No.
|
run buildall |
|
/review |
There was a problem hiding this comment.
Reviewed PR 69836 at head 4a08890b18f91ee9cfc07b1e0b5b3d9f6773ea4a against base 2a4191c1dc94e789e1725d6cb345f46c31a17272. The change meets its stated goal: every loadSnapshot overload reaches the new view guard, plain external views receive an empty pin without table-handle resolution, and any relation TableScanParams (@branch, @tag, @incr, or @options) is rejected before connector time-travel resolution. BindRelation separately rejects FOR VERSION/TIME AS OF on plugin views, and the added unit tests cover all selector kinds plus the overloads with and without a latest fence.
Critical checkpoints: data/version correctness is preserved because the view is expanded as a LogicalSubQueryAlias and the expanded base relations retain their own MVCC pins; the empty view pin is not used by PluginDrivenScanNode as a physical scan. The synchronized isView initialization/reset lifecycle and selector-keyed StatementContext paths were traced with no new lock, persistence, protocol, configuration, or lifecycle issue. Functional parallel paths and schema-cache fallback were reviewed. The changed tests were inspected but not executed, consistent with this static-only review.
The existing inline P1 comment 4421581161 was independently revalidated and dismissed for this head: the current scanParams.isPresent() branch now throws before a selector can be silently dropped, so it is not included in existing_blocking_comment_ids. No additional user focus points were supplied.
No verified new findings. Review convergence was reached in round 1 after the main risk scan and two independent subagent reviews returned NO_NEW_VALUABLE_FINDINGS.
Review converged after 1 round(s).
TPC-H: Total hot run time: 28695 ms |
TPC-DS: Total hot run time: 151099 ms |
ClickBench: Total hot run time: 24.25 s |
924060929
left a comment
There was a problem hiding this comment.
External views execute by expanding their SQL and binding the underlying tables. Previously, snapshot loading could resolve the view name through the table metadata API before expansion. This change returns an empty view pin and rejects unsupported scan parameters while preserving independent snapshot pinning for the expanded base tables.
Independent static review of head 4a08890 against base 2a4191c found no blocking FE issues. I traced the shared snapshot entry, statement snapshot maps, view expansion, schema fallback, and related tests. The selector-loss issue in discussion_r4421581161 is fixed by the current scanParams rejection. APPROVE within the FE scope.
No builds or tests were run during this review. The supplied current-head CI snapshot reports successful P0, FE UT, external regression, compilation, and CheckStyle checks, but check_coverage_fe failed. Required-check policy was not supplied; this approval does not establish merge readiness.
Automated FE-focused static review (gpt-6.1-sol, high). No build or tests were run.
|
PR approved by anyone and no changes requested. |
|
PR approved by at least one committer and no changes requested. |
What problem does this PR solve?
External views currently resolve a table handle when loading the statement snapshot, before their SQL is expanded. Skip that table-only path for known views; their base tables retain normal snapshot pinning.
Reject unsupported view scan parameters such as
@branchand@taginstead of silently ignoring the requested selector.Release note
Avoid table metadata requests for external views during snapshot binding, and reject unsupported view scan parameters.
Check List (For Author)
For this update: Checkstyle passed for both changed Java files. Selector-rejection unit tests were added but not run locally.
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)