25.3 Antalya - Iceberg as alias for DataLakeCatalog with catalog_type='rest'

[ianton-ru]

Solve issue #821.
Database Iceberg as alias to DataLakeCatalog

[arthurpassos]

Just FYI, functions have a better mechanism: https://github.com/ClickHouse/ClickHouse/blob/b2c81981fbc5746aeccece8cf288167b5ad18df4/src/Common/IFactoryWithAliases.h#L23

Could you add a simple test? It can be a duplicate of an existing test using the alias

[ianton-ru]

Could you add a simple test? It can be a duplicate of an existing test using the alias

Added, and fixed masking sensitive info (thanks to tests).

Just FYI, functions have a better mechanism: https://github.com/ClickHouse/ClickHouse/blob/b2c81981fbc5746aeccece8cf288167b5ad18df4/src/Common/IFactoryWithAliases.h#L23

IFactoryWithAliases looks good, but too overkilled for single use. Variant with two functions with the same name also implemented - https://github.com/ClickHouse/ClickHouse/blob/master/src/TableFunctions/TableFunctionObjectStorage.cpp#L331, so for now I left simple code.

[arthurpassos]

test_database_iceberg/test.py::test_hide_sensitive_info is failing, could it be related?

[ianton-ru]

test_database_iceberg/test.py::test_hide_sensitive_info is failing, could it be related?

No, it's failed with NoSuchBucketException, it looks like a bug in new image minio/mc:RELEASE.2025-05-21T01-59-54Z. It reproduced locally with this version, with previous RELEASE.2025-04-16T18-13-26Z works fine.

[arthurpassos]

If tests are ok, I think this looks good.

It appears that the engine name is not widely used, and no AST related code aside from ASTSetQuery.cpp uses it:

laptop@arthur:~/work/s3_hive_style_partitioned_writes$ grep -rn "\"DataLakeCatalog" src
src/Databases/DataLake/DataLakeConstants.h:10:static constexpr auto DATABASE_ENGINE_NAME = "DataLakeCatalog";
src/Databases/DataLake/DatabaseDataLake.cpp:672:    factory.registerDatabase("DataLakeCatalog", create_fn, { .supports_arguments = true, .supports_settings = true });
src/Parsers/FunctionSecretArgumentsFinder.h:631:        else if (engine_name == "DataLakeCatalog")

laptop@arthur:~/work/s3_hive_style_partitioned_writes$ grep -rn "DataLake::DATABASE_ENGINE_NAME" src
src/Databases/DataLake/DatabaseDataLake.h:26:    String getEngineName() const override { return DataLake::DATABASE_ENGINE_NAME; }
src/Parsers/ASTSetQuery.cpp:98:            if (DataLake::DATABASE_ENGINE_NAME == state.create_engine_name)

[arthurpassos]

Adding slack comment just so we can keep track of it:

I approved the PR, but there is a small chance upstream will silently break this feature in the future and chances are we won't catch it.
For instance, we caught an issue in the PR where the upstream code that handles hiding secrets from print messages depends on the engine name used in the AST/ query (which has not been translated to DatalakeCatalog yet). And it did not cover this "alias". See https://github.com/Altinity/ClickHouse/pull/822/files#diff-4ec4a72ae2ddac0e80a10345e13a52baaadda419f596979e70c567a5eaa7beedR98 for more info.
It is not a biggie, but we should keep that in mind

[ianton-ru]

In stateless tests failed 01271_show_privileges and 02995_new_settings_history, both about parquet metadata cache.

[arthurpassos]

In stateless tests failed 01271_show_privileges and 02995_new_settings_history, both about parquet metadata cache.

Isn't it a matter of rebasing? Or you want to avoid rebasing right now?