You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "FANNG1 (via GitHub)" <gi...@apache.org> on 2024/08/23 13:19:04 UTC
[PR] [#575] correct partition-id to field id in PartitionSpec [iceberg-rust]
FANNG1 opened a new pull request, #576:
URL: https://github.com/apache/iceberg-rust/pull/576
fixes: #575
--
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: correct partition-id to field-id in UnboundPartitionField [iceberg-rust]
Posted by "FANNG1 (via GitHub)" <gi...@apache.org>.
FANNG1 commented on code in PR #576:
URL: https://github.com/apache/iceberg-rust/pull/576#discussion_r1729685905
##########
crates/iceberg/src/spec/table_metadata.rs:
##########
@@ -1293,7 +1293,7 @@ mod tests {
name: "x".to_string(),
transform: Transform::Identity,
source_id: 1,
- partition_id: Some(1000),
+ field_id: Some(1000),
Review Comment:
By the way, I think `add_partition_field` is more general to use, seems `add_unbound_field` should only be used in test to specify partition field id.
--
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: correct partition-id to field-id in UnboundPartitionField [iceberg-rust]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #576:
URL: https://github.com/apache/iceberg-rust/pull/576#discussion_r1729151672
##########
crates/iceberg/src/spec/table_metadata.rs:
##########
@@ -1293,7 +1293,7 @@ mod tests {
name: "x".to_string(),
transform: Transform::Identity,
source_id: 1,
- partition_id: Some(1000),
+ field_id: Some(1000),
Review Comment:
I'm a bit surprised that the test against `testdata/table_metadata/TableMetadataV2Valid.json` passed. Would you like to conduct a deep investigation?
--
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: correct partition-id to field-id in UnboundPartitionField [iceberg-rust]
Posted by "FANNG1 (via GitHub)" <gi...@apache.org>.
FANNG1 commented on code in PR #576:
URL: https://github.com/apache/iceberg-rust/pull/576#discussion_r1729678815
##########
crates/iceberg/src/spec/table_metadata.rs:
##########
@@ -1293,7 +1293,7 @@ mod tests {
name: "x".to_string(),
transform: Transform::Identity,
source_id: 1,
- partition_id: Some(1000),
+ field_id: Some(1000),
Review Comment:
When building `PartitionSpec`, `UnboundPartitionField` is transformed to `PartitionField`, in which `partition_id` is transformed `field_id`
```rust
bound_fields.push(PartitionField {
source_id: field.source_id,
field_id: partition_id,
name: field.name,
transform: field.transform,
})
```
https://github.com/apache/iceberg-rust/blob/main/crates/iceberg/src/spec/partition.rs#L415-L420
--
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: correct partition-id to field-id in UnboundPartitionField [iceberg-rust]
Posted by "FANNG1 (via GitHub)" <gi...@apache.org>.
FANNG1 commented on code in PR #576:
URL: https://github.com/apache/iceberg-rust/pull/576#discussion_r1729678815
##########
crates/iceberg/src/spec/table_metadata.rs:
##########
@@ -1293,7 +1293,7 @@ mod tests {
name: "x".to_string(),
transform: Transform::Identity,
source_id: 1,
- partition_id: Some(1000),
+ field_id: Some(1000),
Review Comment:
When build PartitionSpec, `UnboundPartitionField` is transformed to `PartitionField`, in which `partition_id` is transformed `field_id`
```rust
bound_fields.push(PartitionField {
source_id: field.source_id,
field_id: partition_id,
name: field.name,
transform: field.transform,
})
```
https://github.com/apache/iceberg-rust/blob/main/crates/iceberg/src/spec/partition.rs#L415-L420
--
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] [#575] correct partition-id to field-id in UnboundPartitionField [iceberg-rust]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #576:
URL: https://github.com/apache/iceberg-rust/pull/576#discussion_r1729151672
##########
crates/iceberg/src/spec/table_metadata.rs:
##########
@@ -1293,7 +1293,7 @@ mod tests {
name: "x".to_string(),
transform: Transform::Identity,
source_id: 1,
- partition_id: Some(1000),
+ field_id: Some(1000),
Review Comment:
I'm a bit surprised that the test against `testdata/table_metadata/TableMetadataV2Valid.json` passed.
--
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: correct partition-id to field-id in UnboundPartitionField [iceberg-rust]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #576:
URL: https://github.com/apache/iceberg-rust/pull/576#discussion_r1729151672
##########
crates/iceberg/src/spec/table_metadata.rs:
##########
@@ -1293,7 +1293,7 @@ mod tests {
name: "x".to_string(),
transform: Transform::Identity,
source_id: 1,
- partition_id: Some(1000),
+ field_id: Some(1000),
Review Comment:
I'm a bit surprised that the test against `testdata/table_metadata/TableMetadataV2Valid.json` passed before. Would you like to conduct a deep investigation?
--
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: correct partition-id to field-id in UnboundPartitionField [iceberg-rust]
Posted by "FANNG1 (via GitHub)" <gi...@apache.org>.
FANNG1 commented on code in PR #576:
URL: https://github.com/apache/iceberg-rust/pull/576#discussion_r1729188927
##########
crates/iceberg/src/spec/table_metadata.rs:
##########
@@ -1293,7 +1293,7 @@ mod tests {
name: "x".to_string(),
transform: Transform::Identity,
source_id: 1,
- partition_id: Some(1000),
+ field_id: Some(1000),
Review Comment:
sure
--
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: correct partition-id to field-id in UnboundPartitionField [iceberg-rust]
Posted by "FANNG1 (via GitHub)" <gi...@apache.org>.
FANNG1 commented on code in PR #576:
URL: https://github.com/apache/iceberg-rust/pull/576#discussion_r1729785163
##########
crates/iceberg/src/spec/table_metadata.rs:
##########
@@ -1293,7 +1293,7 @@ mod tests {
name: "x".to_string(),
transform: Transform::Identity,
source_id: 1,
- partition_id: Some(1000),
+ field_id: Some(1000),
Review Comment:
ok, 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: correct partition-id to field-id in UnboundPartitionField [iceberg-rust]
Posted by "FANNG1 (via GitHub)" <gi...@apache.org>.
FANNG1 commented on code in PR #576:
URL: https://github.com/apache/iceberg-rust/pull/576#discussion_r1729685905
##########
crates/iceberg/src/spec/table_metadata.rs:
##########
@@ -1293,7 +1293,7 @@ mod tests {
name: "x".to_string(),
transform: Transform::Identity,
source_id: 1,
- partition_id: Some(1000),
+ field_id: Some(1000),
Review Comment:
By the way, I think `add_partition_field` is more general to use, seems `add_unbound_field` should only be used in test to specify partition field id, we should remove `pub ` from `add_unbound_field`.
--
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: correct partition-id to field-id in UnboundPartitionField [iceberg-rust]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #576:
URL: https://github.com/apache/iceberg-rust/pull/576#discussion_r1729727371
##########
crates/iceberg/src/spec/table_metadata.rs:
##########
@@ -1293,7 +1293,7 @@ mod tests {
name: "x".to_string(),
transform: Transform::Identity,
source_id: 1,
- partition_id: Some(1000),
+ field_id: Some(1000),
Review Comment:
Thanks, would you like to submit a new issue for disucsion. I believe this PR is good to go.
--
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: correct partition-id to field-id in UnboundPartitionField [iceberg-rust]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo merged PR #576:
URL: https://github.com/apache/iceberg-rust/pull/576
--
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