wgtmac commented on code in PR #417:
URL: https://github.com/apache/iceberg-cpp/pull/417#discussion_r2634064034
##########
src/iceberg/catalog/rest/rest_catalog.cc:
##########
@@ -77,8 +78,8 @@ Result<CatalogConfig> FetchServerConfig(const ResourcePaths&
paths,
RestCatalog::~RestCatalog() = default;
-Result<std::unique_ptr<RestCatalog>> RestCatalog::Make(
- const RestCatalogProperties& config) {
+Result<std::shared_ptr<RestCatalog>> RestCatalog::Make(
+ const RestCatalogProperties& config, std::shared_ptr<FileIO> file_io) {
Review Comment:
We need to make sure `file_io` is not null.
##########
src/iceberg/test/rest_catalog_test.cc:
##########
@@ -120,14 +125,15 @@ class RestCatalogIntegrationTest : public ::testing::Test
{
void TearDown() override {}
// Helper function to create a REST catalog instance
- Result<std::unique_ptr<RestCatalog>> CreateCatalog() {
+ Result<std::shared_ptr<RestCatalog>> CreateCatalog() {
auto config = RestCatalogProperties::default_properties();
config
->Set(RestCatalogProperties::kUri,
std::format("{}:{}", kLocalhostUri, kRestCatalogPort))
.Set(RestCatalogProperties::kName, std::string(kCatalogName))
.Set(RestCatalogProperties::kWarehouse, std::string(kWarehouseName));
- return RestCatalog::Make(*config);
+ auto file_io = std::make_shared<test::StdFileIO>();
+ return RestCatalog::Make(*config, file_io);
Review Comment:
```suggestion
return RestCatalog::Make(*config, std::make_shared<test::StdFileIO>());
```
##########
src/iceberg/catalog/rest/json_internal.cc:
##########
@@ -336,6 +339,57 @@ Result<ListTablesResponse>
ListTablesResponseFromJson(const nlohmann::json& json
return response;
}
+nlohmann::json ToJson(const CreateTableRequest& request) {
+ nlohmann::json json;
+ json[kName] = request.name;
+ SetOptionalStringField(json, kLocation, request.location);
+ if (request.schema) {
+ json[kSchema] = ToJson(*request.schema);
+ }
+ if (request.partition_spec) {
+ json[kPartitionSpec] = ToJson(*request.partition_spec);
+ }
+ if (request.write_order) {
+ json[kWriteOrder] = ToJson(*request.write_order);
+ }
+ if (request.stage_create) {
+ json[kStageCreate] = request.stage_create;
+ }
+ SetContainerField(json, kProperties, request.properties);
+ return json;
+}
+
+Result<CreateTableRequest> CreateTableRequestFromJson(const nlohmann::json&
json) {
+ CreateTableRequest request;
+ ICEBERG_ASSIGN_OR_RAISE(request.name, GetJsonValue<std::string>(json,
kName));
+ ICEBERG_ASSIGN_OR_RAISE(request.location,
+ GetJsonValueOrDefault<std::string>(json, kLocation));
+ ICEBERG_ASSIGN_OR_RAISE(auto schema_json, GetJsonValue<nlohmann::json>(json,
kSchema));
+ ICEBERG_ASSIGN_OR_RAISE(request.schema, SchemaFromJson(schema_json));
Review Comment:
```suggestion
ICEBERG_ASSIGN_OR_RAISE(auto schema, GetJsonValue<nlohmann::json>(json,
kSchema));
ICEBERG_ASSIGN_OR_RAISE(request.schema, SchemaFromJson(schema));
```
##########
src/iceberg/test/rest_catalog_test.cc:
##########
@@ -337,4 +343,61 @@ TEST_F(RestCatalogIntegrationTest, DropNamespace) {
EXPECT_FALSE(*exists_result);
}
+TEST_F(RestCatalogIntegrationTest, CreateTable) {
+ auto catalog_result = CreateCatalog();
+ ASSERT_THAT(catalog_result, IsOk());
+ auto& catalog = catalog_result.value();
+
+ // Create nested namespace with properties
+ Namespace ns{.levels = {"test_create_table", "apple", "ios"}};
+ std::unordered_map<std::string, std::string> ns_properties{{"owner", "ray"},
+ {"community",
"apache"}};
+
+ // Create parent namespaces first
+ auto status = catalog->CreateNamespace(Namespace{.levels =
{"test_create_table"}}, {});
Review Comment:
Not required by this PR: we can add some common functions to quickly create
the catalog and namespace since these are reused in different cases.
##########
src/iceberg/catalog/rest/json_internal.cc:
##########
@@ -336,6 +339,57 @@ Result<ListTablesResponse>
ListTablesResponseFromJson(const nlohmann::json& json
return response;
}
+nlohmann::json ToJson(const CreateTableRequest& request) {
+ nlohmann::json json;
+ json[kName] = request.name;
+ SetOptionalStringField(json, kLocation, request.location);
+ if (request.schema) {
+ json[kSchema] = ToJson(*request.schema);
+ }
+ if (request.partition_spec) {
+ json[kPartitionSpec] = ToJson(*request.partition_spec);
+ }
+ if (request.write_order) {
+ json[kWriteOrder] = ToJson(*request.write_order);
+ }
+ if (request.stage_create) {
+ json[kStageCreate] = request.stage_create;
+ }
+ SetContainerField(json, kProperties, request.properties);
+ return json;
+}
+
+Result<CreateTableRequest> CreateTableRequestFromJson(const nlohmann::json&
json) {
+ CreateTableRequest request;
+ ICEBERG_ASSIGN_OR_RAISE(request.name, GetJsonValue<std::string>(json,
kName));
+ ICEBERG_ASSIGN_OR_RAISE(request.location,
+ GetJsonValueOrDefault<std::string>(json, kLocation));
+ ICEBERG_ASSIGN_OR_RAISE(auto schema_json, GetJsonValue<nlohmann::json>(json,
kSchema));
+ ICEBERG_ASSIGN_OR_RAISE(request.schema, SchemaFromJson(schema_json));
+
+ if (json.contains(kPartitionSpec)) {
+ ICEBERG_ASSIGN_OR_RAISE(auto partition_spec,
+ GetJsonValue<nlohmann::json>(json,
kPartitionSpec));
+ ICEBERG_ASSIGN_OR_RAISE(request.partition_spec,
+ PartitionSpecFromJson(request.schema,
partition_spec,
+
PartitionSpec::kInitialSpecId));
+ }
+ if (json.contains(kWriteOrder)) {
+ ICEBERG_ASSIGN_OR_RAISE(auto sort_order_json,
+ GetJsonValue<nlohmann::json>(json, kWriteOrder));
+ ICEBERG_ASSIGN_OR_RAISE(request.write_order,
+ SortOrderFromJson(sort_order_json,
request.schema));
Review Comment:
```suggestion
ICEBERG_ASSIGN_OR_RAISE(auto sort_order,
GetJsonValue<nlohmann::json>(json, kWriteOrder));
ICEBERG_ASSIGN_OR_RAISE(request.write_order,
SortOrderFromJson(sort_order, request.schema));
```
##########
src/iceberg/catalog/rest/json_internal.cc:
##########
@@ -336,6 +339,57 @@ Result<ListTablesResponse>
ListTablesResponseFromJson(const nlohmann::json& json
return response;
}
+nlohmann::json ToJson(const CreateTableRequest& request) {
Review Comment:
Please add test cases for new functions.
--
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]
