Conversation
RelEasy — resolution kept with warningsThe AI resolution was pushed, but a post-resolution check is still failing on it. Nothing was rolled back — please review the point(s) below before merging and either fix them or dismiss them as false positives.
|
CREATE TABLE / DROP TABLE for DataLakeCatalog; optionally prune on DROPCREATE TABLE / DROP TABLE for DataLakeCatalog; optionally prune on DROP
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
`-Wdocumentation-html` treats `<table>` in a `///` comment as an unclosed
HTML start tag (backticks are Markdown and the Doxygen-style parser does
not honor them), which is fatal under `-Weverything -Werror`:
src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.cpp:942:32:
error: HTML tag 'table' requires an end tag [-Werror,-Wdocumentation-html]
Build report:
https://altinity-build-artifacts.s3.amazonaws.com/json.html?PR=2305&sha=7809d87ff19fbda330d3238b289f338c146bb533&name_0=PR&name_1=Build%20%28amd_binary%29
#2305
This comment was marked as outdated.
This comment was marked as outdated.
|
For the initial check, here is what audit review was able to hit. PR #2305 Audit Findings1)
|
PR #2305 fix verificationRun from BUILD="https://altinity-build-artifacts.s3.amazonaws.com/PRs/2305/<sha>/build_amd_release/clickhouse-common-static_26.6.2.20001.altinityantalya_amd64.deb"Backport 114864 (transform names
|
6ec36d2 to
d592818
Compare
d592818 to
49215ff
Compare
arthurpassos
left a comment
There was a problem hiding this comment.
It is a big PR, I'll do it partially. Below are my human generated comments
|
Below you can find AI comments: |
…prune on `DROP` Adds native `CREATE TABLE`, `CREATE TABLE ... AS source` and `DROP TABLE` support for `DataLakeCatalog` databases, so tables can be created and removed in the catalog itself instead of only being read through it. `CREATE TABLE` registers the table in the catalog (REST, Glue, Unity, S3 Tables), honouring `IF NOT EXISTS` and rolling the staged metadata back when the creation fails or loses a race. `iceberg_metadata_compression_method` is respected by `GlueCatalog::createTable`. `DROP TABLE` unregisters the table and, when the new `data_lake_delete_data_on_drop` setting is enabled, asks the catalog to purge the table data as well. `IF EXISTS` and `IF NOT EXISTS` are passed down to the catalog so the backends can honour them before rejecting an unsupported purge. The engine named in `CREATE TABLE` is validated against the catalog backend, including its arguments: `tryGetTableImpl` reopens a table from its catalog location plus the database engine arguments, so any other argument given in `CREATE` - storage credentials above all - would apply only to the creation and then be replaced on the first read. Engine `SETTINGS` are accepted with an explicit engine. Clauses unsupported for a catalog table are rejected when inherited from a `CREATE TABLE ... AS` source, also when the destination gives its own engine; `datalake_create_table_as_ignore_unsupported_source_properties` drops them instead. The accepted engine family is the catalog's own, so `CREATE TABLE ... ENGINE = DeltaLakeLocal(...)` is accepted in a Unity-backed database. Only Iceberg metadata is generated for a `CREATE TABLE` without a table engine, so that path throws `NOT_IMPLEMENTED` for a catalog of another family before resolving the table location. `OneLakeCatalog`, `BigLakeCatalog` and `S3TablesCatalog` report their fixed backend from `getStorageType`, so every backend check reads `ICatalog::getStorageType`. A catalog that assigns table locations itself rejects an explicit `ENGINE` and `storage_endpoint` as the placement for a new table and asks for `default_base_location`; `SHOW CREATE TABLE` omits the `ENGINE` clause for such a catalog. `SHOW CREATE TABLE` now includes the Iceberg `PARTITION BY` / `ORDER BY` keys when they can be represented. A lost `IF NOT EXISTS` race throws `TABLE_ALREADY_EXISTS`, which `InterpreterCreateQuery` matches on to turn into a no-op, and the Glue rollback removes the staged metadata file only after `GetTable` confirms that nothing still points at it. A transactional (REST) catalog writes the initial metadata file itself, so ClickHouse no longer leaves an orphaned one next to it. `ON CLUSTER` DDL, including `DROP DATABASE ... ON CLUSTER`, is rejected for `DataLakeCatalog`. ClickHouse#98670 (cherry picked from the state of ClickHouse#98670 at b3b0b22, squashed) Adaptations for antalya-26.6: * Dropped: `UnityV2Catalog`, `DatabaseRemote` and `DeltaLakeCatalogRegistration.cpp` do not exist here, nor do the Delta Lake `createTable` path of `DeltaLakeMetadataDeltaKernel` and `UnityCatalog`, so their changes are not applied. Upstream-only tests that the PR merely touched (`test_create_gzip_metadata`, `test_cluster_insert`, `test_database_unity_v2`) are not added. * Added: `ICatalog::managesTableLocation` and the `S3TablesCatalog` override, which upstream already had in its base. * Adapted: `ICatalog::getTableEngineName` does not exist here, so the engine family comes from `table_engine_definition`, and `getCreateTableQueryImpl` keeps this branch's engine name for unreadable tables. * Adapted: `toIcebergMetadataCompressionExtension` and `Iceberg::makeIcebergLocationURI` do not exist here, so `GlueCatalog` and `IcebergMetadata::createInitial` keep this branch's file naming and location construction. * Adapted: `parseTransformAndArgument` takes a time zone here; the new `getPartitionAndSortingKeyASTsFromMetadata` passes an empty one. * Adapted: the Iceberg REST namespace-identifier fix (`namespaceToJSONArray`) was applied inside `buildUpdateMetadataRequestBody` and `buildUpdateSchemaRequestBody`, where this branch builds those request bodies. * Adapted: `IcebergMetadata::drop` keeps this branch's reachable-file enumeration and takes the resolved `delete_data` flag. * Adapted: `StorageObjectStorageCluster` delegates to `pure_storage` here, so `prepareForDrop` forwards there instead of capturing the setting and calling `StorageObjectStorage::dropImpl` itself. * Adapted: `writeMetadataFiles` in `Iceberg/Mutations.cpp` gained `previous_metadata_file_path` beside this branch's `content_type` and `write_metadata_json_file` parameters. * Adapted: the namespace filter (`allowed_namespaces` / `isNamespaceAllowed`) of `RestCatalog` and `GlueCatalog`, `GlueCatalog::getOrFetchMetadataObject`, and the `NoSuchBucket` wrapping in `createInitial` are kept. `TableMetadata::hasDataLakeSpecificProperties` does not exist here, so `GlueCatalog` checks `getDataLakeSpecificProperties().has_value()`. * Adapted: the new settings are registered in `SettingsChangesHistory.cpp`, since this branch does not declare the history inline.
49215ff to
9c43382
Compare
|
@zvonand have you had time to look into #2305 (comment)? |
| "(got {})", datalake_unsupported_storage_clause); | ||
| } | ||
|
|
||
| if (!ignore_unsupported_source_properties || columns_user_specified) |
There was a problem hiding this comment.
Nit: the amount of times ignore_unsupported_source_properties is checked surprised me. Asked AI for a simplification - not sure it looks better, I'll leave it up to you to decide:
auto reject_unsupported_columns = [&]
{
for (const auto & column : properties.columns)
{
if (column.default_desc.expression || column.default_desc.kind != ColumnDefaultKind::Default)
throw Exception(ErrorCodes::BAD_ARGUMENTS,
"Column '{}': {} is not yet supported by DataLakeCatalog table creation",
column.name, toString(column.default_desc.kind));
if (!column.comment.empty() || column.codec || column.ttl
|| !column.settings.empty() || column.statistics.hasExplicitStatistics())
throw Exception(ErrorCodes::BAD_ARGUMENTS,
"Column '{}': COMMENT, CODEC, TTL, STATISTICS, SETTINGS, and PRIMARY KEY "
"are not supported by DataLakeCatalog table creation",
column.name);
}
if (!properties.indices.empty() || !properties.constraints.empty() || !properties.projections.empty()
|| (create.columns_list && (create.columns_list->primary_key || create.columns_list->primary_key_from_columns)))
throw Exception(ErrorCodes::BAD_ARGUMENTS,
"DataLakeCatalog CREATE TABLE does not support PRIMARY KEY, indices, constraints, or projections");
};
if (ignore_unsupported_source_properties)
{
if (columns_user_specified)
reject_unsupported_columns();
else
{
ColumnsDescription plain_columns;
for (const auto & column : properties.columns)
plain_columns.add(ColumnDescription(column.name, column.type));
properties.columns = std::move(plain_columns);
properties.indices = {};
properties.constraints = {};
properties.projections = {};
auto columns_list = make_intrusive<ASTColumns>();
columns_list->set(columns_list->columns, formatColumns(properties.columns));
create.set(create.columns_list, columns_list);
}
if (comment_user_specified)
{
if (create.comment)
throw Exception(ErrorCodes::BAD_ARGUMENTS,
"Table COMMENT is not supported by DataLakeCatalog table creation "
"(note: CREATE TABLE ... AS inherits the comment from the source table)");
}
else
create.reset(create.comment);
}
else
{
reject_unsupported_columns();
if (create.comment)
throw Exception(ErrorCodes::BAD_ARGUMENTS,
"Table COMMENT is not supported by DataLakeCatalog table creation "
"(note: CREATE TABLE ... AS inherits the comment from the source table)");
if (!as_table_saved.empty())
{
// existing source-storage lookup and findUnsupportedDatalakeStorageClause call
}
}
only partially. now I'll make AI look into AI findings first :) |
| assert "stores Iceberg-family tables" in err | ||
|
|
||
|
|
||
| def test_create_table_unsupported_clauses(started_cluster): |
There was a problem hiding this comment.
It seems to be a duplicate of test_create_table_as_rejects_source_storage_clauses, except that it is not using as but rather specifying all the fields manually. Ok to keep, just wanted to flag that
There was a problem hiding this comment.
Yes, I'd keep it -- create and create as are different things.
| assert "is not yet supported" in err | ||
|
|
||
|
|
||
| def test_create_table_with_engine_unsupported_clauses(started_cluster): |
There was a problem hiding this comment.
hmm.. maybe this is bloated?
| node.query(f"DROP TABLE {target_table}", settings=settings) | ||
|
|
||
|
|
||
| def test_show_create_table_omits_unrepresentable_partition_and_sort_order(started_cluster): |
There was a problem hiding this comment.
Why do we need a test for that? What is a "unrepresentable partition by" clause?
Even if such thing exists (which probably exists, I just didn't quite understand), it sounds like we are testing a limitation of clickhouse
There was a problem hiding this comment.
btw this comment made me notice wrong behavior with ignoring some of the unsupported stuff :) I will fix the behavior a bit and make the test more useful
arthurpassos
left a comment
There was a problem hiding this comment.
I read the drop code, and parts of the create as. Left a couple of comments. I also went through the tests, left a couple of comments regarding test duplication. Regardless, it seems like it works and that's how far I can go with a human review in a reasonable time.
I'll leave it to the author to fix the AI comments.
(cherry picked from commit 7ed36c5)
Cherry-picked from ClickHouse#98670.
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Supports
CREATE TABLE,CREATE TABLE … AS source, andDROP TABLEforDataLakeCatalog;DROP TABLEcan request catalog-side data purge via the newdata_lake_delete_data_on_dropsettingCI/CD Options
Exclude tests:
Regression jobs to run: