Repository navigation
[fix](fe) Allow table access through Iceberg JDBC V0 catalogs (branch-4.1) - #69486
Conversation
…-4.1) ### What problem does this PR solve? Issue Number: N/A Related PR: apache#69452 Problem Summary: Backport apache#69452 to branch-4.1. Iceberg's JDBC catalog implements `ViewCatalog` even when its V0 metadata schema does not support views. Unsupported view probes must not prevent ordinary table listing or reads. Adapt the fix to the release branch's legacy `IcebergMetadataOps` implementation. Return no views only for unsupported JDBC view probes, preserving V1 view support, REST view routing, explicit view-operation failures, and real SQL/authorization errors. Port the six unit-test scenarios to the legacy metadata API and retain the original PostgreSQL V0 SELECT-only regression setup and read/time-travel checks. ### Release note Fix table discovery and access through Iceberg JDBC catalogs using the V0 metadata schema without view support. ### Check List (For Author) - Test - [x] Unit Test: 19 tests passed (six new JDBC probe tests and the existing metadata-operation tests), zero failures/errors/skips. FE Checkstyle passed. Before the fix, both V0 tests reproduced the actual SDK unsupported-view exception. - [ ] Regression test: V0 scenarios ported unchanged from apache#69452; release-branch end-to-end execution is left to CI. - Behavior changed: - [x] Yes. Unsupported JDBC view probes no longer block ordinary table operations. - Does this need documentation? - [x] No. ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label Adapted to the legacy metadata implementation. (cherry picked from commit 57e41e9)
|
run buildall |
|
/review |
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
Codex automated review failed and did not complete. Error: Review context preparation failed before Codex ran; inspect the 'Prepare authoritative PR context and required AGENTS guides' step. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
### What problem does this PR solve? Issue Number: N/A Related PR: apache#69485, apache#69486 ExternalCatalog discarded the requested view, MTMV and temporary-table flags before calling the legacy metadata API. Once JDBC V0 view probing returns false, a wrong-kind DROP could purge an ordinary permanent table. Validate kind and lifetime before dispatch, preserve valid drops, and avoid mutation/journaling for rejected or no-op requests. ### Release note Prevent wrong-kind external DROP statements from deleting table data. ### Check List (For Author) - Test: 27 targeted unit tests passed; five new tests failed before the fix. FE Checkstyle passed. Negative SQL regression added and Groovy syntax checked; release-branch end-to-end SQL not run locally. - Behavior changed: Yes. Reject mismatched DROP types and protect permanent tables from temporary drops. - Does this need documentation: No.
### What problem does this PR solve? Issue Number: N/A Related PR: apache#69485, apache#69486 ExternalCatalog discarded the requested view, MTMV and temporary-table flags before calling the legacy metadata API. Once JDBC V0 view probing returns false, a wrong-kind DROP could purge an ordinary permanent table. Validate kind and lifetime before dispatch, preserve valid drops, and avoid mutation/journaling for rejected or no-op requests. ### Release note Prevent wrong-kind external DROP statements from deleting table data. ### Check List (For Author) - Test: 29 targeted unit tests passed; five new tests failed before the fix. FE Checkstyle passed. Negative SQL regression added and Groovy syntax checked; release-branch end-to-end SQL not run locally. - Behavior changed: Yes. Reject mismatched DROP types and protect permanent tables from temporary drops. - Does this need documentation: No.
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Static review of PR #69486 at cd815f7. Three P2 regressions are reported inline: matching HMS drops now depend on scan-format support, failed Iceberg kind initialization can block a matching DROP VIEW retry, and stale HMS name caches can turn remote-missing IF EXISTS drops into errors. No existing inline findings or additional user focus were supplied.
The JDBC V0 fallback itself is consistent with the pinned SDK: unsupported view probes become empty/false, while V1 probes, REST view gating, explicit view operations, and SQL/authorization errors retain their paths. Checked against Iceberg 1.11.0 JdbcCatalog.
Critical checkpoints:
- Goal and evidence: V0 table access and wrong-kind DROP protection are covered by the new unit/regression assertions; the three object-classification regressions need correction. Tests were inspected, not executed.
- Scope and clarity: production changes are small and focused, but placing the kind check in ExternalCatalog affects all writable connectors, including HMS.
- Concurrency and locks: DDL/query workers may overlap refresh and external clients. Existing table initialization monitors and Iceberg catalog-generation guards remain; the helper adds no shared state, threads, locks, or new lock order. Remote name replacement is an existing non-atomic boundary.
- Lifecycle: cached table shells outlive failed commands. The new guard exposes Iceberg's early objectCreated publication, as detailed inline. No new ownership cycle or static-initialization dependency was introduced.
- Configuration: no configuration items are added. The regression uses existing JDBC schema-version and initialization properties, correctly normalized to SDK keys.
- Compatibility: no storage, image, journal, protocol, or public signature changes. V0 access does not require a V1 migration; existing V1 paths remain enabled.
- Thrift/protobuf: no definitions changed; release-branch field/enum comparisons are inapplicable.
- Parallel paths: reviewed HMS, Iceberg, Paimon and MaxCompute writable dispatch, InternalCatalog semantics, both view probes, REST-enabled/disabled paths, explicit view operations, case-fold collision checks and forced database drops. No additional new defect was substantiated.
- Conditions: TEMPORARY and MTMV checks run before mutation; IF EXISTS does not suppress an existing-object kind mismatch. Parser/Env flags and ErrorCode arguments match the intended SQL forms.
- Coverage: unit tests exercise real V0 SDK guards, synthetic positive view support, error propagation, matching/wrong-kind drops and mutation/journal boundaries. The PostgreSQL V0 regression checks read-only discovery, reads, snapshots, refresh and all six wrong-kind forms. Mocked isView and null-shell absence tests miss the real classification and stale-name cases reported inline; real JDBC V1 view end-to-end coverage is not added.
- Results: no .out files changed. New row comparisons use ORDER BY; aggregate counts and the single initial snapshot are deterministic. No incorrect new expected value was found statically.
- Observability: kind errors identify database/table and requested kind. Existing SDK causes and successful-drop logs remain sufficient for these branches; no new metric is needed.
- Persistence and failover: the new validation rejections and early returns stop before remote mutation and journaling. Existing connector-level IF EXISTS no-ops still run cache retirement and journal hooks. Successful drops retain connector mutation, cache retirement and DropInfo logging/replay. No persisted field changes; the remote-mutation/journal crash interval predates this PR.
- Writes, atomicity and crashes: the guards add validation before existing destructive APIs. No new transaction protocol is introduced. Existing remote replacement/crash windows remain; no new leak or deadlock was established.
- FE/BE propagation: no new FE-to-BE variable or send path.
- Performance and memory: the helper has call-local state and bounded overhead; V0 rejects probes before SQL. Cold kind checks can add a metadata lookup. No new unbounded allocation or BE MemTracker concern; HMS scan-format initialization is unnecessary for DDL and causes the first finding.
- Additional concerns and limits: all five changed files and their relevant callers/consumers were reviewed, including a separate SDK/routing risk pass. There is no additional user-provided focus. This is static-only validation: no builds, test execution, source changes, or submodule initialization. Runtime pass rates, fixture reruns and real V1 view end-to-end behavior remain unverified.
Review converged after 3 round(s).
| ErrorReport.reportDdlException(ErrorCode.ERR_WRONG_OBJECT, dbName, tableName, "MTMV", | ||
| "External catalogs do not support materialized views"); | ||
| } | ||
| boolean actualIsView = dorisTable.isView(); |
There was a problem hiding this comment.
[P2] Check HMS object kind without validating its scan format
For a nontransactional Hive table stored as Avro (also RCFile or SequenceFile), a matching DROP TABLE hms.db.t now fails here: HMSExternalTable.isView() calls makeSureInitialized(), which calls supportedHiveTable() and throws Unsupported hive input format before reaching the drop API. Previously getTableNullable() only constructed the shell, and HiveMetadataOps.dropTableImpl() checked existence/transactional status without requiring a readable format. Thus this shared guard removes a working metadata operation for tables Doris cannot scan. Resolve the kind from raw metastore metadata without scan-format validation, and cover this with a real HMS table rather than a mocked isView().
| "External catalogs do not support materialized views"); | ||
| } | ||
| boolean actualIsView = dorisTable.isView(); | ||
| if (isView != actualIsView) { |
There was a problem hiding this comment.
[P2] Keep failed Iceberg kind probes retryable
For an existing V1 view with a cold IcebergExternalTable shell, let the first DROP VIEW encounter a transient SQL or metadata-read error in viewExists. IcebergExternalTable.makeSureInitialized() sets objectCreated = true before assigning isView, so the failure leaves the cached shell initialized with isView = false. No drop hook invalidates it. After the backend recovers, retrying the matching DROP VIEW now hits this mismatch and reports is not VIEW until refresh or cache expiry. Before this change, dropTableImpl() probed the remote kind afresh and the retry could succeed. Publish kind initialization only after a successful probe, or obtain reliable fresh classification here; add a failure-then-retry test using the real shell.
| ErrorReport.reportDdlException(ErrorCode.ERR_WRONG_OBJECT, dbName, tableName, "MTMV", | ||
| "External catalogs do not support materialized views"); | ||
| } | ||
| boolean actualIsView = dorisTable.isView(); |
There was a problem hiding this comment.
[P2] Preserve IF EXISTS for stale HMS table names
Populate Doris' HMS table-name cache (for example with SHOW TABLES), delete t directly in Hive, then run DROP TABLE IF EXISTS hms.db.t before the name cache refreshes. getTableNullable() can still construct a cold shell from the cached name, so the null check above does not handle this case. The new HMSExternalTable.isView() call loads the now-missing table and throws HMSClientException before HiveMetadataOps.dropTableImpl() can perform its existing tableExists == false && ifExists no-op. This previously successful idempotent drop now fails. Preserve remote-missing IF EXISTS semantics during kind resolution, and test a stale name with a real HMS shell rather than only mocking a null lookup.
FE Regression Coverage ReportIncrement line coverage |
1 similar comment
FE Regression Coverage ReportIncrement line coverage |
|
PR approved by anyone and no changes requested. |
github-actions
Bot
added
the
approved
|
PR approved by at least one committer and no changes requested. |
3 participants
What problem does this PR solve?
Issue Number: N/A
Related PR: #69452
Problem Summary:
Backport #69452 to branch-4.1. Iceberg's JDBC catalog implements
ViewCatalogeven when its V0 metadata schema cannot store views. Treat unsupported JDBC view probes as no views so ordinary table listing and reads work without requiring a V1 migration. Preserve V1 view support, REST view routing, explicit view-operation errors, and real SQL/authorization failures.Preserve the requested DROP kind and lifetime before invoking the legacy metadata API, which accepts none of these flags and can purge an ordinary table. Reject
DROP VIEWandDROP MATERIALIZED VIEWon a table, includingIF EXISTS. An external permanent table cannot satisfyDROP TEMPORARY TABLE: return without mutation forIF EXISTS, otherwise report an unknown table. RejectDROP TABLEon a view while preserving matching table/view drops and missing-objectIF EXISTSbehavior.Add eight drop-dispatch tests using the actual JDBC V0 view guards and a recording writable catalog. Assert that rejected/no-op statements preserve table state and rows without purge or delete journaling. Extend the PostgreSQL V0 regression with all six wrong-kind SQL forms and check table listing and data after every statement.
Release note
Fix table access through Iceberg JDBC V0 catalogs. Prevent wrong-kind external DROP statements from deleting existing table data.
Check List (For Author)
Check List (For Reviewer who merge this PR)