You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "wgtmac (via GitHub)" <gi...@apache.org> on 2025/04/18 15:23:38 UTC
[PR] test: add table metadata deserialization test [iceberg-cpp]
wgtmac opened a new pull request, #82:
URL: https://github.com/apache/iceberg-cpp/pull/82
- Added a set of test files from https://github.com/apache/iceberg/tree/main/core/src/test/resources
- Fixed table metadata deserialization
- Added singleton for unsorted sort order and unpartitioned spec
- Added test case for deserializing valid v1 and v2 metadata files
--
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] test: add table metadata deserialization test [iceberg-cpp]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #82:
URL: https://github.com/apache/iceberg-cpp/pull/82#discussion_r2051786196
##########
.pre-commit-config.yaml:
##########
@@ -33,6 +33,7 @@ repos:
rev: v19.1.5
hooks:
- id: clang-format
+ exclude: ^test/resources/.*\.json$
Review Comment:
Does clang want to format json? 🤔
--
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] test: add table metadata deserialization test [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on code in PR #82:
URL: https://github.com/apache/iceberg-cpp/pull/82#discussion_r2051417742
##########
src/iceberg/sort_order.cc:
##########
@@ -21,14 +21,21 @@
#include <format>
+#include "iceberg/constants.h"
#include "iceberg/util/formatter.h" // IWYU pragma: keep
namespace iceberg {
-SortOrder::SortOrder(int64_t order_id, std::vector<SortField> fields)
+SortOrder::SortOrder(int32_t order_id, std::vector<SortField> fields)
: order_id_(order_id), fields_(std::move(fields)) {}
-int64_t SortOrder::order_id() const { return order_id_; }
+const std::shared_ptr<SortOrder>& SortOrder::Unsorted() {
+ static const std::shared_ptr<SortOrder> unsorted =
+ std::make_shared<SortOrder>(kInitialSortOrderId, std::vector<SortField>{});
Review Comment:
```suggestion
std::make_shared<SortOrder>(kInitialSortOrderId - 1, std::vector<SortField>{});
```
Java impl use 0 as UNSORTED_ORDER order_id, so here should minus 1.
--
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] test: add table metadata deserialization test [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #82:
URL: https://github.com/apache/iceberg-cpp/pull/82#discussion_r2051421227
##########
src/iceberg/sort_order.cc:
##########
@@ -21,14 +21,21 @@
#include <format>
+#include "iceberg/constants.h"
#include "iceberg/util/formatter.h" // IWYU pragma: keep
namespace iceberg {
-SortOrder::SortOrder(int64_t order_id, std::vector<SortField> fields)
+SortOrder::SortOrder(int32_t order_id, std::vector<SortField> fields)
: order_id_(order_id), fields_(std::move(fields)) {}
-int64_t SortOrder::order_id() const { return order_id_; }
+const std::shared_ptr<SortOrder>& SortOrder::Unsorted() {
+ static const std::shared_ptr<SortOrder> unsorted =
+ std::make_shared<SortOrder>(kInitialSortOrderId, std::vector<SortField>{});
Review Comment:
Good catch!
--
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] test: add table metadata deserialization test [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on code in PR #82:
URL: https://github.com/apache/iceberg-cpp/pull/82#discussion_r2051497357
##########
src/iceberg/table_metadata.cc:
##########
@@ -34,6 +38,33 @@ std::string ToString(const MetadataLogEntry& entry) {
entry.metadata_file);
}
+const std::shared_ptr<Schema>& TableMetadata::Schema() const {
+ static const std::shared_ptr<::iceberg::Schema> empty_schema = nullptr;
Review Comment:
I once had the same concern, but this method returns a reference, I guess that's the reason?
--
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] test: add table metadata deserialization test [iceberg-cpp]
Posted by "gty404 (via GitHub)" <gi...@apache.org>.
gty404 commented on code in PR #82:
URL: https://github.com/apache/iceberg-cpp/pull/82#discussion_r2051452269
##########
src/iceberg/table_metadata.cc:
##########
@@ -34,6 +38,33 @@ std::string ToString(const MetadataLogEntry& entry) {
entry.metadata_file);
}
+const std::shared_ptr<Schema>& TableMetadata::Schema() const {
+ static const std::shared_ptr<::iceberg::Schema> empty_schema = nullptr;
Review Comment:
Why not return a truly empty schema object, but null?
--
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] test: add table metadata deserialization test [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #82:
URL: https://github.com/apache/iceberg-cpp/pull/82#discussion_r2051661537
##########
src/iceberg/table_metadata.cc:
##########
@@ -34,6 +38,33 @@ std::string ToString(const MetadataLogEntry& entry) {
entry.metadata_file);
}
+const std::shared_ptr<Schema>& TableMetadata::Schema() const {
+ static const std::shared_ptr<::iceberg::Schema> empty_schema = nullptr;
Review Comment:
I switched to wrap the return in a Result so we can get an explicit `NotFound` error.
--
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] test: add table metadata deserialization test [iceberg-cpp]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo merged PR #82:
URL: https://github.com/apache/iceberg-cpp/pull/82
--
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] test: add table metadata deserialization test [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #82:
URL: https://github.com/apache/iceberg-cpp/pull/82#discussion_r2051878617
##########
.pre-commit-config.yaml:
##########
@@ -33,6 +33,7 @@ repos:
rev: v19.1.5
hooks:
- id: clang-format
+ exclude: ^test/resources/.*\.json$
Review Comment:
Yes, without this change the pre-commit fails and complains that `clang-format` does not support JSON file.
--
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