You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "atwam (via GitHub)" <gi...@apache.org> on 2024/01/31 10:19:32 UTC
[PR] pyarrow: Preserve RecordBatch's schema metadata [arrow-rs]
atwam opened a new pull request, #5355:
URL: https://github.com/apache/arrow-rs/pull/5355
PyArrow's RecordBatch gets imported by using a StructArray, then transforming it into a RecordBatch. This was losing the Schema's metadata in the process (because a StructArray does not hold metadata).
This commit changes the test to show the issue, and fixes it.
# Which issue does this PR close?
Closes #5354.
# What changes are included in this PR?
Changes pyarrow tests to ensure that schema equality also checks for metadata equality.
Fixes `impl FromPyArrow for RecordBatch` to ensure that the resulting RecordBatch has the right schema
# Are there any user-facing changes?
No
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] pyarrow: Preserve RecordBatch's schema metadata [arrow-rs]
Posted by "kylebarron (via GitHub)" <gi...@apache.org>.
kylebarron commented on code in PR #5355:
URL: https://github.com/apache/arrow-rs/pull/5355#discussion_r1473114374
##########
arrow/src/pyarrow.rs:
##########
@@ -355,7 +355,12 @@ impl FromPyArrow for RecordBatch {
));
}
let array = StructArray::from(array_data);
- return Ok(array.into());
+ // StructArray does not embed metadata from schema. We need to override
+ // the output schema with the schema from the capsule.
+ let schema = Arc::new(Schema::try_from(schema_ptr).map_err(to_py_err)?);
+ return RecordBatch::from(array)
+ .with_schema(schema)
+ .map_err(to_py_err);
Review Comment:
Instead of first inferring a schema from the record batch and then overwriting it, maybe it would be clearer to [access the columns](https://docs.rs/arrow-array/50.0.0/src/arrow_array/record_batch.rs.html#513) and then use `RecordBatch::try_new`?
```rs
let schema = Arc::new(Schema::try_from(schema_ptr).map_err(to_py_err)?);
let (_fields, columns, nulls) = array.into_parts();
// Assert null count is 0
return RecordBatch::try_new(schema, columns).map_err(to_py_err);
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] pyarrow: Preserve RecordBatch's schema metadata [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #5355:
URL: https://github.com/apache/arrow-rs/pull/5355
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] pyarrow: Preserve RecordBatch's schema metadata [arrow-rs]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #5355:
URL: https://github.com/apache/arrow-rs/pull/5355#issuecomment-1927362825
> I have more changes pending that use `schema.equals(other, check_metadata=True)` for testing, but these fail because of [apache/arrow#39865](https://github.com/apache/arrow/issues/39865). Will wait for the cpp code to be fixed
FTR, the C++ side was [fixed](https://github.com/apache/arrow/pull/39866) now.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] pyarrow: Preserve RecordBatch's schema metadata [arrow-rs]
Posted by "atwam (via GitHub)" <gi...@apache.org>.
atwam commented on PR #5355:
URL: https://github.com/apache/arrow-rs/pull/5355#issuecomment-1919647592
I have more changes pending that actually use `schema.equals(other, check_metadata=True)`, which seems to be a better test, but these fail until https://github.com/apache/arrow/issues/39865 is fixed (see discussion in #5355 )
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] pyarrow: Preserve RecordBatch's schema metadata [arrow-rs]
Posted by "atwam (via GitHub)" <gi...@apache.org>.
atwam commented on PR #5355:
URL: https://github.com/apache/arrow-rs/pull/5355#issuecomment-1927368440
> > I have more changes pending that use `schema.equals(other, check_metadata=True)` for testing, but these fail because of [apache/arrow#39865](https://github.com/apache/arrow/issues/39865). Will wait for the cpp code to be fixed
>
> FTR, the C++ side was [fixed](https://github.com/apache/arrow/pull/39866) now.
Great, will create a quick pr tomorrow with the better/fixed tests. We may as well use a proper equality test.
Thanks a lot for your help here!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] pyarrow: Preserve RecordBatch's schema metadata [arrow-rs]
Posted by "atwam (via GitHub)" <gi...@apache.org>.
atwam commented on code in PR #5355:
URL: https://github.com/apache/arrow-rs/pull/5355#discussion_r1473136727
##########
arrow/src/pyarrow.rs:
##########
@@ -355,7 +355,12 @@ impl FromPyArrow for RecordBatch {
));
}
let array = StructArray::from(array_data);
- return Ok(array.into());
+ // StructArray does not embed metadata from schema. We need to override
+ // the output schema with the schema from the capsule.
+ let schema = Arc::new(Schema::try_from(schema_ptr).map_err(to_py_err)?);
+ return RecordBatch::from(array)
+ .with_schema(schema)
+ .map_err(to_py_err);
Review Comment:
Excellent point, I should have looked at the conversion code from `StructArray`. Fixed.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] pyarrow: Preserve RecordBatch's schema metadata [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #5355:
URL: https://github.com/apache/arrow-rs/pull/5355#issuecomment-1925747094
Thanks everyone
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org