HeartLinked commented on code in PR #296:
URL: https://github.com/apache/iceberg-cpp/pull/296#discussion_r2554789043
##########
src/iceberg/test/rest_catalog_test.cc:
##########
@@ -61,7 +61,7 @@ class RestCatalogTest : public ::testing::Test {
RestCatalogConfig config_;
};
-TEST_F(RestCatalogTest, MakeCatalogSuccess) {
+TEST_F(RestCatalogTest, DISABLED_MakeCatalogSuccess) {
Review Comment:
Please see the comment above in line 33-34. This is a temporary integration
test that requires a local REST server, such as the Docker image
apache/iceberg-rest-fixture provided by Apache Iceberg. Currently, this method
allows testing locally, but it obviously cannot pass the Github CI process, so
it is a temporary expedient. In this first version PR, we clearly cannot
comprehensively complete such a large amount of work. If you are interested,
you can help complete this issue:
https://github.com/apache/iceberg-cpp/issues/333 :)
##########
src/iceberg/test/rest_catalog_test.cc:
##########
@@ -82,7 +82,7 @@ TEST_F(RestCatalogTest, DISABLED_MakeCatalogEmptyUri) {
EXPECT_THAT(catalog_result, HasErrorMessage("uri"));
}
-TEST_F(RestCatalogTest, MakeCatalogWithCustomProperties) {
+TEST_F(RestCatalogTest, DISABLED_MakeCatalogWithCustomProperties) {
Review Comment:
Same reply as above.
##########
src/iceberg/test/rest_catalog_test.cc:
##########
@@ -61,7 +61,7 @@ class RestCatalogTest : public ::testing::Test {
RestCatalogConfig config_;
};
-TEST_F(RestCatalogTest, MakeCatalogSuccess) {
+TEST_F(RestCatalogTest, DISABLED_MakeCatalogSuccess) {
Review Comment:
Please see the comment above in line 33-34. This is a temporary integration
test that requires a local REST server, such as the Docker image
apache/iceberg-rest-fixture provided by Apache Iceberg. Currently, this method
allows testing locally, but it obviously cannot pass the Github CI process, so
it is a temporary expedient. In this first version PR, we clearly cannot
comprehensively complete such a large amount of work. If you are interested,
you can help complete this issue:
https://github.com/apache/iceberg-cpp/issues/333 :)
##########
src/iceberg/catalog/rest/config.h:
##########
@@ -0,0 +1,69 @@
+/*
+ * 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 <memory>
+#include <string>
+#include <unordered_map>
+
+#include <cpr/cprtypes.h>
+
+#include "iceberg/catalog/rest/iceberg_rest_export.h"
+#include "iceberg/result.h"
+#include "iceberg/util/config.h"
+
+/// \file iceberg/catalog/rest/config.h
+/// \brief RestCatalogConfig implementation for Iceberg REST API.
+
+namespace iceberg::rest {
+
+/// \brief Configuration class for a REST Catalog.
+class ICEBERG_REST_EXPORT RestCatalogConfig : public
ConfigBase<RestCatalogConfig> {
+ public:
+ template <typename T>
+ using Entry = const ConfigBase<RestCatalogConfig>::Entry<T>;
+
+ /// \brief The URI of the REST catalog server.
+ inline static std::string_view kUri{"uri"};
+
+ /// \brief The name of the catalog.
+ inline static std::string_view kName{"name"};
+
+ /// \brief The warehouse path.
+ inline static std::string_view kWarehouse{"warehouse"};
Review Comment:
Same reply as above.
##########
src/iceberg/catalog/rest/config.h:
##########
@@ -0,0 +1,69 @@
+/*
+ * 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 <memory>
+#include <string>
+#include <unordered_map>
+
+#include <cpr/cprtypes.h>
+
+#include "iceberg/catalog/rest/iceberg_rest_export.h"
+#include "iceberg/result.h"
+#include "iceberg/util/config.h"
+
+/// \file iceberg/catalog/rest/config.h
+/// \brief RestCatalogConfig implementation for Iceberg REST API.
+
+namespace iceberg::rest {
+
+/// \brief Configuration class for a REST Catalog.
+class ICEBERG_REST_EXPORT RestCatalogConfig : public
ConfigBase<RestCatalogConfig> {
+ public:
+ template <typename T>
+ using Entry = const ConfigBase<RestCatalogConfig>::Entry<T>;
+
+ /// \brief The URI of the REST catalog server.
+ inline static std::string_view kUri{"uri"};
+
+ /// \brief The name of the catalog.
+ inline static std::string_view kName{"name"};
Review Comment:
According to @wgtmac 's advice, we have change this to an entry like `inline
static Entry<std::string> kUri{"uri", ""};`, then this can not be constexpr.
##########
src/iceberg/test/rest_catalog_test.cc:
##########
@@ -93,7 +93,7 @@ TEST_F(RestCatalogTest, MakeCatalogWithCustomProperties) {
EXPECT_THAT(catalog_result, IsOk());
}
-TEST_F(RestCatalogTest, ListNamespaces) {
+TEST_F(RestCatalogTest, DISABLED_ListNamespaces) {
Review Comment:
Same reply as above.
##########
src/iceberg/test/rest_catalog_test.cc:
##########
@@ -82,7 +82,7 @@ TEST_F(RestCatalogTest, DISABLED_MakeCatalogEmptyUri) {
EXPECT_THAT(catalog_result, HasErrorMessage("uri"));
}
-TEST_F(RestCatalogTest, MakeCatalogWithCustomProperties) {
+TEST_F(RestCatalogTest, DISABLED_MakeCatalogWithCustomProperties) {
Review Comment:
Same reply as above.
##########
src/iceberg/test/rest_catalog_test.cc:
##########
@@ -93,7 +93,7 @@ TEST_F(RestCatalogTest, MakeCatalogWithCustomProperties) {
EXPECT_THAT(catalog_result, IsOk());
}
-TEST_F(RestCatalogTest, ListNamespaces) {
+TEST_F(RestCatalogTest, DISABLED_ListNamespaces) {
Review Comment:
Same reply as above.
--
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]
