Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: _getitem_at_placeholder checks #3368

Merged
merged 1 commit into from
Jan 16, 2025

Conversation

pfackeldey
Copy link
Collaborator

Some implementations of _getitem_at_placeholder have been wrong, e.g. Index classes can't be instances of PlaceholderArrays - only their .data can be.

Also a ListoffsetArray should return true if either its offsets or its contents contain PlaceholderArrays. Now that I think about it, that may be wrong from my side and the check should be only for the offsets (as originally intended). Let me know if this is the case, then I update this PR.

Copy link
Member

@jpivarski jpivarski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree 100%—only an Index's data can be a Placeholder, not an Index. Might this have been responsible for some spurious cases of trying to access PlaceholderArrays at runtime? (I haven't followed the logic through, but it seems like it might be related.)

@pfackeldey
Copy link
Collaborator Author

I agree 100%—only an Index's data can be a Placeholder, not an Index. Might this have been responsible for some spurious cases of trying to access PlaceholderArrays at runtime? (I haven't followed the logic through, but it seems like it might be related.)

Unfortunately not likely, unless a user manages to make a repr call be part of the dask graph with dask-awkward - which I doubt. Executing the DAG is non-interactive and there's not really a way to interact with the "materialized" awkward Array during a .compute() phase. _getitem_at_placeholder() is only used for constructing the repr of an awkward Array that may contain PlaceholderArrays as far as I understand.

Without this PR the repr of awkward Arrays that contain ListoffsetArrays (and some others that have Index instances on them) would lead to an error; I stumbled over this while playing around with dask-contrib/dask-awkward#565.

Copy link
Collaborator

@ianna ianna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pfackeldey - Great, thanks! Please, merge it if you finished with it. Thanks!

@pfackeldey pfackeldey merged commit f35dc08 into main Jan 16, 2025
39 checks passed
@pfackeldey pfackeldey deleted the pfackeldey/fix_getitem_at_placerholder branch January 16, 2025 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants