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

downcast_primitive_array and downcast_dictionary_array are not hygienic wrt imports #6400

Closed
davidhewitt opened this issue Sep 16, 2024 · 7 comments · Fixed by #6620
Closed
Labels
arrow Changes to the arrow crate enhancement Any new improvement worthy of a entry in the changelog

Comments

@davidhewitt
Copy link

Describe the bug

Using downcast_primitive_array and downcast_dictionary_array require arrow_schema to be in scope of the expanded code.

To Reproduce

  1. use arrow::downcast_primitive_array without arrow_schema included as a direct dependency
  2. failure to compile due to expanded code using paths from arrow_schema

Expected behavior

Compile should succeed.

Additional context

A workaround is to manually insert something which roughly matches arrow_schema into the namespace, e.g.

    use datafusion::arrow::datatypes as arrow_schema;  // using datafusion facade
    downcast_primitive_array!( ... ); // ^ will now use the above-defined arrow_schema
@tustvold tustvold added enhancement Any new improvement worthy of a entry in the changelog and removed bug labels Sep 16, 2024
@tustvold
Copy link
Contributor

tustvold commented Sep 16, 2024

This isn't a bug, it's inherent to the nature of macros and the limitations thereof. I'm not aware of any way to avoid this. If you wish to use a macro, you have to have the defining crate in scope

@davidhewitt
Copy link
Author

I broadly agree, though in this case the defining crate is arrow_array (or arrow), but it references a separate crate arrow_schema.

error[E0433]: failed to resolve: use of undeclared crate or module `arrow_schema`
 --> src/main.rs:5:5
  |
5 |     arrow::downcast_primitive_array!(array => { });
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ use of undeclared crate or module `arrow_schema`

@tustvold
Copy link
Contributor

Yeah, this is an artifact of when the crates were split apart. I suppose we could add a private re-export of arrow-schema within arrow-array for this specific case, but you'd still not be able to use the macro from a top-level crate like datafusion or arrow.

@davidhewitt
Copy link
Author

If the re-export was public but hidden, then it would roughly fix the issue.

Another option could be to make the top-level arrow crate definite a separate copy of these macros rather than re-export.

Neither solution is particularly satisfying :(

@alamb
Copy link
Contributor

alamb commented Sep 17, 2024

TLDR I think we should make this work even if it is non ideal (like a copy of the macro) as it is a usability papercut

In general, I think most/many users use the arrow crate (not one of the subcrates) and therefore I believe anything we can do to make that usage easier (e.g. re-exporting things from the subcrates / improving macros, etc) is valuable overall

I don't think we should prevent / make it harder for people to use the subcrates (doing so reduces the dependency burden on downstream projects) but I think using subcrates has a higher barrier to entry. I think it is important to make it as easy as possible for people to start using arrow and then if they find it useful they can spend time to optimize their usage more

@tustvold
Copy link
Contributor

I wonder if we could create a meta-macro that generates the downcast_primitive_array macro, this would avoid duplication, and allow redefining the downcast macro when re-exporting 🤔

tustvold added a commit to tustvold/arrow-rs that referenced this issue Oct 24, 2024
tustvold added a commit to tustvold/arrow-rs that referenced this issue Oct 24, 2024
alamb added a commit that referenced this issue Nov 8, 2024
* Make downcast macros hygenic (#6400)

* Format

* Update arrow-array/src/cast.rs

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

* fix fmt

---------

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
@alamb alamb added the arrow Changes to the arrow crate label Nov 16, 2024
@alamb
Copy link
Contributor

alamb commented Nov 16, 2024

label_issue.py automatically added labels {'arrow'} from #6620

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow Changes to the arrow crate enhancement Any new improvement worthy of a entry in the changelog
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants