Uh oh!
There was an error while loading. Please reload this page.
GH-39010: [Python] Introduce maps_as_pydicts parameter for to_pylist, to_pydict, as_py - #45471
Conversation
Fix ExampleUuidScalarType Add tests for `maps_as_pydicts` Add test for duplicate map keys Formatting fixes Add docstring for 'maps_as_pydicts' Formatting fixes Call from_arrays from Table Fix last hopefully issues Correct MapScalar method "as_py" when there are multiple keys present
pitrou
commented
Feb 10, 2025
While this is not a bad idea in itself, it seems like the roundtripping concern could be solved more efficiently by making |
pitrou
commented
Feb 10, 2025
Also:
Please note that |
jonasdedden
commented
Feb 10, 2025
Let me clarify what this is about. Map fields are already createable with You can use
Yes, but this is part of a very large distributed machine learning setup, where relatively intricate filters applied on deeply nested list/struct/map columns. The compute of the actual machine learning outclasses the compute one has to do to deserialize Python objects by many orders of magnitude. For pure data queries, we would not use bare Python objects of course. |
pitrou
commented
Feb 10, 2025
I see, thanks. Then, do we want to reuse the same parameter signature as in the Pandas-related PR? I.e., allow either |
jonasdedden
commented
Feb 10, 2025
Sure, I actually also stumbled across that when I revisited that original Github issue. Before I do that, I'd like to ask whether you're generally fine with adding this new parameter to every |
pitrou
commented
Feb 10, 2025
That sounds ok to me. Ideally, |
pitrou
commented
Feb 10, 2025
By the way, we probably want to make the new parameter keyword-only? |
jonasdedden
commented
Feb 10, 2025
I addressed the remarks :) There is some weird error in the "Docs" job, I don't know what this is about. |
pitrou
commented
Feb 11, 2025
Hmm, it looks like some of the CI failures will need #45500 to be merged first |
jonasdedden
commented
Feb 13, 2025
I rebased the branch, now the CI tests seem fine again, I think? Could we get a approval/review of this? :) |
pitrou
left a comment
There was a problem hiding this comment.
Thanks @jonded94 ! This looks good on the principle, here are some assorted comments.
| This can change the ordering of (key, value) pairs, and will | ||
| deduplicate multiple keys, resulting in a possible loss of data. |
There was a problem hiding this comment.
I think the ordering comment is obsolete, as Python dicts are ordered nowadays. Unless the underlying implementation does something weird, ordering should therefore be preserved.
There was a problem hiding this comment.
Removed the ordering part, added some explanation of which value survives on duplicate keys.
Uh oh!
There was an error while loading. Please reload this page.
| Arrow Map, as in [(key1, value1), (key2, value2), ...]. | ||
| If 'lossy' or 'strict', convert Arrow Map arrays to native Python dicts. | ||
| This can change the ordering of (key, value) pairs, and will |
| with pytest.raises(ValueError): | ||
| assert s.as_py(maps_as_pydicts="strict") | ||
| assert s.as_py(maps_as_pydicts="lossy") == {'a': 2} |
There was a problem hiding this comment.
Can we check that a warning is actually emitted? See pytest.warns
There was a problem hiding this comment.
Implemented a check for this warning
Uh oh!
There was an error while loading. Please reload this page.
| raise ValueError( | ||
| "Invalid value for 'maps_as_pydicts': " | ||
| + "valid values are 'lossy', 'strict' or `None` (default). " | ||
| + f"Received '{maps_as_pydicts}'." |
There was a problem hiding this comment.
Nit: it may be more idiomatic to use the repr here
| + f"Received '{maps_as_pydicts}'." | |
| + f"Received {maps_as_pydicts!r}." |
There was a problem hiding this comment.
Implemented the suggested change
| for key, value in self: | ||
| if key in result_dict: | ||
| if maps_as_pydicts == "strict": | ||
| raise ValueError( |
There was a problem hiding this comment.
I would make this a KeyError. Also, the message should perhaps contain the duplicate key?
pitrou
commented
Feb 20, 2025
@github-actions crossbow submit -g python |
Revision: 93045c4 Submitted crossbow builds: ursacomputing/crossbow @ actions-9728f80818 |
pitrou
commented
Feb 20, 2025
CI failures are unrelated. |
Linchin
commented
Feb 24, 2025
Just fyi this might cause backward incompatibility issue because the user defined extension types are not expecting |
omatthew98
commented
Feb 25, 2025
We (Ray Data team) are also running into backward compatibility issues like this in our tests against pyarrow nightly with the same error mentioned here: [2025-02-25T06:27:20Z] ===================================FAILURES===================================--| [2025-02-25T06:27:20Z] ____________test_convert_to_pyarrow_array_object_ext_type_fallback____________| [2025-02-25T06:27:20Z]
| [2025-02-25T06:27:20Z] deftest_convert_to_pyarrow_array_object_ext_type_fallback():
| [2025-02-25T06:27:20Z] column_values=create_ragged_ndarray(
| [2025-02-25T06:27:20Z] [
| [2025-02-25T06:27:20Z] "hi",
| [2025-02-25T06:27:20Z] 1,
| [2025-02-25T06:27:20Z] None,
| [2025-02-25T06:27:20Z] [[[[]]]],
| [2025-02-25T06:27:20Z] {"a": [[{"b": 2, "c": UserObj(i=123)}]]},
| [2025-02-25T06:27:20Z] UserObj(i=456),
| [2025-02-25T06:27:20Z] ]
| [2025-02-25T06:27:20Z] )
| [2025-02-25T06:27:20Z] column_name="py_object_column"| [2025-02-25T06:27:20Z]
| [2025-02-25T06:27:20Z] # First, assert that straightforward conversion into Arrow native types fails| [2025-02-25T06:27:20Z] withpytest.raises(ArrowConversionError) asexc_info:
| [2025-02-25T06:27:20Z] _convert_to_pyarrow_native_array(column_values, column_name)
| [2025-02-25T06:27:20Z]
| [2025-02-25T06:27:20Z] assert (
| [2025-02-25T06:27:20Z] str(exc_info.value)
| [2025-02-25T06:27:20Z] =="Error converting data to Arrow: ['hi' 1 None list([[[[]]]]) {'a': [[{'b': 2, 'c': UserObj(i=123)}]]}\n UserObj(i=456)]"# noqa: E501| [2025-02-25T06:27:20Z] )
| [2025-02-25T06:27:20Z]
| [2025-02-25T06:27:20Z] # Subsequently, assert that fallback to `ArrowObjectExtensionType` succeeds| [2025-02-25T06:27:20Z] pa_array=convert_to_pyarrow_array(column_values, column_name)
| [2025-02-25T06:27:20Z]
| [2025-02-25T06:27:20Z] >assertpa_array.to_pylist() ==column_values.tolist()
| [2025-02-25T06:27:20Z]
| [2025-02-25T06:27:20Z] python/ray/air/tests/test_arrow.py:121:
| [2025-02-25T06:27:20Z] _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
| [2025-02-25T06:27:20Z]
| [2025-02-25T06:27:20Z] > ???
| [2025-02-25T06:27:20Z] ETypeError: as_py() gotanunexpectedkeywordargument'maps_as_pydicts' |
pitrou
commented
Feb 25, 2025
@Linchin@omatthew98 I think the way around this would be to take a For example turn this: classJSONArrowScalar(pa.ExtensionScalar):
defas_py(self):
returnJSONArray._deserialize_json(self.value.as_py() ifself.valueelseNone)into this: classJSONArrowScalar(pa.ExtensionScalar):
defas_py(self, **kwargs):
returnJSONArray._deserialize_json(self.value.as_py(**kwargs) ifself.valueelseNone) |
pitrou
commented
Feb 25, 2025
I've updated the PR description, we should remember to call out this potential incompatibility in the release notes for the next version. |
#51041) ## Why are these changes needed? Our tests with pyarrow nightly caught a backwards incompatibility bug with a [recent pyarrow change](apache/arrow#45471). To fix this we simply need to pass along kwargs in our `as_py` method as suggested by the pyarrow team [here](apache/arrow#45471 (comment)). --------- Signed-off-by: Matthew Owen <mowen@anyscale.com>
ray-project#51041) ## Why are these changes needed? Our tests with pyarrow nightly caught a backwards incompatibility bug with a [recent pyarrow change](apache/arrow#45471). To fix this we simply need to pass along kwargs in our `as_py` method as suggested by the pyarrow team [here](apache/arrow#45471 (comment)). --------- Signed-off-by: Matthew Owen <mowen@anyscale.com>
#51041) ## Why are these changes needed? Our tests with pyarrow nightly caught a backwards incompatibility bug with a [recent pyarrow change](apache/arrow#45471). To fix this we simply need to pass along kwargs in our `as_py` method as suggested by the pyarrow team [here](apache/arrow#45471 (comment)). --------- Signed-off-by: Matthew Owen <mowen@anyscale.com> Signed-off-by: Abrar Sheikh <abrar@anyscale.com>
ray-project#51041) ## Why are these changes needed? Our tests with pyarrow nightly caught a backwards incompatibility bug with a [recent pyarrow change](apache/arrow#45471). To fix this we simply need to pass along kwargs in our `as_py` method as suggested by the pyarrow team [here](apache/arrow#45471 (comment)). --------- Signed-off-by: Matthew Owen <mowen@anyscale.com>
Rationale for this change
Currently, unfortunately
MapScalar/Arraytypes are not deserialized into proper Pythondicts, which is unfortunate since this breaks "roundtrips" from Python -> Arrow -> Python:This is especially bad when storing TiBs of deeply nested data (think of lists in structs in maps...) that were created from Python and serialized into Arrow/Parquet, since they can't be read in again with native
pyarrowmethods without doing extremely ugly and computationally costly workarounds.What changes are included in this PR?
A new parameter
maps_as_pydictsis introduced toto_pylist,to_pydict,as_pywhich will allow proper roundtrips:Are these changes tested?
Yes. There are tests for
to_pylistandto_pydictincluded forpyarrow.Table, whilst low-levelMapScalarand especially a nesting withListScalarandStructScalaris tested.Also, duplicate keys now should throw an error, which is also tested for.
Are there any user-facing changes?
Yes. The
as_py()method on Scalar instances can be called with a new keyword argumentmaps_as_pydicts.As a consequence, if you implement your own Scalar subclass (for example for an extension type), you should change its signature to accept that new argument. For example this definition:
could be changed to: