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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
