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

The config_namespace! macro requires datafusion::common::Result to be in scope #14518

Closed
davisp opened this issue Feb 5, 2025 · 3 comments · Fixed by #14520
Closed

The config_namespace! macro requires datafusion::common::Result to be in scope #14518

davisp opened this issue Feb 5, 2025 · 3 comments · Fixed by #14520
Labels
bug Something isn't working

Comments

@davisp
Copy link
Member

davisp commented Feb 5, 2025

Describe the bug

Just got caught by this. If you don't have datafusion::common::Result in scope as Result when using config_namespace!, rust-analyzer returns diagnostics for enum takes 2 generic arguments but 1 generic argument was supplied. I ended up having to cargo expand the module and paste it into an editor to figure out what it was complaining about.

I'm pretty sure I just need to fully specify the Result type here:

fn set(&mut self, key: &str, value: &str) -> Result<()> {

As in, just prefix it with ::datafusion::common:: on that line.

To Reproduce

use datafusion::common::config::{ConfigField, Visit};
use datafusion::common::config_namespace;
use datafusion::common::error::_config_err;

// This has to be uncommented to fix the compiler error.
// use datafusion::common::Result;

config_namespace! {
    /// Options for controlling TileDB specific optimizations
    pub struct ExampleOptions {
        /// Control whether TileDB will attempt to do predicate pushdown.
        /// Setting this to false will disable all predicate push down analysis
        /// and rely on DataFusion to filter returned data. Generally speaking,
        /// this should only be disabled when testing predicate pushdown
        /// logic.
        pub some_setting: String, default = String::new()
    }
}

Expected behavior

I would expect config_namespace! to work whether or not I have a specific identifier in scope.

Additional context

I'm more than happy to post a PR for this as long as someone Ok's the proposed solution (or can point me at a better approach).

@davisp davisp added the bug Something isn't working label Feb 5, 2025
@rkrishn7
Copy link
Contributor

rkrishn7 commented Feb 5, 2025

Your solution makes sense to me @davisp! Consider using the $crate metavariable within the macro definition.

Also, it looks like the macro was removed as an export in #14188 🤔 . We'll likely want to add back the #[macro_export] attribute if there's use of it outside datafusion_common.

@logan-keede
Copy link
Contributor

you are right, removing #[macro_export] does not seem to be related to the issue I was trying to solve.
It was a miss on my part. Perhaps @davisp can fix that, while solving this issue.

Also, is it the cause of this issue? it seems like we can still load the macro just not by #[macro_use].(I haven't use macros much in rust.)

@davisp
Copy link
Member Author

davisp commented Feb 5, 2025

@rkrishn7 Awesome, I'll open a PR in a few minutes. Thanks for the reminder on $crate being a thing.

@logan-keede I'll add it back, no problem! Also that's not the cause of this issue so no worries there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants