Uh oh!
There was an error while loading. Please reload this page.
GH-38575: [Python] Include metadata when creating pa.schema from PyCapsule - #41538
Conversation
7c98632 to
d7d54caCompareUh oh!
There was an error while loading. Please reload this page.
jorisvandenbossche
left a comment
There was a problem hiding this comment.
Thanks for the contribution!
There was a problem hiding this comment.
What you additionally need to test is pa.schema(wrapped_schema, metadata=..), i.e. passing metadata to the pa.schema(..) constructor function when passing a schema object to it (the test as you edited it now will already pass, I think, and so not cover the regression)
There was a problem hiding this comment.
Ah, thanks. I tried pushing just the test first hoping CI would run them but got lazy when it didn't. :)
5b99daf to
5556af6Comparejorisvandenbossche
left a comment
There was a problem hiding this comment.
Thanks for the update!
| wrapped_schema = Wrapper(schema) | ||
| assert pa.schema(wrapped_schema) == schema | ||
| assert pa.schema(wrapped_schema).metadata == {b"a": b"b"} |
There was a problem hiding this comment.
Added this additional line, because assert pa.schema(wrapped_schema) == schema (the line above) in itself does not check the metadata (that gets ignored in basic equality testing)
JacobHayes
commented
May 16, 2024
Thanks for the help @jorisvandenbossche - I need to spend a bit more time getting my dev pyarrow builds working so I'm not just shooting in the dark 🙈 |
After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit 6a9e2d5. There were 5 benchmark results indicating a performance regression:
The full Conbench report has more details. It also includes information about 3 possible false positives for unstable benchmarks that are known to sometimes produce them. |
…m PyCapsule (apache#41538) ### Rationale for this change Fixes the dropped `pa.schema` metadata reported in apache#38575, which was introduced in apache#37797. ### What changes are included in this PR? Passes through the `metadata` to the short-circuited `Schema` created with `_import_from_c_capsule`. ### Are these changes tested? Yes - added `metadata` to the existing test. ### Are there any user-facing changes? I'm not sure this quite rises to the `(b) a bug that caused incorrect or invalid data to be produced,` condition, but I added that note to be safe since the resulting schema is "incorrect" (and broke some round-trip tests on my end after a pyarrow update): **This PR contains a "Critical Fix".** * GitHub Issue: apache#38575 Lead-authored-by: Jacob Hayes <jacob.r.hayes@gmail.com> Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com> Signed-off-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
Rationale for this change
Fixes the dropped
pa.schemametadata reported in #38575, which was introduced in #37797.What changes are included in this PR?
Passes through the
metadatato the short-circuitedSchemacreated with_import_from_c_capsule.Are these changes tested?
Yes - added
metadatato the existing test.Are there any user-facing changes?
I'm not sure this quite rises to the
(b) a bug that caused incorrect or invalid data to be produced,condition, but I added that note to be safe since the resulting schema is "incorrect" (and broke some round-trip tests on my end after a pyarrow update):This PR contains a "Critical Fix".