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/11/26 07:46:16 UTC
[PR] refactor: unify lazy init for table [iceberg-cpp]
wgtmac opened a new pull request, #344:
URL: https://github.com/apache/iceberg-cpp/pull/344
(no comment)
--
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] refactor: unify lazy init for table [iceberg-cpp]
Posted by "HuaHuaY (via GitHub)" <gi...@apache.org>.
HuaHuaY commented on code in PR #344:
URL: https://github.com/apache/iceberg-cpp/pull/344#discussion_r2565686508
##########
src/iceberg/table_metadata.cc:
##########
@@ -162,6 +162,70 @@ bool operator==(const TableMetadata& lhs, const TableMetadata& rhs) {
lhs.next_row_id == rhs.next_row_id;
}
+// TableMetadataCache implementation
+
+Result<TableMetadataCache::SchemasMapRef> TableMetadataCache::GetSchemasById() const {
+ return schemas_map_.Get(metadata_);
+}
+
+Result<TableMetadataCache::PartitionSpecsMapRef>
+TableMetadataCache::GetPartitionSpecsById() const {
+ return partition_specs_map_.Get(metadata_);
+}
+
+Result<TableMetadataCache::SortOrdersMapRef> TableMetadataCache::GetSortOrdersById()
+ const {
+ return sort_orders_map_.Get(metadata_);
+}
+
+Result<TableMetadataCache::SnapshotsMapRef> TableMetadataCache::GetSnapshotsById() const {
+ return snapshot_map_.Get(metadata_);
+}
+
+Result<TableMetadataCache::SchemasMap> TableMetadataCache::InitSchemasMap(
+ const TableMetadata* metadata) {
+ SchemasMap schemas_map;
+ schemas_map.reserve(metadata->schemas.size());
Review Comment:
Sad. `std::ranges::to` is supported by gcc 14. But some iceberg-cpp's dependencies can't be compiled by gcc 14, we can only use gcc 13 in ci.
--
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] refactor: unify lazy init for table [iceberg-cpp]
Posted by "HuaHuaY (via GitHub)" <gi...@apache.org>.
HuaHuaY commented on code in PR #344:
URL: https://github.com/apache/iceberg-cpp/pull/344#discussion_r2565686508
##########
src/iceberg/table_metadata.cc:
##########
@@ -162,6 +162,70 @@ bool operator==(const TableMetadata& lhs, const TableMetadata& rhs) {
lhs.next_row_id == rhs.next_row_id;
}
+// TableMetadataCache implementation
+
+Result<TableMetadataCache::SchemasMapRef> TableMetadataCache::GetSchemasById() const {
+ return schemas_map_.Get(metadata_);
+}
+
+Result<TableMetadataCache::PartitionSpecsMapRef>
+TableMetadataCache::GetPartitionSpecsById() const {
+ return partition_specs_map_.Get(metadata_);
+}
+
+Result<TableMetadataCache::SortOrdersMapRef> TableMetadataCache::GetSortOrdersById()
+ const {
+ return sort_orders_map_.Get(metadata_);
+}
+
+Result<TableMetadataCache::SnapshotsMapRef> TableMetadataCache::GetSnapshotsById() const {
+ return snapshot_map_.Get(metadata_);
+}
+
+Result<TableMetadataCache::SchemasMap> TableMetadataCache::InitSchemasMap(
+ const TableMetadata* metadata) {
+ SchemasMap schemas_map;
+ schemas_map.reserve(metadata->schemas.size());
Review Comment:
Sad. `std::ranges::to` is supported by gcc 14. But we can use gcc 13 in ci.
I test in my local environment. `vendoredarrow-src` can be compiled by gcc 14 but can not be compiled by clang base on gcc 14. The difference is cmake will generate different compile flags. Clang will report that `atomic_load` is deprecated.
Maybe we can upgrade gcc's version in ci from 13 to 14.
##########
src/iceberg/table_metadata.cc:
##########
@@ -162,6 +162,70 @@ bool operator==(const TableMetadata& lhs, const TableMetadata& rhs) {
lhs.next_row_id == rhs.next_row_id;
}
+// TableMetadataCache implementation
+
+Result<TableMetadataCache::SchemasMapRef> TableMetadataCache::GetSchemasById() const {
+ return schemas_map_.Get(metadata_);
+}
+
+Result<TableMetadataCache::PartitionSpecsMapRef>
+TableMetadataCache::GetPartitionSpecsById() const {
+ return partition_specs_map_.Get(metadata_);
+}
+
+Result<TableMetadataCache::SortOrdersMapRef> TableMetadataCache::GetSortOrdersById()
+ const {
+ return sort_orders_map_.Get(metadata_);
+}
+
+Result<TableMetadataCache::SnapshotsMapRef> TableMetadataCache::GetSnapshotsById() const {
+ return snapshot_map_.Get(metadata_);
+}
+
+Result<TableMetadataCache::SchemasMap> TableMetadataCache::InitSchemasMap(
+ const TableMetadata* metadata) {
+ SchemasMap schemas_map;
+ schemas_map.reserve(metadata->schemas.size());
Review Comment:
Sad. `std::ranges::to` is supported by gcc 14. But we use gcc 13 in ci.
I test in my local environment. `vendoredarrow-src` can be compiled by gcc 14 but can not be compiled by clang base on gcc 14. The difference is cmake will generate different compile flags. Clang will report that `atomic_load` is deprecated.
Maybe we can upgrade gcc's version in ci from 13 to 14.
--
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] refactor: unify lazy init for table [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #344:
URL: https://github.com/apache/iceberg-cpp/pull/344#discussion_r2565537042
##########
src/iceberg/table_metadata.cc:
##########
@@ -162,6 +162,70 @@ bool operator==(const TableMetadata& lhs, const TableMetadata& rhs) {
lhs.next_row_id == rhs.next_row_id;
}
+// TableMetadataCache implementation
+
+Result<TableMetadataCache::SchemasMapRef> TableMetadataCache::GetSchemasById() const {
+ return schemas_map_.Get(metadata_);
+}
+
+Result<TableMetadataCache::PartitionSpecsMapRef>
+TableMetadataCache::GetPartitionSpecsById() const {
+ return partition_specs_map_.Get(metadata_);
+}
+
+Result<TableMetadataCache::SortOrdersMapRef> TableMetadataCache::GetSortOrdersById()
+ const {
+ return sort_orders_map_.Get(metadata_);
+}
+
+Result<TableMetadataCache::SnapshotsMapRef> TableMetadataCache::GetSnapshotsById() const {
+ return snapshot_map_.Get(metadata_);
+}
+
+Result<TableMetadataCache::SchemasMap> TableMetadataCache::InitSchemasMap(
+ const TableMetadata* metadata) {
+ SchemasMap schemas_map;
+ schemas_map.reserve(metadata->schemas.size());
Review Comment:
../src/iceberg/table_metadata.cc: In static member function ‘static iceberg::Result<std::unordered_map<int, std::shared_ptr<iceberg::Schema>, std::hash<int>, std::equal_to<int>, std::allocator<std::pair<const int, std::shared_ptr<iceberg::Schema> > > > > iceberg::TableMetadataCache::InitSchemasMap(const iceberg::TableMetadata*)’:
../src/iceberg/table_metadata.cc:194:23: error: ‘to’ is not a member of ‘std::ranges’
194 | std::ranges::to<SchemasMap>();
| ^~
../src/iceberg/table_metadata.cc:194:36: error: expected primary-expression before ‘>’ token
194 | std::ranges::to<SchemasMap>();
| ^
../src/iceberg/table_metadata.cc:194:38: error: expected primary-expression before ‘)’ token
194 | std::ranges::to<SchemasMap>();
|
--
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] refactor: unify lazy init for table [iceberg-cpp]
Posted by "HuaHuaY (via GitHub)" <gi...@apache.org>.
HuaHuaY commented on code in PR #344:
URL: https://github.com/apache/iceberg-cpp/pull/344#discussion_r2564633305
##########
src/iceberg/table_metadata.cc:
##########
@@ -162,6 +162,70 @@ bool operator==(const TableMetadata& lhs, const TableMetadata& rhs) {
lhs.next_row_id == rhs.next_row_id;
}
+// TableMetadataCache implementation
+
+Result<TableMetadataCache::SchemasMapRef> TableMetadataCache::GetSchemasById() const {
+ return schemas_map_.Get(metadata_);
+}
+
+Result<TableMetadataCache::PartitionSpecsMapRef>
+TableMetadataCache::GetPartitionSpecsById() const {
+ return partition_specs_map_.Get(metadata_);
+}
+
+Result<TableMetadataCache::SortOrdersMapRef> TableMetadataCache::GetSortOrdersById()
+ const {
+ return sort_orders_map_.Get(metadata_);
+}
+
+Result<TableMetadataCache::SnapshotsMapRef> TableMetadataCache::GetSnapshotsById() const {
+ return snapshot_map_.Get(metadata_);
+}
+
+Result<TableMetadataCache::SchemasMap> TableMetadataCache::InitSchemasMap(
+ const TableMetadata* metadata) {
+ SchemasMap schemas_map;
+ schemas_map.reserve(metadata->schemas.size());
Review Comment:
Just a suggestion. We can use `std::ranges::to` in `#include <ranges>` and don't need to call `reserve`. For example:
```cpp
Result<TableMetadataCache::SchemasMap> TableMetadataCache::InitSchemasMap(
const TableMetadata* metadata) {
return metadata->schemas | std::views::transform([](auto& schema) {
return std::pair(schema->schema_id().value(), schema);
}) |
std::ranges::to<SchemasMap>();
}
```
--
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] refactor: unify lazy init for table [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac merged PR #344:
URL: https://github.com/apache/iceberg-cpp/pull/344
--
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] refactor: unify lazy init for table [iceberg-cpp]
Posted by "HuaHuaY (via GitHub)" <gi...@apache.org>.
HuaHuaY commented on code in PR #344:
URL: https://github.com/apache/iceberg-cpp/pull/344#discussion_r2565686508
##########
src/iceberg/table_metadata.cc:
##########
@@ -162,6 +162,70 @@ bool operator==(const TableMetadata& lhs, const TableMetadata& rhs) {
lhs.next_row_id == rhs.next_row_id;
}
+// TableMetadataCache implementation
+
+Result<TableMetadataCache::SchemasMapRef> TableMetadataCache::GetSchemasById() const {
+ return schemas_map_.Get(metadata_);
+}
+
+Result<TableMetadataCache::PartitionSpecsMapRef>
+TableMetadataCache::GetPartitionSpecsById() const {
+ return partition_specs_map_.Get(metadata_);
+}
+
+Result<TableMetadataCache::SortOrdersMapRef> TableMetadataCache::GetSortOrdersById()
+ const {
+ return sort_orders_map_.Get(metadata_);
+}
+
+Result<TableMetadataCache::SnapshotsMapRef> TableMetadataCache::GetSnapshotsById() const {
+ return snapshot_map_.Get(metadata_);
+}
+
+Result<TableMetadataCache::SchemasMap> TableMetadataCache::InitSchemasMap(
+ const TableMetadata* metadata) {
+ SchemasMap schemas_map;
+ schemas_map.reserve(metadata->schemas.size());
Review Comment:
Sad. `std::ranges::to` is supported by gcc 14. But iceberg-cpp's dependence can't be compiled by gcc 14, we can only use gcc 13 in ci.
--
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] refactor: unify lazy init for table [iceberg-cpp]
Posted by "HuaHuaY (via GitHub)" <gi...@apache.org>.
HuaHuaY commented on code in PR #344:
URL: https://github.com/apache/iceberg-cpp/pull/344#discussion_r2564633305
##########
src/iceberg/table_metadata.cc:
##########
@@ -162,6 +162,70 @@ bool operator==(const TableMetadata& lhs, const TableMetadata& rhs) {
lhs.next_row_id == rhs.next_row_id;
}
+// TableMetadataCache implementation
+
+Result<TableMetadataCache::SchemasMapRef> TableMetadataCache::GetSchemasById() const {
+ return schemas_map_.Get(metadata_);
+}
+
+Result<TableMetadataCache::PartitionSpecsMapRef>
+TableMetadataCache::GetPartitionSpecsById() const {
+ return partition_specs_map_.Get(metadata_);
+}
+
+Result<TableMetadataCache::SortOrdersMapRef> TableMetadataCache::GetSortOrdersById()
+ const {
+ return sort_orders_map_.Get(metadata_);
+}
+
+Result<TableMetadataCache::SnapshotsMapRef> TableMetadataCache::GetSnapshotsById() const {
+ return snapshot_map_.Get(metadata_);
+}
+
+Result<TableMetadataCache::SchemasMap> TableMetadataCache::InitSchemasMap(
+ const TableMetadata* metadata) {
+ SchemasMap schemas_map;
+ schemas_map.reserve(metadata->schemas.size());
Review Comment:
Just FYI. We can use `std::ranges::to` in `#include <ranges>` and don't need to call `reserve`. For example:
```cpp
Result<TableMetadataCache::SchemasMap> TableMetadataCache::InitSchemasMap(
const TableMetadata* metadata) {
return metadata->schemas | std::views::transform([](auto& schema) {
return std::pair(schema->schema_id().value(), schema);
}) |
std::ranges::to<SchemasMap>();
}
```
--
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] refactor: unify lazy init for table [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #344:
URL: https://github.com/apache/iceberg-cpp/pull/344#issuecomment-3583866812
Thanks @HuaHuaY @HeartLinked @Fokko @zhjwpku for the review!
--
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] refactor: unify lazy init for table [iceberg-cpp]
Posted by "HuaHuaY (via GitHub)" <gi...@apache.org>.
HuaHuaY commented on code in PR #344:
URL: https://github.com/apache/iceberg-cpp/pull/344#discussion_r2565686508
##########
src/iceberg/table_metadata.cc:
##########
@@ -162,6 +162,70 @@ bool operator==(const TableMetadata& lhs, const TableMetadata& rhs) {
lhs.next_row_id == rhs.next_row_id;
}
+// TableMetadataCache implementation
+
+Result<TableMetadataCache::SchemasMapRef> TableMetadataCache::GetSchemasById() const {
+ return schemas_map_.Get(metadata_);
+}
+
+Result<TableMetadataCache::PartitionSpecsMapRef>
+TableMetadataCache::GetPartitionSpecsById() const {
+ return partition_specs_map_.Get(metadata_);
+}
+
+Result<TableMetadataCache::SortOrdersMapRef> TableMetadataCache::GetSortOrdersById()
+ const {
+ return sort_orders_map_.Get(metadata_);
+}
+
+Result<TableMetadataCache::SnapshotsMapRef> TableMetadataCache::GetSnapshotsById() const {
+ return snapshot_map_.Get(metadata_);
+}
+
+Result<TableMetadataCache::SchemasMap> TableMetadataCache::InitSchemasMap(
+ const TableMetadata* metadata) {
+ SchemasMap schemas_map;
+ schemas_map.reserve(metadata->schemas.size());
Review Comment:
Sad. `std::ranges::to` is supported by gcc 14. But iceberg-cpp's dependency can't be compiled by gcc 14, we can only use gcc 13 in ci.
--
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