-
Notifications
You must be signed in to change notification settings - Fork 76
refactor: unify lazy init for table #344
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
ff3a3f9 to
96741af
Compare
| Result<TableMetadataCache::SchemasMap> TableMetadataCache::InitSchemasMap( | ||
| const TableMetadata* metadata) { | ||
| SchemasMap schemas_map; | ||
| schemas_map.reserve(metadata->schemas.size()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just FYI. We can use std::ranges::to in #include <ranges> and don't need to call reserve. For example:
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>();
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
../src/iceberg/table_metadata.cc: In static member function ‘static iceberg::Result<std::unordered_map<int, std::shared_ptriceberg::Schema, std::hash, std::equal_to, std::allocator<std::pair<const int, std::shared_ptriceberg::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();
| ^~
../src/iceberg/table_metadata.cc:194:36: error: expected primary-expression before ‘>’ token
194 | std::ranges::to();
| ^
../src/iceberg/table_metadata.cc:194:38: error: expected primary-expression before ‘)’ token
194 | std::ranges::to();
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
HeartLinked
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
This reverts commit 5e4a108.
Fokko
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, that looks much cleaner @wgtmac 🙌
|
Thanks @HuaHuaY @HeartLinked @Fokko @zhjwpku for the review! |
Introduced a
TableMetadataCacheto support lazy initialization of mappings for schemas, partition specs, sort orders and snapshots.