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-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
