feat(iceberg): add external Iceberg connector#25368
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
Add the external Iceberg connector foundation across SQL parsing, catalog metadata, scan planning, execution, append/DML writes, maintenance procedures, residency security, object IO, and MOI-facing ref/job plumbing. Include generated parser/proto updates, bootstrap system tables, unit/integration harnesses, local Nessie/MinIO/Spark test assets, and CI workflow coverage for Iceberg connector validation.
Preserve hidden Iceberg DML metadata columns through insert construction, request row ordinals for position-delete writes, and align writer metadata indexes with the actual input projection. Also guards local Iceberg test artifacts from accidental staging.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Requesting changes for several blockers:
-
pkg/frontend/authenticate.go:6361-6364, 6604-6610, 9798-9799+pkg/frontend/iceberg.go:30-235CREATE/ALTER/DROP ICEBERG CATALOGandSHOW ICEBERG *are classified asprivilegeKindNone, and the privilege engine short-circuits success for that kind. The handlers then directly mutate/readmo_iceberg_*with no compensating admin check. Any ordinary user in an account can reconfigure/drop account-wide catalogs and enumerate catalog URIs/namespaces/table mappings.- Suggested fix: require
accountadmin/moadmin(or a dedicated privilege) for all Iceberg catalog DDL and metadata-show statements.
-
pkg/frontend/iceberg.go:338-351,pkg/frontend/iceberg_call.go:210-262,pkg/sql/iceberg/ddl.go:30-45,62-75,pkg/sql/iceberg/dao.go:636-642,pkg/sql/iceberg/residency.go:159-170catalog_idis only unique within an account (max(catalog_id)+1 where account_id = ...), but cluster-scoped residency policies are stored withaccount_id=0and matched on barecatalog_id. A cluster policy created for account As catalog1will also be evaluated for account Bs catalog1, which breaks tenant isolation.- Suggested fix: make cluster policy identity include the owning account (or switch to a globally unique catalog identifier/name/URI) and enforce lookups on that full identity.
-
pkg/iceberg/metadata/object_reader_factory.go:145-156,pkg/iceberg/io/object_io_registry.go:29-66,79-109,pkg/sql/colexec/external/parquet.go:237-240- The remote-signing path registers
ObjectIORefs withttl=0, which becomes the global 30-minute default. File opens resolve that ref lazily per file, so any long scan/DML/maintenance flow that is still opening files after 30 minutes starts failing with “object IO ref is not registered or expired”. On top of that, production code never callsReleaseObjectIORef, so refs also accumulate indefinitely in the global registry. - Suggested fix: tie ObjectIORef lifetime to query/operator teardown instead of the fixed 30m default for remote-signing flows, explicitly release refs on teardown, and add opportunistic/background sweeping for abandoned entries.
- The remote-signing path registers
-
pkg/sql/iceberg/dml_executor.go:51-93,pkg/sql/iceberg/dml_delete_builder.go:133-159,391-447,pkg/iceberg/dml/workflow.go:75-149,255-295- The DML builders materialize replacement/delete objects before the commit workflow (and its orphan recorder) starts. If a later build/validation step fails, the executor returns immediately and never reaches the only orphan-recording path, leaving uncommitted files in table storage with no cleanup record.
- Suggested fix: do not write objects until a workflow/orphan tracker is active, or register/clean up every materialized object if action-stream construction fails.
-
pkg/sql/colexec/external/iceberg_delete_apply.go:516-547- Equality-delete decoding for
timestampwithisAdjustedToUTC=falsesubtracts the offset fromtime.Now(), not the offset for the timestamp being decoded. Around DST / historical offset changes, the decoded key shifts and equality deletes silently stop matching rows. - Suggested fix: decode timestamp-without-timezone values using the values own local-time semantics, not the current session offset, and add a DST regression test.
- Equality-delete decoding for
-
pkg/sql/plan/build_insert.go:199-227+pkg/sql/iceberg/dml_delete_builder.go:559-571INSERT OVERWRITE PARTITIONaccepts static partition tuples without validating the keys against the target Iceberg partition spec. If the user supplies a typo/unknown field, runtime matches zero files and still appends the new files, silently degrading overwrite into append.- Suggested fix: validate overwrite-partition keys against the Iceberg partition spec during planning or coordinator setup and fail if any requested field is unknown/inapplicable.
addressed the blockers in this patch.
On coverage: the existing Iceberg E2E gate does not contribute to Go coverage because it runs SQL/service-level tests, not coverage-instrumented Go tests. I moved the local E2E Go runner from optools/ to test/iceberg/ and updated the CI script path, since it is a test runner and should not drag down production Go modified-line coverage. I also added focused UT coverage for the fixes above. |
What type of PR is this?
Which issue(s) this PR fixes:
issue #23359
What this PR does / why we need it: