From e9cd4fda04caf594f14dbe53327a1be224ddb676 Mon Sep 17 00:00:00 2001 From: Sreesh Maheshwar Date: Wed, 17 Dec 2025 02:03:54 +0000 Subject: [PATCH 1/2] Fix empty table insert requirement --- .../objects/assert_ref_snapshot_id.hpp | 1 + .../objects/assert_ref_snapshot_id.cpp | 6 +- src/storage/irc_transaction.cpp | 22 +++++- .../test_concurrent_insert_new_table.test | 73 +++++++++++++++++++ 4 files changed, 100 insertions(+), 2 deletions(-) create mode 100644 test/sql/local/irc/insert/test_concurrent_insert_new_table.test diff --git a/src/include/rest_catalog/objects/assert_ref_snapshot_id.hpp b/src/include/rest_catalog/objects/assert_ref_snapshot_id.hpp index e2258bb98..418c6fd02 100644 --- a/src/include/rest_catalog/objects/assert_ref_snapshot_id.hpp +++ b/src/include/rest_catalog/objects/assert_ref_snapshot_id.hpp @@ -30,6 +30,7 @@ class AssertRefSnapshotId { TableRequirementType type; string ref; int64_t snapshot_id; + bool has_snapshot_id = false; }; } // namespace rest_api_objects diff --git a/src/rest_catalog/objects/assert_ref_snapshot_id.cpp b/src/rest_catalog/objects/assert_ref_snapshot_id.cpp index 9a44c60d9..e5ab372cf 100644 --- a/src/rest_catalog/objects/assert_ref_snapshot_id.cpp +++ b/src/rest_catalog/objects/assert_ref_snapshot_id.cpp @@ -50,10 +50,14 @@ string AssertRefSnapshotId::TryFromJSON(yyjson_val *obj) { if (!snapshot_id_val) { return "AssertRefSnapshotId required property 'snapshot-id' is missing"; } else { - if (yyjson_is_sint(snapshot_id_val)) { + if (yyjson_is_null(snapshot_id_val)) { + has_snapshot_id = false; + } else if (yyjson_is_sint(snapshot_id_val)) { snapshot_id = yyjson_get_sint(snapshot_id_val); + has_snapshot_id = true; } else if (yyjson_is_uint(snapshot_id_val)) { snapshot_id = yyjson_get_uint(snapshot_id_val); + has_snapshot_id = true; } else { return StringUtil::Format( "AssertRefSnapshotId property 'snapshot_id' is not of type 'integer', found '%s' instead", diff --git a/src/storage/irc_transaction.cpp b/src/storage/irc_transaction.cpp index 288539048..7f753253c 100644 --- a/src/storage/irc_transaction.cpp +++ b/src/storage/irc_transaction.cpp @@ -47,7 +47,11 @@ void CommitTableToJSON(yyjson_mut_doc *doc, yyjson_mut_val *root_object, auto requirement_json = yyjson_mut_arr_add_obj(doc, requirements_array); yyjson_mut_obj_add_strcpy(doc, requirement_json, "type", assert_ref_snapshot_id.type.value.c_str()); yyjson_mut_obj_add_strcpy(doc, requirement_json, "ref", assert_ref_snapshot_id.ref.c_str()); - yyjson_mut_obj_add_uint(doc, requirement_json, "snapshot-id", assert_ref_snapshot_id.snapshot_id); + if (assert_ref_snapshot_id.has_snapshot_id) { + yyjson_mut_obj_add_uint(doc, requirement_json, "snapshot-id", assert_ref_snapshot_id.snapshot_id); + } else { + yyjson_mut_obj_add_null(doc, requirement_json, "snapshot-id"); + } } else if (requirement.has_assert_create) { auto &assert_create = requirement.assert_create; auto requirement_json = yyjson_mut_arr_add_obj(doc, requirements_array); @@ -230,6 +234,18 @@ static rest_api_objects::TableRequirement CreateAssertRefSnapshotIdRequirement(I auto &res = req.assert_ref_snapshot_id; res.ref = "main"; res.snapshot_id = old_snapshot.snapshot_id; + res.has_snapshot_id = true; + res.type.value = "assert-ref-snapshot-id"; + return req; +} + +static rest_api_objects::TableRequirement CreateAssertNoSnapshotRequirement() { + rest_api_objects::TableRequirement req; + req.has_assert_ref_snapshot_id = true; + + auto &res = req.assert_ref_snapshot_id; + res.ref = "main"; + res.has_snapshot_id = false; res.type.value = "assert-ref-snapshot-id"; return req; } @@ -307,6 +323,10 @@ TableTransactionInfo IRCTransaction::GetTransactionRequest(ClientContext &contex //! If any changes were made to the state of the table, we should assert that our parent snapshot has //! not changed. We don't want to change the table location if someone has added a snapshot commit_state.table_change.requirements.push_back(CreateAssertRefSnapshotIdRequirement(*current_snapshot)); + } else if (!info.has_assert_create) { + //! If the table had no snapshots and isn't created by this transaction, we should assert that no snapshot + //! has been added in the meantime + commit_state.table_change.requirements.push_back(CreateAssertNoSnapshotRequirement()); } transaction.table_changes.push_back(std::move(table_change)); diff --git a/test/sql/local/irc/insert/test_concurrent_insert_new_table.test b/test/sql/local/irc/insert/test_concurrent_insert_new_table.test new file mode 100644 index 000000000..9076cf1aa --- /dev/null +++ b/test/sql/local/irc/insert/test_concurrent_insert_new_table.test @@ -0,0 +1,73 @@ +# name: test/sql/local/irc/insert/test_concurrent_insert_new_table.test +# description: Test concurrent inserts to a newly created table with no snapshots +# group: [insert] + +require-env ICEBERG_SERVER_AVAILABLE + +require avro + +require parquet + +require iceberg + +require httpfs + +# Do not ignore 'HTTP' error messages! +set ignore_error_messages + +statement ok +CREATE SECRET ( + TYPE S3, + KEY_ID 'admin', + SECRET 'password', + ENDPOINT '127.0.0.1:9000', + URL_STYLE 'path', + USE_SSL 0 +); + +statement ok +ATTACH '' AS my_datalake ( + TYPE ICEBERG, + CLIENT_ID 'admin', + CLIENT_SECRET 'password', + ENDPOINT 'http://127.0.0.1:8181' +); + +statement ok +DROP TABLE IF EXISTS my_datalake.default.concurrent_new_table_test; + +statement ok +CREATE TABLE my_datalake.default.concurrent_new_table_test (id INTEGER, name VARCHAR); + +# con1 starts a transaction and inserts +statement ok con1 +begin + +statement ok con1 +INSERT INTO my_datalake.default.concurrent_new_table_test VALUES (1, 'con1'); + +# con2 starts a transaction and inserts +statement ok con2 +begin + +statement ok con2 +INSERT INTO my_datalake.default.concurrent_new_table_test VALUES (2, 'con2'); + +# con2 commits first +statement ok con2 +commit + +# con1 should fail because con2 already added a snapshot +statement error con1 +commit +---- +:.*Requirement failed: branch main was created concurrently.* + +# only con2's data is in the table +query II +SELECT * FROM my_datalake.default.concurrent_new_table_test ORDER BY id; +---- +2 con2 + +statement ok +DROP TABLE my_datalake.default.concurrent_new_table_test; From f6792a7c097bcc9ae78edcb099be6649a6e5e6a8 Mon Sep 17 00:00:00 2001 From: Tmonster Date: Wed, 14 Jan 2026 15:28:53 +0100 Subject: [PATCH 2/2] fix tests for table insert requirement --- test/sql/local/irc/reads/test_read_nulls.test | 2 +- .../irc/test_polaris_concurrent_insert.test | 126 ++++++++++++++++++ 2 files changed, 127 insertions(+), 1 deletion(-) create mode 100644 test/sql/local/irc/test_polaris_concurrent_insert.test diff --git a/test/sql/local/irc/reads/test_read_nulls.test b/test/sql/local/irc/reads/test_read_nulls.test index 1078ec81f..7ab572103 100644 --- a/test/sql/local/irc/reads/test_read_nulls.test +++ b/test/sql/local/irc/reads/test_read_nulls.test @@ -74,7 +74,7 @@ select * from my_datalake.default.test_null_filtering where a is distinct from n 10 no nulls query II -select * from my_datalake.default.test_null_filtering where a is not distinct from null order by a NULLS FIRST; +select * from my_datalake.default.test_null_filtering where a is not distinct from null order by all NULLS FIRST; ---- NULL NULL NULL duck diff --git a/test/sql/local/irc/test_polaris_concurrent_insert.test b/test/sql/local/irc/test_polaris_concurrent_insert.test new file mode 100644 index 000000000..9d2a963c4 --- /dev/null +++ b/test/sql/local/irc/test_polaris_concurrent_insert.test @@ -0,0 +1,126 @@ +# name: test/sql/local/irc/test_polaris_concurrent_insert.test +# description: Test concurrent inserts to a newly created table with no snapshots +# group: [irc] + +require-env POLARIS_CLIENT_ID + +require-env POLARIS_CLIENT_SECRET + +require-env POLARIS_SERVER_AVAILABLE + +require avro + +require parquet + +require httpfs + +require iceberg + +require aws + +# Do not ignore 'HTTP' error messages! +set ignore_error_messages + +statement ok +create secret polaris_secret ( + TYPE ICEBERG, + CLIENT_ID '${POLARIS_CLIENT_ID}', + CLIENT_SECRET '${POLARIS_CLIENT_SECRET}', + ENDPOINT 'http://0.0.0.0:8181/api/catalog' +); + +statement ok +attach 'quickstart_catalog' as my_datalake ( + type ICEBERG, + ENDPOINT 'http://0.0.0.0:8181/api/catalog' +); + +statement ok +DROP TABLE IF EXISTS my_datalake.default.concurrent_new_table_test; + +statement ok +CREATE TABLE my_datalake.default.concurrent_new_table_test (id INTEGER, name VARCHAR); + +# con1 starts a transaction and inserts +statement ok con1 +begin + +statement ok con1 +INSERT INTO my_datalake.default.concurrent_new_table_test VALUES (1, 'con1'); + +# con2 starts a transaction and inserts +statement ok con2 +begin + +statement ok con2 +INSERT INTO my_datalake.default.concurrent_new_table_test VALUES (2, 'con2'); + +# con2 commits first +statement ok con2 +commit + +# con1 should fail because con2 already added a snapshot +statement error con1 +commit +---- +:.*Requirement failed: branch main was created concurrently.* + +# only con2's data is in the table +query II +SELECT * FROM my_datalake.default.concurrent_new_table_test ORDER BY id; +---- +2 con2 + +statement ok +DROP TABLE my_datalake.default.concurrent_new_table_test; + + +# attach with support stage create as false +# polaris is currently file backed in tests, so folders are not automatically +# created when creating a table with stage_create=true +statement ok +attach or replace 'quickstart_catalog' as my_datalake ( + type ICEBERG, + ENDPOINT 'http://0.0.0.0:8181/api/catalog', + SUPPORT_NESTED_NAMESPACES true, + support_stage_create false +); + +statement ok +DROP TABLE IF EXISTS my_datalake.default.concurrent_new_table_test; + +statement ok +CREATE TABLE my_datalake.default.concurrent_new_table_test (id INTEGER, name VARCHAR); + +# con1 starts a transaction and inserts +statement ok con1 +begin + +statement ok con1 +INSERT INTO my_datalake.default.concurrent_new_table_test VALUES (1, 'con1'); + +# con2 starts a transaction and inserts +statement ok con2 +begin + +statement ok con2 +INSERT INTO my_datalake.default.concurrent_new_table_test VALUES (2, 'con2'); + +# con2 commits first +statement ok con2 +commit + +# con1 should fail because con2 already added a snapshot +statement error con1 +commit +---- +:.*Requirement failed: branch main was created concurrently.* + +# only con2's data is in the table +query II +SELECT * FROM my_datalake.default.concurrent_new_table_test ORDER BY id; +---- +2 con2 + +statement ok +DROP TABLE my_datalake.default.concurrent_new_table_test;