Repository navigation
[fix](fe) Allow table access through Iceberg JDBC V0 catalogs (branch-4.2) - #69485
Conversation
…-4.2) ### What problem does this PR solve? Issue Number: N/A Related PR: apache#69452 Problem Summary: Backport apache#69452 to branch-4.2. 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: 21 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:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
One verified P1 requires changes: the new V0 absence result can turn DROP VIEW c.db.t, DROP TEMPORARY TABLE c.db.t, or DROP MATERIALIZED VIEW c.db.t into a purging drop of the ordinary permanent table db.t. The inline finding traces the complete dispatch and recommends preserving the requested object kind before mutation. No existing inline threads or blocking comment IDs were supplied.
Reviewed every hunk in all three changed files, their discovery/classification/DDL callers, REST default and delegated routing, catalog generation guards, JDBC property forwarding, and the pinned Iceberg 1.11.0 implementation. The SDK guard and purge behavior were checked against the release-tag JdbcCatalog source. No additional user-provided focus was specified.
Critical checkpoint conclusions:
- Goal: ordinary V0 table discovery and reads are unblocked without migrating metadata. Preserving explicit operation-kind restrictions is incomplete because of the specialized DROP finding.
- Scope: one private helper and two probe call sites are focused; related unit and integration checks accompany them.
- Concurrency: no new threads, shared state or locks. Probes remain synchronous within existing authentication and catalog-generation guards; no changed lock order or added work under locks.
- Lifecycle: callbacks are call-local; existing operation guards retain their release behavior. Regression statements, connections, loader and Doris catalogs have cleanup paths; database-first reset removes old role dependencies.
- Configuration: none added. Existing initialization and schema-version properties reach the SDK correctly; the SELECT-only V0 reader does not require metadata migration.
- Compatibility: no public signature, protocol or storage format change; no forced V1 upgrade.
- Thrift/protobuf: untouched; cross-branch field/enum comparisons do not apply.
- Parallel paths: both probe paths and session overloads were traced. V1 success, REST routing, non-JDBC failures and explicit view loads remain intact; the view, temporary-table and materialized-view DROP commands share the reported wrong-kind mutation path.
- Conditional handling: the documented JDBC-only fallback matches the pinned V0 guards. SQL/authorization/connection failures use other exception types and continue to propagate in inspected supported paths.
- Test coverage: all six unit scenarios and the PostgreSQL V0 discovery, SELECT-only access, refresh and historical-read checks were inspected. Missing negative coverage is each specialized DROP command against an existing ordinary permanent table through the writable V0 catalog; it must prove table/data survival, including IF EXISTS.
- Test results: no expected-output files changed. Ordered row assertions and current/historical counts agree with the insertion/snapshot sequence statically.
- Observability: existing unexpected-error propagation and causes remain available; no separate new metric or distributed phase is needed for this helper.
- Transactions/persistence: no EditLog format, replay, transaction machinery or failover handling changed. The reported erroneous deletion nevertheless reaches the existing mutation path.
- Writes/atomicity: no new write protocol was added, but specialized DROP commands can now select the existing purge=true table write; this is the blocking correctness issue.
- FE/BE variables: no new state crosses the boundary.
- Performance: no new SQL call, scan, retained allocation or substantial hot-path cost; V0 SDK rejection occurs before backend work.
- Other core invariants: no BE memory tracking, delete bitmap, rowset version or nullable-column implementation changes. The final missed-issue sweep found no separate confirmed defect.
Validation limits: static review only, as required. No builds, tests, live PostgreSQL/S3 execution or submodule initialization were performed. Successful JDBC view behavior is represented by unit overrides rather than a live V1 view test. All changed paths and identified risks have evidence-backed dispositions; no unresolved review candidate remains.
Review converged after 3 round(s).
| return viewCatalog(ctx).map(activeCatalog -> | ||
| activeCatalog.viewExists(getTableIdentifier(remoteDbName, remoteViewName))).orElse(false); | ||
| return viewCatalog(ctx).map(activeCatalog -> probeViews(activeCatalog, | ||
| () -> activeCatalog.viewExists(getTableIdentifier(remoteDbName, remoteViewName)), false)).orElse(false); |
There was a problem hiding this comment.
[P1] Preserve the requested drop kind before allowing the V0 fallback
With a writable JDBC V0 catalog containing an ordinary permanent table c.db.t, DROP VIEW c.db.t now deletes and purges that table. Env.dropView passes isView=true, but ExternalCatalog.dropTable discards it and invokes IcebergMetadataOps.dropTableImpl. This changed probe converts V0's UnsupportedOperationException to false, selecting performDropTable and catalog.dropTable(id, true) instead of stopping before mutation. DROP TEMPORARY TABLE c.db.t and DROP MATERIALIZED VIEW c.db.t also reach this branch because the same dispatch discards mustTemporary and isMtmv; IF EXISTS does not protect an existing table. Preserve or reject the requested kind/lifetime before invoking the table mutation, and add negative tests on a writable V0 catalog proving these statements leave ordinary table metadata and rows intact.
There was a problem hiding this comment.
Fixed in the latest update and synchronized to #69486.
ExternalCatalog.dropTable now checks the requested object kind and temporary-table lifetime before calling the legacy metadata API. DROP VIEW/DROP MATERIALIZED VIEW cannot delete an existing table, including with IF EXISTS. DROP TEMPORARY TABLE IF EXISTS returns without mutation, while the non-IF EXISTS form reports an unknown table. Reverse table/view mismatches are rejected; matching table and view drops remain supported.
Added eight dispatch tests using the real SDK V0 view guards and a recording writable JDBC catalog. They check table state/rows, purge calls, and delete journaling; five tests failed before the fix and all eight now pass on both branches. Added the six PostgreSQL SQL forms with table-list/data checks after each statement.
Local validation: 29 targeted unit tests pass on branch-4.2 and 27 on branch-4.1, with zero failures/errors/skips; FE Checkstyle passes on both. Regression Groovy syntax was checked; release-branch end-to-end SQL was not run locally. Both PRs have fresh run buildall and /review triggers.
### 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.
### 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.
|
run buildall |
|
/review |
There was a problem hiding this comment.
Static review of #69485 at d4dfc66. One new P2 finding: the shared object-kind check initializes the HMS scan representation, preventing matching DROP TABLE for ordinary nontransactional SequenceFile/Avro tables. The inline comment explains the concrete call chain and missing coverage.
Existing P1 comment 4354683158 was independently revalidated and is fixed for its reported JDBC V0 permanent-table scenario. Requested view/materialized-view/temporary kinds are now rejected or treated as a no-op before table mutation and journaling, including IF EXISTS. It is not reposted or retained as an existing blocking finding. No additional user focus was supplied.
The JDBC probe change restores V0 table discovery without requiring migration. V1 view support, default/delegated REST routing, explicit view-load errors, and SQL/authorization failure propagation were traced. The pinned Iceberg 1.11.0 JdbcCatalog source confirms V0 guards run before SQL; no concrete additional supported V1 failure swallowed by the catch was found.
Critical-checkpoint conclusions:
- Goal and proof: V0 table access and wrong-kind safeguards follow the intended paths; six probe unit tests, eight drop-dispatch unit tests, and the PostgreSQL V0 regression were inspected. Matching HMS drops regress as reported inline.
- Scope and clarity: the helper and guards are focused, but the common kind check needs to avoid connector scan validation.
- Concurrency and locks: no threads or helper state added. Existing synchronized lazy initialization and catalog-generation guards remain; the new HMS initialization adds remote work to DROP. No additional lock-order defect was substantiated.
- Lifecycle and static initialization: no production resource ownership or static-initialization changes. Existing generation guards close on exceptions; regression connections/statements/classloader use finally cleanup.
- Configuration: no new production setting; tests use existing JDBC schema/init properties.
- Compatibility: no public API, wire, storage-format, or journal-format change. V0 remains V0 and V1 remains view-capable; the matching HMS behavior change is the inline finding.
- Thrift/protobuf: no definitions changed; release-branch field/enum comparison is not applicable.
- Parallel paths: Iceberg, HMS, Paimon, MaxCompute, and default/delegated REST paths were examined. HMS is affected by the finding. MaxCompute view-drop and stale Iceberg kind hypotheses did not establish a new regression and were dismissed.
- Conditions and errors: capability and kind/lifetime conditions are commented; genuine supported SQL/access failures propagate, and IF EXISTS does not relax an existing object's kind mismatch.
- Coverage: new tests cover negative and matching kinds, temporary variants, missing IF EXISTS, V0 probes, successful view-enabled JDBC behavior, and propagated failures. The dispatch tests mock isView and miss real HMS initialization. Live JDBC/HMS/REST behavior was not executed.
- Expected results: no .out files changed; new multivalue checks are ordered, and initial-snapshot/current-versus-historical counts are consistent statically.
- Observability: standard kind errors carry object names and DROP guidance; existing failure wrappers/logging remain. No additional metric requirement identified for this capability fallback.
- Persistence and failover: guard rejection/no-op precedes remote mutation, post-drop hooks, and EditLog. Successful paths keep existing DropInfo/replay behavior; no new durable state or failover protocol.
- Writes and atomicity: no write-transaction/crash-recovery change. The reported V0 wrong-kind purge is fenced; the new HMS failure occurs before deletion. Existing remote replacement/drop-versus-journal windows are unchanged.
- FE/BE variables: none added; no new propagation path.
- Performance: no new unbounded scan in the probe helper. The extra full HMS initialization/metadata fetch is part of the reported regression; no separate verified hot-path defect found.
- Other issues: all five changed files, upstream SQL flag producers, real table kinds, downstream mutation/cache retirement/journaling, fixture privileges, refresh, snapshot determinism, and special-case paths received a final static sweep. No additional verified finding or unresolved concrete candidate remains.
Review scope is static only: no builds, tests, source modifications, or submodule initialization were performed. End-to-end SQL and runtime concurrency remain unexecuted. Convergence describes completion of this bounded static review, not runtime validation.
Review converged after 2 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 the object kind without requiring a readable HMS table
This shared dispatch also handles HMS catalogs, where HMSExternalTable.isView() calls makeSureInitialized() and supportedHiveTable(). For an ordinary nontransactional Hive table stored as SequenceFile (or Avro), that initialization throws NotSupportedException because its input format is not supported for scanning. Consequently, a matching DROP TABLE hms.db.t (including IF EXISTS) now fails here before HiveMetadataOps.dropTableImpl can delete the table; previously that path only required existence and a nontransactional table. Resolve the table/view kind from lightweight remote metadata without validating scan support, and add a dispatch test using a real HMS table with an unsupported input format. The current tests mock isView(), so they miss this regression.
FE Regression Coverage ReportIncrement line coverage |
3 similar comments
FE Regression Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
### What problem does this PR solve? Issue Number: None Related PR: apache#69408, apache#69485 Problem Summary: Merge the latest branch-4.2 into the existing fix branch. Resolve the IcebergMetadataOps import conflict by retaining FileIO and LocationProvider for guarded column drops and JdbcCatalog for the upstream JDBC V0 view probes. Preserve both implementations without introducing new behavior. ### Release note None ### Check List (For Author) - Test: Unit Test: 125 passed, zero failures/errors/skips: IcebergColumnDropTest, IcebergRestSchemaCommitTest, IcebergMetadataOpsValidationTest, IcebergJdbcViewProbeTest, ExternalCatalogDropKindTest; FE Checkstyle passed (zero violations) - Behavior changed: No (merge resolution only) - Does this need documentation: No
|
PR approved by anyone and no changes requested. |
1 similar comment
|
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. |
What problem does this PR solve?
Issue Number: N/A
Related PR: #69452
Problem Summary:
Backport #69452 to branch-4.2. 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)