You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "lishuxu (via GitHub)" <gi...@apache.org> on 2025/07/22 09:22:26 UTC
[PR] feat: Refresh method support for table [iceberg-cpp]
lishuxu opened a new pull request, #152:
URL: https://github.com/apache/iceberg-cpp/pull/152
(no comment)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Refresh method support for table [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #152:
URL: https://github.com/apache/iceberg-cpp/pull/152#discussion_r2253066793
##########
test/in_memory_catalog_test.cc:
##########
@@ -115,6 +114,50 @@ TEST_F(InMemoryCatalogTest, RegisterTable) {
ASSERT_EQ(table.value()->location(), "s3://bucket/test/location");
}
+TEST_F(InMemoryCatalogTest, RefreshTable) {
+ TableIdentifier tableIdent{.ns = {}, .name = "t1"};
+ std::shared_ptr<MockInMemoryCatalog> mock_catalog =
+ std::make_shared<MockInMemoryCatalog>(
+ "mock_catalog", file_io_, "/tmp/warehouse/",
+ std::unordered_map<std::string, std::string>());
+ auto table_location = GenerateTestTableLocation(tableIdent.name);
+ auto buildTable = [this, &tableIdent, &mock_catalog, &table_location](
+ int64_t snapshot_id, int64_t version) -> std::unique_ptr<Table> {
+ std::unique_ptr<TableMetadata> metadata;
+
+ ReadTableMetadata("TableMetadataV2Valid.json", &metadata);
Review Comment:
I would recommend to do the following:
```cpp
class MockCatalog : public Catalog {
public:
MockCatalog() = default;
~MockCatalog() override = default;
MOCK_METHOD(std::string_view, name, (), (const, override));
MOCK_METHOD(Status, CreateNamespace,
(const Namespace&, (const std::unordered_map<std::string, std::string>&)),
(override));
MOCK_METHOD((Result<std::vector<Namespace>>), ListNamespaces, (const Namespace&),
(const, override));
MOCK_METHOD((Result<std::unordered_map<std::string, std::string>>),
GetNamespaceProperties, (const Namespace&), (const, override));
MOCK_METHOD(Status, UpdateNamespaceProperties,
(const Namespace&, (const std::unordered_map<std::string, std::string>&),
(const std::unordered_set<std::string>&)),
(override));
MOCK_METHOD(Status, DropNamespace, (const Namespace&), (override));
MOCK_METHOD(Result<bool>, NamespaceExists, (const Namespace&), (const, override));
MOCK_METHOD((Result<std::vector<TableIdentifier>>), ListTables, (const Namespace&),
(const, override));
MOCK_METHOD((Result<std::unique_ptr<Table>>), LoadTable, (const TableIdentifier&),
(override));
MOCK_METHOD(Status, DropTable, (const TableIdentifier&, bool), (override));
MOCK_METHOD(Result<bool>, TableExists, (const TableIdentifier&), (const, override));
MOCK_METHOD((Result<std::shared_ptr<Table>>), RegisterTable,
(const TableIdentifier&, const std::string&), (override));
MOCK_METHOD((std::unique_ptr<Catalog::TableBuilder>), BuildTable,
(const TableIdentifier&, const Schema&), (const, override));
MOCK_METHOD((Result<std::unique_ptr<Table>>), CreateTable,
(const TableIdentifier&, const Schema&, const PartitionSpec&,
const std::string&, (const std::unordered_map<std::string, std::string>&)),
(override));
MOCK_METHOD((Result<std::unique_ptr<Table>>), UpdateTable,
(const TableIdentifier&,
(const std::vector<std::unique_ptr<UpdateRequirement>>&),
(const std::vector<std::unique_ptr<MetadataUpdate>>&)),
(override));
MOCK_METHOD((Result<std::shared_ptr<Transaction>>), StageCreateTable,
(const TableIdentifier&, const Schema&, const PartitionSpec&,
const std::string&, (const std::unordered_map<std::string, std::string>&)),
(override));
};
TEST(CatalogTest, MockCatalog) {
TableIdentifier table_ident{.ns = {}, .name = "t1"};
auto schema = std::make_shared<Schema>(
std::vector<SchemaField>{SchemaField::MakeRequired(1, "x", int64())},
/*schema_id=*/1);
std::shared_ptr<FileIO> io;
auto catalog = std::make_shared<MockCatalog>();
EXPECT_CALL(*catalog, name()).WillOnce(::testing::Return("test_catalog"));
EXPECT_EQ(catalog->name(), "test_catalog");
// Mock 1st call to LoadTable
EXPECT_CALL(*catalog, LoadTable(::testing::_))
.WillOnce(::testing::Return(
std::make_unique<Table>(table_ident,
std::make_shared<TableMetadata>(TableMetadata{
.schemas = {schema},
.current_schema_id = 1,
.current_snapshot_id = 1,
.snapshots = {std::make_shared<Snapshot>(Snapshot{
.snapshot_id = 1,
.sequence_number = 1,
})}}),
"s3://location", io, catalog)));
auto load_table_result = catalog->LoadTable(table_ident);
ASSERT_THAT(load_table_result, IsOk());
auto loaded_table = std::move(load_table_result.value());
ASSERT_EQ(loaded_table->current_snapshot().value()->snapshot_id, 1);
// Mock 2nd call to LoadTable
EXPECT_CALL(*catalog, LoadTable(::testing::_))
.WillOnce(::testing::Return(
std::make_unique<Table>(table_ident,
std::make_shared<TableMetadata>(TableMetadata{
.schemas = {schema},
.current_schema_id = 1,
.current_snapshot_id = 2,
.snapshots = {std::make_shared<Snapshot>(Snapshot{
.snapshot_id = 1,
.sequence_number = 1,
}),
std::make_shared<Snapshot>(Snapshot{
.snapshot_id = 2,
.sequence_number = 2,
})}}),
"s3://location", io, catalog)));
auto refreshed_result = loaded_table->Refresh();
ASSERT_THAT(refreshed_result, IsOk());
ASSERT_EQ(refreshed_result.value()->current_snapshot().value()->snapshot_id, 2);
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Refresh method support for table [iceberg-cpp]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko merged PR #152:
URL: https://github.com/apache/iceberg-cpp/pull/152
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Refresh method support for table [iceberg-cpp]
Posted by "lishuxu (via GitHub)" <gi...@apache.org>.
lishuxu commented on code in PR #152:
URL: https://github.com/apache/iceberg-cpp/pull/152#discussion_r2249508749
##########
test/mock_catalog.h:
##########
@@ -0,0 +1,36 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#pragma once
+
+#include "iceberg/catalog/in_memory_catalog.h"
+
+namespace iceberg {
+
+class ICEBERG_EXPORT MockInMemoryCatalog : public InMemoryCatalog {
Review Comment:
Some tests rely on InMemoryCatalog's other methods. Mocking Catalog directly would require mocking all methods the tests touch, which could be more cumbersome.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Refresh method support for table [iceberg-cpp]
Posted by "lishuxu (via GitHub)" <gi...@apache.org>.
lishuxu commented on code in PR #152:
URL: https://github.com/apache/iceberg-cpp/pull/152#discussion_r2249510586
##########
test/in_memory_catalog_test.cc:
##########
@@ -115,6 +114,50 @@ TEST_F(InMemoryCatalogTest, RegisterTable) {
ASSERT_EQ(table.value()->location(), "s3://bucket/test/location");
}
+TEST_F(InMemoryCatalogTest, RefreshTable) {
+ TableIdentifier tableIdent{.ns = {}, .name = "t1"};
+ std::shared_ptr<MockInMemoryCatalog> mock_catalog =
+ std::make_shared<MockInMemoryCatalog>(
+ "mock_catalog", file_io_, "/tmp/warehouse/",
+ std::unordered_map<std::string, std::string>());
+ auto table_location = GenerateTestTableLocation(tableIdent.name);
+ auto buildTable = [this, &tableIdent, &mock_catalog, &table_location](
+ int64_t snapshot_id, int64_t version) -> std::unique_ptr<Table> {
+ std::unique_ptr<TableMetadata> metadata;
+
+ ReadTableMetadata("TableMetadataV2Valid.json", &metadata);
Review Comment:
I simplified the buildTable lambda logic — the steps are simple too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Refresh method support for table [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #152:
URL: https://github.com/apache/iceberg-cpp/pull/152#discussion_r2265577075
##########
test/mock_catalog.h:
##########
@@ -0,0 +1,85 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#pragma once
+
+#include "iceberg/catalog.h"
+
+namespace iceberg {
+
+class MockCatalog : public Catalog {
+ public:
+ MockCatalog() = default;
+ ~MockCatalog() override = default;
+
+ MOCK_METHOD(std::string_view, name, (), (const, override));
Review Comment:
Let's use NOLINT to work around this.
##########
src/iceberg/catalog.h:
##########
@@ -175,7 +175,6 @@ class ICEBERG_EXPORT Catalog {
/// \return a Table instance or ErrorKind::kAlreadyExists if the table already exists
virtual Result<std::shared_ptr<Table>> RegisterTable(
const TableIdentifier& identifier, const std::string& metadata_file_location) = 0;
-
Review Comment:
revert this line?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Refresh method support for table [iceberg-cpp]
Posted by "lishuxu (via GitHub)" <gi...@apache.org>.
lishuxu commented on code in PR #152:
URL: https://github.com/apache/iceberg-cpp/pull/152#discussion_r2249081682
##########
src/iceberg/table.h:
##########
@@ -114,9 +117,11 @@ class ICEBERG_EXPORT Table {
const std::shared_ptr<FileIO>& io() const;
private:
+ friend class Table;
+
Review Comment:
Yes, looks unnecessary.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Refresh method support for table [iceberg-cpp]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on PR #152:
URL: https://github.com/apache/iceberg-cpp/pull/152#issuecomment-3184218935
This looks good, thanks @lishuxu for working on this, and thanks @wgtmac and @zeroshade for the review 🚀
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Refresh method support for table [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #152:
URL: https://github.com/apache/iceberg-cpp/pull/152#discussion_r2227168655
##########
test/in_memory_catalog_test.cc:
##########
@@ -115,6 +115,30 @@ TEST_F(InMemoryCatalogTest, RegisterTable) {
ASSERT_EQ(table.value()->location(), "s3://bucket/test/location");
}
+TEST_F(InMemoryCatalogTest, RefreshTable) {
+ TableIdentifier tableIdent{.ns = {}, .name = "t1"};
+
+ std::unique_ptr<TableMetadata> metadata;
+ ASSERT_NO_FATAL_FAILURE(ReadTableMetadata("TableMetadataV2Valid.json", &metadata));
+
+ auto table_location = GenerateTestTableLocation(tableIdent.name);
+ auto metadata_location = std::format("{}v0.metadata.json", table_location);
+ auto status = TableMetadataUtil::Write(*file_io_, metadata_location, *metadata);
+ EXPECT_THAT(status, IsOk());
+
+ auto table = catalog_->RegisterTable(tableIdent, metadata_location);
+ EXPECT_THAT(table, IsOk());
+ ASSERT_TRUE(table.value()->current_snapshot().has_value());
+ ASSERT_EQ(table.value()->current_snapshot().value()->snapshot_id, 3055729675574597004);
+
+ // Now we don't support commit method in catalog, so here only test refresh with the
Review Comment:
I think we can add a `test/mock_catalog.h` to use gmock to write a `MockCatalog` so that we can inject table metadata at any time by mocking the call of `Catalog::LoadTable`. In the future, we may need to mock the catalog a lot of times, so I think it deserves to be added.
##########
src/iceberg/table.h:
##########
@@ -114,9 +117,11 @@ class ICEBERG_EXPORT Table {
const std::shared_ptr<FileIO>& io() const;
private:
+ friend class Table;
+
Review Comment:
```suggestion
```
##########
src/iceberg/table.cc:
##########
@@ -21,16 +21,36 @@
#include <algorithm>
+#include "iceberg/catalog.h"
#include "iceberg/partition_spec.h"
#include "iceberg/schema.h"
#include "iceberg/sort_order.h"
#include "iceberg/table_metadata.h"
#include "iceberg/table_scan.h"
+#include "iceberg/util/macros.h"
namespace iceberg {
const std::string& Table::uuid() const { return metadata_->table_uuid; }
+Status Table::Refresh() {
+ if (!catalog_) {
+ return InvalidArgument("Cannot refresh table metadata without a catalog");
+ }
+
+ ICEBERG_ASSIGN_OR_RAISE(auto fresh, catalog_->LoadTable(identifier_));
+ if (metadata_location_ != fresh->metadata_location_) {
+ metadata_ = std::move(fresh->metadata_);
+ metadata_location_ = std::move(fresh->metadata_location_);
+ io_ = std::move(fresh->io_);
Review Comment:
```suggestion
ICEBERG_ASSIGN_OR_RAISE(auto refreshed_table, catalog_->LoadTable(identifier_));
if (metadata_location_ != refreshed_table->metadata_location_) {
metadata_ = std::move(refreshed_table->metadata_);
metadata_location_ = std::move(refreshed_table->metadata_location_);
io_ = std::move(refreshed_table->io_);
```
##########
src/iceberg/table.h:
##########
@@ -114,9 +117,11 @@ class ICEBERG_EXPORT Table {
const std::shared_ptr<FileIO>& io() const;
private:
+ friend class Table;
+
Review Comment:
We don't need this, right?
##########
src/iceberg/table.cc:
##########
@@ -21,16 +21,36 @@
#include <algorithm>
+#include "iceberg/catalog.h"
#include "iceberg/partition_spec.h"
#include "iceberg/schema.h"
#include "iceberg/sort_order.h"
#include "iceberg/table_metadata.h"
#include "iceberg/table_scan.h"
+#include "iceberg/util/macros.h"
namespace iceberg {
const std::string& Table::uuid() const { return metadata_->table_uuid; }
+Status Table::Refresh() {
+ if (!catalog_) {
+ return InvalidArgument("Cannot refresh table metadata without a catalog");
Review Comment:
```suggestion
return NotSupported("Cannot refresh table metadata without a catalog");
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Refresh method support for table [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #152:
URL: https://github.com/apache/iceberg-cpp/pull/152#discussion_r2253066793
##########
test/in_memory_catalog_test.cc:
##########
@@ -115,6 +114,50 @@ TEST_F(InMemoryCatalogTest, RegisterTable) {
ASSERT_EQ(table.value()->location(), "s3://bucket/test/location");
}
+TEST_F(InMemoryCatalogTest, RefreshTable) {
+ TableIdentifier tableIdent{.ns = {}, .name = "t1"};
+ std::shared_ptr<MockInMemoryCatalog> mock_catalog =
+ std::make_shared<MockInMemoryCatalog>(
+ "mock_catalog", file_io_, "/tmp/warehouse/",
+ std::unordered_map<std::string, std::string>());
+ auto table_location = GenerateTestTableLocation(tableIdent.name);
+ auto buildTable = [this, &tableIdent, &mock_catalog, &table_location](
+ int64_t snapshot_id, int64_t version) -> std::unique_ptr<Table> {
+ std::unique_ptr<TableMetadata> metadata;
+
+ ReadTableMetadata("TableMetadataV2Valid.json", &metadata);
Review Comment:
I would recommend to do the following:
```
class MockCatalog : public Catalog {
public:
MockCatalog() = default;
~MockCatalog() override = default;
MOCK_METHOD(std::string_view, name, (), (const, override));
MOCK_METHOD(Status, CreateNamespace,
(const Namespace&, (const std::unordered_map<std::string, std::string>&)),
(override));
MOCK_METHOD((Result<std::vector<Namespace>>), ListNamespaces, (const Namespace&),
(const, override));
MOCK_METHOD((Result<std::unordered_map<std::string, std::string>>),
GetNamespaceProperties, (const Namespace&), (const, override));
MOCK_METHOD(Status, UpdateNamespaceProperties,
(const Namespace&, (const std::unordered_map<std::string, std::string>&),
(const std::unordered_set<std::string>&)),
(override));
MOCK_METHOD(Status, DropNamespace, (const Namespace&), (override));
MOCK_METHOD(Result<bool>, NamespaceExists, (const Namespace&), (const, override));
MOCK_METHOD((Result<std::vector<TableIdentifier>>), ListTables, (const Namespace&),
(const, override));
MOCK_METHOD((Result<std::unique_ptr<Table>>), LoadTable, (const TableIdentifier&),
(override));
MOCK_METHOD(Status, DropTable, (const TableIdentifier&, bool), (override));
MOCK_METHOD(Result<bool>, TableExists, (const TableIdentifier&), (const, override));
MOCK_METHOD((Result<std::shared_ptr<Table>>), RegisterTable,
(const TableIdentifier&, const std::string&), (override));
MOCK_METHOD((std::unique_ptr<Catalog::TableBuilder>), BuildTable,
(const TableIdentifier&, const Schema&), (const, override));
MOCK_METHOD((Result<std::unique_ptr<Table>>), CreateTable,
(const TableIdentifier&, const Schema&, const PartitionSpec&,
const std::string&, (const std::unordered_map<std::string, std::string>&)),
(override));
MOCK_METHOD((Result<std::unique_ptr<Table>>), UpdateTable,
(const TableIdentifier&,
(const std::vector<std::unique_ptr<UpdateRequirement>>&),
(const std::vector<std::unique_ptr<MetadataUpdate>>&)),
(override));
MOCK_METHOD((Result<std::shared_ptr<Transaction>>), StageCreateTable,
(const TableIdentifier&, const Schema&, const PartitionSpec&,
const std::string&, (const std::unordered_map<std::string, std::string>&)),
(override));
};
TEST(CatalogTest, MockCatalog) {
TableIdentifier table_ident{.ns = {}, .name = "t1"};
auto schema = std::make_shared<Schema>(
std::vector<SchemaField>{SchemaField::MakeRequired(1, "x", int64())},
/*schema_id=*/1);
std::shared_ptr<FileIO> io;
auto catalog = std::make_shared<MockCatalog>();
EXPECT_CALL(*catalog, name()).WillOnce(::testing::Return("test_catalog"));
EXPECT_EQ(catalog->name(), "test_catalog");
// Mock 1st call to LoadTable
EXPECT_CALL(*catalog, LoadTable(::testing::_))
.WillOnce(::testing::Return(
std::make_unique<Table>(table_ident,
std::make_shared<TableMetadata>(TableMetadata{
.schemas = {schema},
.current_schema_id = 1,
.current_snapshot_id = 1,
.snapshots = {std::make_shared<Snapshot>(Snapshot{
.snapshot_id = 1,
.sequence_number = 0,
})}}),
"s3://location", io, catalog)));
auto load_table_result = catalog->LoadTable(table_ident);
ASSERT_THAT(load_table_result, IsOk());
auto loaded_table = std::move(load_table_result.value());
ASSERT_EQ(loaded_table->current_snapshot().value()->snapshot_id, 1);
// Mock 2nd call to LoadTable
EXPECT_CALL(*catalog, LoadTable(::testing::_))
.WillOnce(::testing::Return(
std::make_unique<Table>(table_ident,
std::make_shared<TableMetadata>(TableMetadata{
.schemas = {schema},
.current_schema_id = 1,
.current_snapshot_id = 2,
.snapshots = {std::make_shared<Snapshot>(Snapshot{
.snapshot_id = 1,
.sequence_number = 1,
}),
std::make_shared<Snapshot>(Snapshot{
.snapshot_id = 2,
.sequence_number = 2,
})}}),
"s3://location", io, catalog)));
auto refreshed_result = loaded_table->Refresh();
ASSERT_THAT(refreshed_result, IsOk());
ASSERT_EQ(refreshed_result.value()->current_snapshot().value()->snapshot_id, 2);
}
```
##########
test/in_memory_catalog_test.cc:
##########
@@ -115,6 +114,50 @@ TEST_F(InMemoryCatalogTest, RegisterTable) {
ASSERT_EQ(table.value()->location(), "s3://bucket/test/location");
}
+TEST_F(InMemoryCatalogTest, RefreshTable) {
+ TableIdentifier tableIdent{.ns = {}, .name = "t1"};
+ std::shared_ptr<MockInMemoryCatalog> mock_catalog =
+ std::make_shared<MockInMemoryCatalog>(
+ "mock_catalog", file_io_, "/tmp/warehouse/",
+ std::unordered_map<std::string, std::string>());
+ auto table_location = GenerateTestTableLocation(tableIdent.name);
+ auto buildTable = [this, &tableIdent, &mock_catalog, &table_location](
+ int64_t snapshot_id, int64_t version) -> std::unique_ptr<Table> {
+ std::unique_ptr<TableMetadata> metadata;
+
+ ReadTableMetadata("TableMetadataV2Valid.json", &metadata);
Review Comment:
I would recommend to do the following:
```cpp
class MockCatalog : public Catalog {
public:
MockCatalog() = default;
~MockCatalog() override = default;
MOCK_METHOD(std::string_view, name, (), (const, override));
MOCK_METHOD(Status, CreateNamespace,
(const Namespace&, (const std::unordered_map<std::string, std::string>&)),
(override));
MOCK_METHOD((Result<std::vector<Namespace>>), ListNamespaces, (const Namespace&),
(const, override));
MOCK_METHOD((Result<std::unordered_map<std::string, std::string>>),
GetNamespaceProperties, (const Namespace&), (const, override));
MOCK_METHOD(Status, UpdateNamespaceProperties,
(const Namespace&, (const std::unordered_map<std::string, std::string>&),
(const std::unordered_set<std::string>&)),
(override));
MOCK_METHOD(Status, DropNamespace, (const Namespace&), (override));
MOCK_METHOD(Result<bool>, NamespaceExists, (const Namespace&), (const, override));
MOCK_METHOD((Result<std::vector<TableIdentifier>>), ListTables, (const Namespace&),
(const, override));
MOCK_METHOD((Result<std::unique_ptr<Table>>), LoadTable, (const TableIdentifier&),
(override));
MOCK_METHOD(Status, DropTable, (const TableIdentifier&, bool), (override));
MOCK_METHOD(Result<bool>, TableExists, (const TableIdentifier&), (const, override));
MOCK_METHOD((Result<std::shared_ptr<Table>>), RegisterTable,
(const TableIdentifier&, const std::string&), (override));
MOCK_METHOD((std::unique_ptr<Catalog::TableBuilder>), BuildTable,
(const TableIdentifier&, const Schema&), (const, override));
MOCK_METHOD((Result<std::unique_ptr<Table>>), CreateTable,
(const TableIdentifier&, const Schema&, const PartitionSpec&,
const std::string&, (const std::unordered_map<std::string, std::string>&)),
(override));
MOCK_METHOD((Result<std::unique_ptr<Table>>), UpdateTable,
(const TableIdentifier&,
(const std::vector<std::unique_ptr<UpdateRequirement>>&),
(const std::vector<std::unique_ptr<MetadataUpdate>>&)),
(override));
MOCK_METHOD((Result<std::shared_ptr<Transaction>>), StageCreateTable,
(const TableIdentifier&, const Schema&, const PartitionSpec&,
const std::string&, (const std::unordered_map<std::string, std::string>&)),
(override));
};
TEST(CatalogTest, MockCatalog) {
TableIdentifier table_ident{.ns = {}, .name = "t1"};
auto schema = std::make_shared<Schema>(
std::vector<SchemaField>{SchemaField::MakeRequired(1, "x", int64())},
/*schema_id=*/1);
std::shared_ptr<FileIO> io;
auto catalog = std::make_shared<MockCatalog>();
EXPECT_CALL(*catalog, name()).WillOnce(::testing::Return("test_catalog"));
EXPECT_EQ(catalog->name(), "test_catalog");
// Mock 1st call to LoadTable
EXPECT_CALL(*catalog, LoadTable(::testing::_))
.WillOnce(::testing::Return(
std::make_unique<Table>(table_ident,
std::make_shared<TableMetadata>(TableMetadata{
.schemas = {schema},
.current_schema_id = 1,
.current_snapshot_id = 1,
.snapshots = {std::make_shared<Snapshot>(Snapshot{
.snapshot_id = 1,
.sequence_number = 0,
})}}),
"s3://location", io, catalog)));
auto load_table_result = catalog->LoadTable(table_ident);
ASSERT_THAT(load_table_result, IsOk());
auto loaded_table = std::move(load_table_result.value());
ASSERT_EQ(loaded_table->current_snapshot().value()->snapshot_id, 1);
// Mock 2nd call to LoadTable
EXPECT_CALL(*catalog, LoadTable(::testing::_))
.WillOnce(::testing::Return(
std::make_unique<Table>(table_ident,
std::make_shared<TableMetadata>(TableMetadata{
.schemas = {schema},
.current_schema_id = 1,
.current_snapshot_id = 2,
.snapshots = {std::make_shared<Snapshot>(Snapshot{
.snapshot_id = 1,
.sequence_number = 1,
}),
std::make_shared<Snapshot>(Snapshot{
.snapshot_id = 2,
.sequence_number = 2,
})}}),
"s3://location", io, catalog)));
auto refreshed_result = loaded_table->Refresh();
ASSERT_THAT(refreshed_result, IsOk());
ASSERT_EQ(refreshed_result.value()->current_snapshot().value()->snapshot_id, 2);
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Refresh method support for table [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #152:
URL: https://github.com/apache/iceberg-cpp/pull/152#issuecomment-3175177146
@Fokko @zeroshade Could you help review this? Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Refresh method support for table [iceberg-cpp]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #152:
URL: https://github.com/apache/iceberg-cpp/pull/152#discussion_r2267822777
##########
src/iceberg/table.cc:
##########
@@ -21,16 +21,36 @@
#include <algorithm>
+#include "iceberg/catalog.h"
#include "iceberg/partition_spec.h"
#include "iceberg/schema.h"
#include "iceberg/sort_order.h"
#include "iceberg/table_metadata.h"
#include "iceberg/table_scan.h"
+#include "iceberg/util/macros.h"
namespace iceberg {
const std::string& Table::uuid() const { return metadata_->table_uuid; }
+Status Table::Refresh() {
+ if (!catalog_) {
+ return NotSupported("Refresh is not supported for table without a catalog");
+ }
+
+ ICEBERG_ASSIGN_OR_RAISE(auto refreshed_table, catalog_->LoadTable(identifier_));
+ if (metadata_location_ != refreshed_table->metadata_location_) {
+ metadata_ = std::move(refreshed_table->metadata_);
+ metadata_location_ = std::move(refreshed_table->metadata_location_);
+ io_ = std::move(refreshed_table->io_);
+
+ schemas_map_.reset();
+ partition_spec_map_.reset();
+ sort_orders_map_.reset();
Review Comment:
shouldn't these get updated from the refreshed table?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Refresh method support for table [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #152:
URL: https://github.com/apache/iceberg-cpp/pull/152#discussion_r2238166054
##########
src/iceberg/catalog/in_memory_catalog.h:
##########
@@ -93,6 +93,10 @@ class ICEBERG_EXPORT InMemoryCatalog
std::unique_ptr<iceberg::TableBuilder> BuildTable(const TableIdentifier& identifier,
const Schema& schema) const override;
+ protected:
+ Status UpdateTableMetaLocationInternal(const TableIdentifier& identifier,
Review Comment:
Please remove this function. We need to use GMock to update the location without hacking any API.
##########
test/mock_catalog.h:
##########
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#pragma once
+
+#include "iceberg/catalog/in_memory_catalog.h"
+
+namespace iceberg {
+
+class ICEBERG_EXPORT MockInMemoryCatalog : public InMemoryCatalog {
Review Comment:
Please take a look at https://google.github.io/googletest/gmock_for_dummies.html#writing-the-mock-class to use `MOCK_METHOD` to override functions of Catalog. Then you can use `EXPECT_CALL` with `WillOnce` of `MockCatalog::LoadTable` to mock different returns for each time.
##########
src/iceberg/table.cc:
##########
@@ -21,16 +21,36 @@
#include <algorithm>
+#include "iceberg/catalog.h"
#include "iceberg/partition_spec.h"
#include "iceberg/schema.h"
#include "iceberg/sort_order.h"
#include "iceberg/table_metadata.h"
#include "iceberg/table_scan.h"
+#include "iceberg/util/macros.h"
namespace iceberg {
const std::string& Table::uuid() const { return metadata_->table_uuid; }
+Status Table::Refresh() {
+ if (!catalog_) {
+ return InvalidArgument("Refresh is not supported for table without a catalog");
Review Comment:
```suggestion
return NotSupported("Refresh is not supported for table without a catalog");
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Refresh method support for table [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #152:
URL: https://github.com/apache/iceberg-cpp/pull/152#discussion_r2249273902
##########
src/iceberg/catalog/in_memory_catalog.cc:
##########
@@ -297,6 +306,17 @@ Status InMemoryNamespace::UnregisterTable(TableIdentifier const& table_ident) {
return {};
}
+Status InMemoryNamespace::UpdateTableMetaLocation(const TableIdentifier& table_ident,
+ const std::string& metadata_location) {
+ const auto ns = GetNamespace(this, table_ident.ns);
+ ICEBERG_RETURN_UNEXPECTED(ns);
+ if (!ns.value()->table_metadata_locations_.contains(table_ident.name)) {
+ return NotFound("{} does not exist", table_ident.name);
+ }
+ ns.value()->table_metadata_locations_[table_ident.name] = metadata_location;
+ return {};
+}
+
Review Comment:
```suggestion
```
ditto
##########
test/in_memory_catalog_test.cc:
##########
@@ -115,6 +114,50 @@ TEST_F(InMemoryCatalogTest, RegisterTable) {
ASSERT_EQ(table.value()->location(), "s3://bucket/test/location");
}
+TEST_F(InMemoryCatalogTest, RefreshTable) {
+ TableIdentifier tableIdent{.ns = {}, .name = "t1"};
+ std::shared_ptr<MockInMemoryCatalog> mock_catalog =
+ std::make_shared<MockInMemoryCatalog>(
+ "mock_catalog", file_io_, "/tmp/warehouse/",
+ std::unordered_map<std::string, std::string>());
+ auto table_location = GenerateTestTableLocation(tableIdent.name);
+ auto buildTable = [this, &tableIdent, &mock_catalog, &table_location](
+ int64_t snapshot_id, int64_t version) -> std::unique_ptr<Table> {
+ std::unique_ptr<TableMetadata> metadata;
+
+ ReadTableMetadata("TableMetadataV2Valid.json", &metadata);
Review Comment:
We can use simple steps for the test, for example:
```
auto metadata_v0 = std::make_shared<TableMetadata>(
{.format_version = 1,
.table_uuid = "1234567890",
...
});
auto metadata_v1 = ...;
// Use created two metadatas to create two tables
auto table_v0 = ...
auto table_v1 = ...
// Mock LoadTable like you did
EXPECT_CALL(*mock_catalog, LoadTable(::testing::_))
.WillOnce(::testing::Return(
::testing::ByMove(Result<std::unique_ptr<Table>>(std::move(table_v0)))))
.WillOnce(::testing::Return(
::testing::ByMove(Result<std::unique_ptr<Table>>(std::move(table_v1)))));
...
```
##########
src/iceberg/catalog/in_memory_catalog.cc:
##########
@@ -109,6 +109,15 @@ class ICEBERG_EXPORT InMemoryNamespace {
/// ErrorKind::kNoSuchTable if the table does not exist.
Status UnregisterTable(const TableIdentifier& table_ident);
+ /// \brief Updates the metadata location of an existing table.
+ ///
+ /// \param table_ident The fully qualified identifier of the table.
+ /// \param metadata_location The path to the table's metadata.
+ /// \return Status::OK if the table metadata location is updated;
+ /// Error otherwise.
+ Status UpdateTableMetaLocation(const TableIdentifier& table_ident,
+ const std::string& metadata_location);
+
Review Comment:
```suggestion
```
##########
test/mock_catalog.h:
##########
@@ -0,0 +1,36 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#pragma once
+
+#include "iceberg/catalog/in_memory_catalog.h"
+
+namespace iceberg {
+
+class ICEBERG_EXPORT MockInMemoryCatalog : public InMemoryCatalog {
Review Comment:
BTW, should we simply use `class MockCatalog : public Catalog` instead of extending `InMemoryCatalog`? Usually we mock everything of the catalog so we don't need anything from InMemoryCatalog.
##########
test/mock_catalog.h:
##########
@@ -0,0 +1,36 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#pragma once
+
+#include "iceberg/catalog/in_memory_catalog.h"
+
+namespace iceberg {
+
+class ICEBERG_EXPORT MockInMemoryCatalog : public InMemoryCatalog {
Review Comment:
```suggestion
class MockInMemoryCatalog : public InMemoryCatalog {
```
Test files do not need to add this.
##########
src/iceberg/catalog/in_memory_catalog.cc:
##########
@@ -109,6 +109,15 @@ class ICEBERG_EXPORT InMemoryNamespace {
/// ErrorKind::kNoSuchTable if the table does not exist.
Status UnregisterTable(const TableIdentifier& table_ident);
+ /// \brief Updates the metadata location of an existing table.
+ ///
+ /// \param table_ident The fully qualified identifier of the table.
+ /// \param metadata_location The path to the table's metadata.
+ /// \return Status::OK if the table metadata location is updated;
+ /// Error otherwise.
+ Status UpdateTableMetaLocation(const TableIdentifier& table_ident,
+ const std::string& metadata_location);
+
Review Comment:
This hack is no longer needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Refresh method support for table [iceberg-cpp]
Posted by "lishuxu (via GitHub)" <gi...@apache.org>.
lishuxu commented on code in PR #152:
URL: https://github.com/apache/iceberg-cpp/pull/152#discussion_r2249508319
##########
src/iceberg/catalog/in_memory_catalog.cc:
##########
@@ -109,6 +109,15 @@ class ICEBERG_EXPORT InMemoryNamespace {
/// ErrorKind::kNoSuchTable if the table does not exist.
Status UnregisterTable(const TableIdentifier& table_ident);
+ /// \brief Updates the metadata location of an existing table.
+ ///
+ /// \param table_ident The fully qualified identifier of the table.
+ /// \param metadata_location The path to the table's metadata.
+ /// \return Status::OK if the table metadata location is updated;
+ /// Error otherwise.
+ Status UpdateTableMetaLocation(const TableIdentifier& table_ident,
+ const std::string& metadata_location);
+
Review Comment:
Yes, this is no longer needed now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Refresh method support for table [iceberg-cpp]
Posted by "lishuxu (via GitHub)" <gi...@apache.org>.
lishuxu commented on code in PR #152:
URL: https://github.com/apache/iceberg-cpp/pull/152#discussion_r2265743712
##########
src/iceberg/catalog.h:
##########
@@ -175,7 +175,6 @@ class ICEBERG_EXPORT Catalog {
/// \return a Table instance or ErrorKind::kAlreadyExists if the table already exists
virtual Result<std::shared_ptr<Table>> RegisterTable(
const TableIdentifier& identifier, const std::string& metadata_file_location) = 0;
-
Review Comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: Refresh method support for table [iceberg-cpp]
Posted by "lishuxu (via GitHub)" <gi...@apache.org>.
lishuxu commented on code in PR #152:
URL: https://github.com/apache/iceberg-cpp/pull/152#discussion_r2268510058
##########
src/iceberg/table.cc:
##########
@@ -21,16 +21,36 @@
#include <algorithm>
+#include "iceberg/catalog.h"
#include "iceberg/partition_spec.h"
#include "iceberg/schema.h"
#include "iceberg/sort_order.h"
#include "iceberg/table_metadata.h"
#include "iceberg/table_scan.h"
+#include "iceberg/util/macros.h"
namespace iceberg {
const std::string& Table::uuid() const { return metadata_->table_uuid; }
+Status Table::Refresh() {
+ if (!catalog_) {
+ return NotSupported("Refresh is not supported for table without a catalog");
+ }
+
+ ICEBERG_ASSIGN_OR_RAISE(auto refreshed_table, catalog_->LoadTable(identifier_));
+ if (metadata_location_ != refreshed_table->metadata_location_) {
+ metadata_ = std::move(refreshed_table->metadata_);
+ metadata_location_ = std::move(refreshed_table->metadata_location_);
+ io_ = std::move(refreshed_table->io_);
+
+ schemas_map_.reset();
+ partition_spec_map_.reset();
+ sort_orders_map_.reset();
Review Comment:
Because this information is lazily initialized in the table, and the refreshed table hasn’t initialized these objects either, so a reset is sufficient.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org