You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "jonathanc-n (via GitHub)" <gi...@apache.org> on 2025/03/31 22:21:37 UTC
[PR] fix: Preserve list name in schema conversion [iceberg-rust]
jonathanc-n opened a new pull request, #1149:
URL: https://github.com/apache/iceberg-rust/pull/1149
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
- part of #1039 .
## What changes are included in this PR?
Preserve list name in serialization and deserialization.
<!--
Provide a summary of the modifications in this PR. List the main changes such as new features, bug fixes, refactoring, or any other updates.
-->
## Are these changes tested?
unit test
<!--
Specify what test covers (unit test, integration test, etc.).
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
--
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: Preserve list name in schema conversion [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #1149:
URL: https://github.com/apache/iceberg-rust/pull/1149#discussion_r2022859479
##########
crates/iceberg/src/spec/datatypes.rs:
##########
@@ -633,11 +633,11 @@ impl NestedField {
}
/// Construct list type's element field.
- pub fn list_element(id: i32, field_type: Type, required: bool) -> Self {
+ pub fn list_element(id: i32, field_type: Type, field_name: String, required: bool) -> Self {
Review Comment:
I prefer to have two methods:
1. `list_element` method unchanged.
2. `list_element` with name, which accepts an extra name.
For the behavior, I didn't find equivalent things in java/python, but I think it's reasonable as iceberg spec didn't enforce that list type element's field name must be `element`. cc @Fokko @sdd WDYT?
--
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: Preserve list name in schema conversion [iceberg-rust]
Posted by "jonathanc-n (via GitHub)" <gi...@apache.org>.
jonathanc-n closed pull request #1149: fix: Preserve list name in schema conversion
URL: https://github.com/apache/iceberg-rust/pull/1149
--
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: Preserve list name in schema conversion [iceberg-rust]
Posted by "jonathanc-n (via GitHub)" <gi...@apache.org>.
jonathanc-n commented on code in PR #1149:
URL: https://github.com/apache/iceberg-rust/pull/1149#discussion_r2021852830
##########
crates/iceberg/src/spec/datatypes.rs:
##########
@@ -633,11 +633,11 @@ impl NestedField {
}
/// Construct list type's element field.
- pub fn list_element(id: i32, field_type: Type, required: bool) -> Self {
+ pub fn list_element(id: i32, field_type: Type, field_name: String, required: bool) -> Self {
Review Comment:
Should this be an `Option<String>` or is this fine. The alternative is that if `None` we can pass in `LIST_FIELD_NAME`
--
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: Preserve list name in schema conversion [iceberg-rust]
Posted by "jonathanc-n (via GitHub)" <gi...@apache.org>.
jonathanc-n commented on PR #1149:
URL: https://github.com/apache/iceberg-rust/pull/1149#issuecomment-2767669247
Would like to request some help here. I believe the catalog is running into an error while deserializing the response to query the catalog.
Here is the message:
```thread 'shared_tests::scan_all_type::test_scan_all_type' panicked at crates/integration_tests/tests/shared_tests/scan_all_type.rs:141:10:
called `Result::unwrap()` on an `Err` value: Unexpected => Failed to parse response from rest catalog server```
--
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: Preserve list name in schema conversion [iceberg-rust]
Posted by "ZENOTME (via GitHub)" <gi...@apache.org>.
ZENOTME commented on code in PR #1149:
URL: https://github.com/apache/iceberg-rust/pull/1149#discussion_r2024035494
##########
crates/iceberg/src/spec/datatypes.rs:
##########
@@ -633,11 +633,11 @@ impl NestedField {
}
/// Construct list type's element field.
- pub fn list_element(id: i32, field_type: Type, required: bool) -> Self {
+ pub fn list_element(id: i32, field_type: Type, field_name: String, required: bool) -> Self {
Review Comment:
If users want a custom list element name, can they use `required` or `optional` directly? Like arrow does: https://docs.rs/arrow/53.4.1/arrow/datatypes/struct.Field.html#method.new
Same as map
--
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: Preserve list name in schema conversion [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on PR #1149:
URL: https://github.com/apache/iceberg-rust/pull/1149#issuecomment-2772512366
> Would like to request some help here. I believe the catalog is running into an error while deserializing the response to query the catalog.
>
> Here is the message:
>
> ```
> called `Result::unwrap()` on an `Err` value: Unexpected => Failed to parse response from rest catalog server
> ```
Hi, I think the error is caused by the change in serialized json format of list type.
I think we can't allow user to have customized list element name as it required by iceberg spec in serialized format: https://github.com/apache/iceberg-rust?tab=readme-ov-file#supported-rust-version
--
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: Preserve list name in schema conversion [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on PR #1149:
URL: https://github.com/apache/iceberg-rust/pull/1149#issuecomment-2772517504
cc @jonathanc-n We should close this?
--
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