You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "a-agmon (via GitHub)" <gi...@apache.org> on 2024/05/08 05:43:28 UTC
[PR] added serde aliases to support both forms conventions [iceberg-rust]
a-agmon opened a new pull request, #365:
URL: https://github.com/apache/iceberg-rust/pull/365
This PR fixes #338
The PR adds serde aliases to fields that were renamed in https://github.com/apache/iceberg/pull/5338 (merged in 1.5.0), in order to support both conventions/versions - i.e., new and previous naming conventions.
In the current implementation of Iceberg-Rust, deserialization is done using field names rather than field id, and therefore manifests written using newer Iceberg lib are not supported.
--
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] fix (manifest-list): added serde aliases to support both forms conventions [iceberg-rust]
Posted by "a-agmon (via GitHub)" <gi...@apache.org>.
a-agmon commented on PR #365:
URL: https://github.com/apache/iceberg-rust/pull/365#issuecomment-2101597092
Added test files and test.
Thanks @sdd
--
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] fix (manifest-list): added serde aliases to support both forms conventions [iceberg-rust]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on PR #365:
URL: https://github.com/apache/iceberg-rust/pull/365#issuecomment-2102592152
> BTW, it would be nice to change the field names of the ManifestFileV2 struct to the 'correct' ones (for example, pub added_data_files_count to pub added_files_count but it would require changes in other modules.
I would like that as well, maybe we can do that in a separate PR so we can get this one in.
> With respect to the more long term solution that resolves the field name by field-id, I have patched this for my own use case, and will be happy to PR, but would be grateful for some "pre-ruling" on the direction and design of the solution. Mostly because there some open questions about this, like the use of apace_avro crate etc.
I think the solution that you posted makes sense. In PyIceberg we decided, in the end, to implement our own Avro reader to make these things easier. What we do there is convert an Avro schema directly to an Iceberg schema and use an Iceberg Avro reader (that you would also use for data-files), to read the Avro files into the right struct`s.
--
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] fix (manifest-list): added serde aliases to support both forms conventions [iceberg-rust]
Posted by "sdd (via GitHub)" <gi...@apache.org>.
sdd commented on PR #365:
URL: https://github.com/apache/iceberg-rust/pull/365#issuecomment-2099979115
Looks good, at least until we move to field IDs. But could we add a test data file to check the aliases work? 👍🏼
--
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] fix (manifest-list): added serde aliases to support both forms conventions [iceberg-rust]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko merged PR #365:
URL: https://github.com/apache/iceberg-rust/pull/365
--
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] fix (manifest-list): added serde aliases to support both forms conventions [iceberg-rust]
Posted by "zeodtr (via GitHub)" <gi...@apache.org>.
zeodtr commented on PR #365:
URL: https://github.com/apache/iceberg-rust/pull/365#issuecomment-2102243287
@a-agmon I think the PR is simple and clear.
BTW, it would be nice to change the field names of the `ManifestFileV2` struct to the 'correct' ones (for example, `pub added_data_files_count` to `pub added_files_count` but it would require changes in other modules.
--
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] fix (manifest-list): added serde aliases to support both forms conventions [iceberg-rust]
Posted by "a-agmon (via GitHub)" <gi...@apache.org>.
a-agmon commented on PR #365:
URL: https://github.com/apache/iceberg-rust/pull/365#issuecomment-2100100759
> Looks good, at least until we move to field IDs. But could we add a test data file to check the aliases work? 👍🏼
Absolutely. Thanks
--
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] fix (manifest-list): added serde aliases to support both forms conventions [iceberg-rust]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #365:
URL: https://github.com/apache/iceberg-rust/pull/365#discussion_r1594775872
##########
crates/iceberg/src/spec/manifest_list.rs:
##########
@@ -802,6 +802,9 @@ pub(super) mod _serde {
pub key_metadata: Option<ByteBuf>,
}
+ // Aliases were added to fields that were renemaed in Iceberg 1.5.0 (https://github.com/apache/iceberg/pull/5338), in order to support both conventions/versions.
Review Comment:
```suggestion
// Aliases were added to fields that were renamed in Iceberg 1.5.0 (https://github.com/apache/iceberg/pull/5338), in order to support both conventions/versions.
```
--
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] fix (manifest-list): added serde aliases to support both forms conventions [iceberg-rust]
Posted by "a-agmon (via GitHub)" <gi...@apache.org>.
a-agmon commented on PR #365:
URL: https://github.com/apache/iceberg-rust/pull/365#issuecomment-2102212707
> I'm okay with this, but before the next release, we need to get field-ID resolution in place. That's a very important part of Iceberg. In this case, we stumbled into a naming difference of Spark, but it might also be that other engines produce slightly different names.
Thanks @Fokko ,
Typo fixed and Yes, this is indeed aimed to be an interim solution, though the test and test files should be good to help us test a more robust solution.
With respect to the more long term solution that resolves the field name by field-id, I have patched this for my own use case, and will be happy to PR, but would be grateful for some "pre-ruling" on the direction and design of the solution. Mostly because there some open questions about this, like the use of apace_avro crate etc.
Anyways, here is the note about the suggested direction
https://github.com/apache/iceberg-rust/issues/338#issuecomment-2095268461
--
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