GH-50847: [Python] Add Type_RUN_END_ENCODED to _NESTED_TYPES set - #50848
Open
Nishuuzz wants to merge 1 commit into
Open
GH-50847: [Python] Add Type_RUN_END_ENCODED to _NESTED_TYPES set#50848Nishuuzz wants to merge 1 commit into
Nishuuzz wants to merge 1 commit into
Conversation
pyarrow.types.is_nested() answered False for run-end encoded types while the C++ arrow::is_nested() answers True for them. The Python predicate reads from a hardcoded _NESTED_TYPES set that never had Type_RUN_END_ENCODED added to it; run-end encoded was the only type the two definitions still disagreed about. Also extend test_is_nested_or_struct with map and dictionary, so the set is pinned at both ends rather than only for the types that are nested.
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale for this change
pyarrow.types.is_nested()said a run-end encoded type wasn't nested, while the C++arrow::is_nested()says it is:The Python side reads from a hardcoded
_NESTED_TYPESset inpython/pyarrow/types.pyrather than from the C++ trait, so it has to be kept in step by hand. Lining that set up againstis_nested()incpp/src/arrow/type_traits.h, run-end encoded was the only type the two still disagreed about — the list-view types and fixed-size list are already there.It's also out of step with the type itself. Run-end encoded has two children, the run ends and the values, and every other type in pyarrow that has children answers
Truehere.This is the same thing that happened to fixed-size list in #40171, fixed by #40172, and the list-view types were added after that. This looks like the last one missed when run-end encoding went in.
What changes are included in this PR?
Adds
Type_RUN_END_ENCODEDto_NESTED_TYPES, which is the whole fix.In the test I've added the run-end encoded case, and while I was there also a map and a dictionary. Map was nested already but wasn't asserted anywhere, and dictionary is the interesting negative — it wraps a value type but is deliberately not nested in either implementation, so pinning it means a later change can't quietly sweep it in.
Are these changes tested?
Yes.
test_is_nested_or_structfails on the current code and passes with the change.I don't have a local C++ build, so I checked this by running the updated
test_types.pyagainst an installed pyarrow 25.0.0 with the same one-line change applied to itstypes.py. Before the change that file had 87 passing withtest_is_nested_or_structfailing; after it, 88 passing. The two errors and one failure I see in both runs are environmental on my machine and unrelated — the errors are thepickle_modulefixture, which comes from a conftest I wasn't loading, and the failure istest_pytz_timezone_roundtrip.Are there any user-facing changes?
Yes, though it's small.
pa.types.is_nested()now returnsTruefor run-end encoded types where it previously returnedFalse. Anything branching on that predicate will take the nested path for these types, which is the intended answer and what the C++ implementation has always given. Nothing inside pyarrow readsis_nestedor_NESTED_TYPES, so the effect is limited to callers.