Skip to content

26.1 Antalya port - DataLakeCatalog namespace filter#1455

Open
ianton-ru wants to merge 9 commits intoantalya-26.1from
frontport/antalya-26.1/data_lake_namespace
Open

26.1 Antalya port - DataLakeCatalog namespace filter#1455
ianton-ru wants to merge 9 commits intoantalya-26.1from
frontport/antalya-26.1/data_lake_namespace

Conversation

@ianton-ru
Copy link
Copy Markdown

@ianton-ru ianton-ru commented Feb 26, 2026

Changelog category (leave one):

  • New Feature

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Frontport for Antalya 26.1

Documentation entry for user-facing changes

...

CI/CD Options

Exclude tests:

  • Fast test
  • Integration Tests
  • Stateless tests
  • Stateful tests
  • Performance tests
  • All with ASAN
  • All with TSAN
  • All with MSAN
  • All with UBSAN
  • All with Coverage
  • All with Aarch64
  • All Regression
  • Disable CI Cache

Regression jobs to run:

  • Fast suites (mostly <1h)
  • Aggregate Functions (2h)
  • Alter (1.5h)
  • Benchmark (30m)
  • ClickHouse Keeper (1h)
  • Iceberg (2h)
  • LDAP (1h)
  • Parquet (1.5h)
  • RBAC (1.5h)
  • SSL Server (1h)
  • S3 (2h)
  • Tiered Storage (2h)

@ianton-ru ianton-ru added antalya port-antalya PRs to be ported to all new Antalya releases antalya-26.1 labels Feb 26, 2026
@github-actions
Copy link
Copy Markdown

github-actions bot commented Feb 26, 2026

Workflow [PR], commit [6d41411]

@mkmkme
Copy link
Copy Markdown
Collaborator

mkmkme commented Mar 25, 2026

For the record

AI audit note: This review comment was generated by AI (gpt-5.3-codex).

Audit update for PR #1455 (DataLakeCatalog namespaces filter — rest / glue / unity):

Confirmed defects

High: OneLake RestCatalog path leaves namespace filter empty so every namespace is denied

  • Impact: catalog_type Iceberg OneLake (ICEBERG_ONELAKE) uses the second RestCatalog constructor. It does not take namespaces and does not initialize allowed_namespaces. That member default-constructs to an empty AllowedNamespaces, which matches the gtest TestAllBlocked behavior (no *, no entries → all checks fail). Listing tables, reading metadata, and related flows will treat all namespaces as filtered (CATALOG_NAMESPACE_DISABLED / empty catalog), even when namespaces defaults to * in database settings.
  • Anchor: RestCatalog::RestCatalog(...) (OneLake overload) and member AllowedNamespaces allowed_namespaces in src/Databases/DataLake/RestCatalog.cpp / RestCatalog.h; DatabaseDataLake::getCatalog() case DB::DatabaseDataLakeCatalogType::ICEBERG_ONELAKE in src/Databases/DataLake/DatabaseDataLake.cpp (no settings[DatabaseDataLakeSetting::namespaces].value passed).
  • Trigger: Create a DataLake database with OneLake catalog (default namespaces='*') and use any non-empty namespace.
  • Why defect: Intended default is “allow all” (*); this path never applies that (or any) filter string, so behavior is not equivalent to REST/Glue/Unity and breaks the catalog.
  • Fix direction (short): Extend the OneLake constructor (and DatabaseDataLake call) to pass settings[DatabaseDataLakeSetting::namespaces].value and initialize allowed_namespaces the same way as the first constructor, or default allowed_namespaces to AllowedNamespaces("*") when no string is provided.
  • Regression test direction (short): Integration or functional test: OneLake DataLake DB with default namespaces can list/query at least one schema/table that works on the base branch.

Low: Public API comment typo in AllowedNamespaces

  • Impact: Misleading doc comment only (nesetd vs nested).
  • Anchor: src/Databases/DataLake/RestCatalog.hisNamespaceAllowed comment.
  • Trigger: N/A (static).
  • Why defect: Wrong parameter name in user-facing nested class comment.
  • Fix direction (short): Replace nesetd with nested.
  • Regression test direction (short): None.

Coverage summary

  • Scope reviewed: PR diff via gh pr diff 1455namespaces setting, CatalogSettings, DatabaseDataLake::getCatalog / getConfiguration, GlueCatalog / UnityCatalog / RestCatalog (including AllowedNamespaces parsing and checks), DataLakeConfiguration + Iceberg/Delta storage wiring, new gtests, integration tests (test_database_iceberg, test_database_glue, test_database_delta).
  • Categories failed: Constructor / initialization parity across RestCatalog overloads (OneLake path).
  • Categories passed: REST + Glue + Unity wiring; nested-namespace semantics for REST; tryGetTableMetadata rethrow for CATALOG_NAMESPACE_DISABLED; integration coverage for REST/Glue/Unity filters; logical review of trie parser vs gtests.
  • Assumptions/limits: No full compile/run in this environment; OneLake defect is proven from PR branch file contents (gh api on refs/pull/1455/head) and the empty-filter semantics encoded in AllowedNamespaces + unit tests.

@ianton-ru
Copy link
Copy Markdown
Author

PR to upstream - ClickHouse#95388

@svb-alt svb-alt requested a review from mkmkme March 26, 2026 12:24
Copy link
Copy Markdown
Collaborator

@mkmkme mkmkme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

bool GlueCatalog::existsTable(const std::string & database_name, const std::string & table_name) const
{
if (!isNamespaceAllowed(database_name))
throw DB::Exception(DB::ErrorCodes::CATALOG_NAMESPACE_DISABLED, "Namespace {} is filtered by `namespaces` database parameter", database_name);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: I'd rather use a forward declaration of this error code instead of using the fully qualified name, but this is cosmetics.

(applies to all added throw statements below as well)

@mkmkme
Copy link
Copy Markdown
Collaborator

mkmkme commented Mar 27, 2026

And for the record, the last audit report came clean

AI audit note: This review comment was generated by AI (claude-4.6-opus).

Audit update for PR #1455 (DataLakeCatalog namespace filter):

Confirmed defects

No confirmed defects in reviewed scope.

Coverage summary

Scope reviewed: All 16 changed files (461 additions, 33 deletions). Full call-graph analysis of namespace filtering across RestCatalog, GlueCatalog, and UnityCatalog, including the new AllowedNamespaces tree matcher, all catalog entrypoints (getTables, existsTable, tryGetTableMetadata, getTableMetadata, createTable, dropTable, empty), the DataLakeConfiguration propagation path, and all integration/unit tests.

Categories passed:

  • Correctness of AllowedNamespaces tree construction and lookup (all 6 gtest patterns verified by manual trace)
  • Namespace filter applied consistently on all catalog entrypoints: read paths (existsTabletryGetTableMetadatagetTableMetadataImpl for Rest; direct check for Glue/Unity), write paths (createTable, dropTable), listing paths (getTables, empty, getNamespacesRecursive)
  • CATALOG_NAMESPACE_DISABLED correctly re-thrown from tryGetTableMetadata catch blocks (not swallowed)
  • Thread safety: AllowedNamespaces / unordered_set immutable after construction; concurrent reads from thread pool in RestCatalog::getTables() are safe
  • Backward compatibility: default namespaces='*' allows everything; existing non-filter tests unaffected
  • C++ lifetime/ownership: no use-after-free/move/dangling risks; all filter data owned by catalog object lifetime
  • Iterator/reference invalidation: no mutation of filter structures after construction
  • Exception safety: boost::split allocations in constructor; safe for RAII cleanup
  • Integer overflow/signedness: loop indices are size_t, no risk
  • No data races, lock-order issues, or deadlock potential in new code
  • Error-contract consistency: all three catalogs throw the same CATALOG_NAMESPACE_DISABLED error code with consistent message format
  • DatabaseDataLake::getConfiguration() correctly propagates namespaces to DataLakeConfiguration for all storage/format combinations (S3/Azure/HDFS/Local Iceberg, S3/Local DeltaLake, S3/Local Hive)
  • Integration tests cover listing, read, create, drop across Rest/Glue/Unity with both allowed and disallowed namespaces; nested namespace patterns exercised for Rest

Categories failed: None.

Assumptions/limits: Static analysis only; no runtime execution; getNamespacesRecursive result parameter collects all namespaces (including filtered) but is unused by all callers; namespaces='' blocks all access (undocumented but consistent behavior); documentation says "comma-separated" but code also accepts space-separated input via boost::is_any_of(", ").

@Selfeer
Copy link
Copy Markdown
Collaborator

Selfeer commented Mar 30, 2026

Audit Report: PR #1455 - DataLakeCatalog namespace filter

AI audit note: This review comment was generated by AI (gpt-5.3-codex).

Audit update for PR #1455 (DataLakeCatalog namespace filter):

Confirmed defects

No confirmed defects in reviewed scope.

Coverage summary

  • Scope reviewed: final PR head diff (16 files, 461 additions/33 deletions): namespace setting propagation (DatabaseDataLakeSettings, CatalogSettings, DatabaseDataLake::getCatalog, DataLakeConfiguration), REST/Glue/Unity filter enforcement on list/read/create/drop paths, CATALOG_NAMESPACE_DISABLED error-contract wiring, REST AllowedNamespaces parser/matcher, and new gtest/integration tests.
  • Categories failed: none.
  • Categories passed: constructor parity across REST overloads (including OneLake), fail-closed behavior for disallowed namespaces across REST/Glue/Unity entrypoints, nested namespace semantics for REST (ns, ns.child, ns.*), error-contract consistency (CATALOG_NAMESPACE_DISABLED propagation including REST tryGetTableMetadata rethrow), immutable filter-state concurrency safety in threaded REST listing, C++ bug-type checks (lifetime/iterator/race/exception-safety/overflow/RAII/UB) for changed logic, and mutation-path rollback consistency (no partial local state mutation before remote calls on blocked namespaces).
  • Assumptions/limits: static analysis only (no local runtime execution); external backend/auth timeout behavior and OneLake end-to-end runtime coverage remain unexecuted in this environment.

@Selfeer
Copy link
Copy Markdown
Collaborator

Selfeer commented Mar 30, 2026

PR #1455 CI Triage — 26.1 Antalya port - DataLakeCatalog namespace filter

Field Value
PR #1455frontport/antalya-26.1/data_lake_namespaceantalya-26.1
Author @ianton-ru
Commit 6d414117ebc349d655be46121721a716ddd83270
Workflow Run 23646052335
CI Report ci_run_report.html
Date 2026-03-27
Merge Status CONFLICTING

PR Description

Frontport of #1337 to antalya-26.1. Adds namespace filter support to DataLakeCatalog (REST, Glue, Unity catalogs). Changed files are exclusively in src/Databases/DataLake/, integration tests for database_iceberg/delta/glue, and docs.

Summary

Category Count Details
PR-caused regression 0
Pre-existing flaky / snapshot gap 2 rankCorrState (aarch64), iceberg_metadata_staleness_ms (settings)
Pre-existing known fails (XFail) 4 max_alter_threads, max_final_threads, max_parsing_threads, max_threads
Infrastructure / transient 2 BuzzHouse server crash, Stateless 01079_parallel_alter_modify_zookeeper_long
Docker CVE scan 1 CVE-2026-2673 (High) in alpine image

Verdict: No failures are caused by this PR. All CI failures are pre-existing or transient.


CI Jobs Status

Job Name Status Failure Type
BuzzHouse (amd_debug) ❌ failure Infrastructure — server crash (TCPHandler core dump)
Grype Scan (alpine image) ❌ failure Docker CVE — CVE-2026-2673 (High)
Regression aarch64 aggregate_functions_3 ❌ failure Pre-existing — missing aarch64 snapshot
Regression aarch64 settings ❌ failure Pre-existing — missing snapshot for new setting
Regression release settings ❌ failure Pre-existing — missing snapshot for new setting
Stateless tests (amd_debug, AsyncInsert, s3 storage, sequential) ❌ failure Infrastructure — flaky test, passed on retry
All other jobs (80+) ✅ success

Detailed Failure Analysis

1. BuzzHouse (amd_debug) — Infrastructure / Transient

  • Failure: Server lost connection during fuzzer run. run-fuzzer.sh: line 285: wait: pid 612 is not a child of this shell. Server exit code 127.
  • Core dump: core.TCPHandler.612-4846 produced and uploaded.
  • Classification: Infrastructure issue. BuzzHouse fuzzer caused a server crash unrelated to this PR's DataLake changes. The fuzzer generates random SQL; crashes here are not attributable to specific PRs unless the crash is in changed code paths.
  • Relation to PR: None. PR only changes DataLake catalog namespace logic; the fuzzer crash occurred in TCPHandler.

2. Stateless tests — 01079_parallel_alter_modify_zookeeper_long — Infrastructure / Flaky

  • Failure: Code: 53. DB::Exception: Type mismatch for column value1. Column has type Float64, got type UInt32: While executing WaitForAsyncInsert. (TYPE_MISMATCH)
  • Result: 595 passed, 70 skipped, 1 broken. Passed on automatic retry.
  • Classification: Known flaky test. This is a long-running concurrent ALTER test that is sensitive to timing. It passed on retry, confirming transient nature.
  • Relation to PR: None. This test exercises ALTER MODIFY operations, not DataLake functionality.

3. Regression aggregate_functions_3 (aarch64) — Pre-existing Snapshot Gap

  • Failing test: /aggregate functions/part 3/state/rankCorrState
  • Error: SnapshotNotFoundError for steps.py.rankcorrstate>=26.1.aarch64.snapshot, key _aggregate_functions_state_rankCorrState_with_group_by_binary
  • Root cause: The with_group_by_binary snapshot key exists for x86_64 but is missing from the aarch64 snapshot file. This is a regression test repo coverage gap.
  • Confirmed pre-existing: PR #1527 (completely unrelated "Cluster Joins part 2" PR) also fails on the same aggregate_functions_3 aarch64 job.
  • Relation to PR: None. Missing snapshot predates this PR.

4. Regression settings (aarch64 + x86_64) — Pre-existing Snapshot Gap

  • Failing test: /settings/default values/iceberg_metadata_staleness_ms
  • Error: SnapshotNotFoundError for default_values.py.default values>=26.1_antalya.snapshot, key iceberg_metadata_staleness_ms
  • Root cause: A new setting iceberg_metadata_staleness_ms was introduced in the antalya-26.1 base branch and arrived via merge commits. The regression test snapshot file has not been updated to include this setting's expected default value.
  • Confirmed NOT from PR diff: gh pr diff 1455 contains no lines matching iceberg_metadata_staleness. The setting came from base branch merges.
  • Known fails in same job (XFail): max_alter_threads, max_final_threads, max_parsing_threads, max_threads — all show auto(16) vs auto(8) mismatch, already marked XFail (CI runner CPU count difference).
  • Relation to PR: None. The setting is from the base branch, not the PR's namespace filter changes.

5. Grype Scan — Docker CVE

  • Failing scan: altinityinfra/clickhouse-server:1455-26.1.4.20001.altinityantalya-alpine
  • CVE: CVE-2026-2673 (High severity, NVD CPE match)
  • Classification: Base image vulnerability, not related to PR changes.
  • Other CVEs found (passed threshold): CVE-2025-60876 (Medium), CVE-2026-27171 (Medium)

New Fails in PR (from CI Report)

The CI report's "New Fails in PR" section (compared with base sha a76c804b0e9734060962ad7cd97a2caaf7c44292) reports:

Nothing to report

This confirms no new test failures were introduced by the PR's changes compared to the base branch.


Checks New Fails (from CI Report)

Check Test Classification
Regression aarch64 settings iceberg_metadata_staleness_ms Pre-existing (base branch new setting)
Regression release settings iceberg_metadata_staleness_ms Pre-existing (base branch new setting)

Regression New Fails (from CI Report)

Suite Test Classification
Regression aarch64 aggregate_functions_3 rankCorrState Pre-existing (missing aarch64 snapshot)
Regression aarch64 settings iceberg_metadata_staleness_ms Pre-existing (base branch new setting)
Regression release settings iceberg_metadata_staleness_ms Pre-existing (base branch new setting)

Recommendations

  1. Merge-ready from CI perspective — No PR-caused regressions detected.
  2. Resolve merge conflicts — PR currently shows CONFLICTING merge status with antalya-26.1.
  3. Snapshot updates needed (separate from this PR):
    • Add rankCorrState with_group_by_binary key to aggregate_functions/tests/snapshots/steps.py.rankcorrstate>=26.1.aarch64.snapshot
    • Add iceberg_metadata_staleness_ms entry to settings/tests/snapshots/default_values.py.default values>=26.1_antalya.snapshot
  4. BuzzHouse core dump — May be worth reviewing (core.TCPHandler.612-4846 in S3 artifacts) but is unrelated to this PR.

@Selfeer
Copy link
Copy Markdown
Collaborator

Selfeer commented Mar 30, 2026

Also the test that was created for this change in 25.8 is green locally: https://github.com/Altinity/clickhouse-regression/blob/main/iceberg/tests/iceberg_engine/namespace_filtering.py

It's not yet part of the CI/CD tho as a result of the overall instability of the suite as of now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

antalya antalya-26.1 antalya-26.1.6.20001 port-antalya PRs to be ported to all new Antalya releases verified Verified by QA

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants