Skip to content

Conversation

@alamb
Copy link
Contributor

@alamb alamb commented Jun 21, 2023

Suggested by @comphead on https://github.com/apache/arrow-datafusion/pull/6721/files#r1235622262

I made some macros and then had trouble using them. Issues I ran into:

  1. The macros return Result<..> rather than DataFusionError so they can't be used in all places (e.g the one @comphead suggested in https://github.com/apache/arrow-datafusion/pull/6721/files#r1235622262)
  2. Many errors are created using format!(...) which I couldn't figure out how to make into a nice macro invocation

Maybe someone else has some ideas

Copy link
Contributor

@comphead comphead left a comment

Choose a reason for hiding this comment

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

I tried that

            if val.is_null() {
                plan_err!(
                    "Invalid window frame: start bound cannot be UNBOUNDED FOLLOWING"
                )?
            }
        } else if let WindowFrameBound::Preceding(val) = &end_bound {
            if val.is_null() {
                plan_err!(
                    "Invalid window frame: end bound cannot be UNBOUNDED PRECEDING"
                )?
            }

Looks more concise. Reg to format! checking if that can be done better

@comphead
Copy link
Contributor

comphead commented Jun 21, 2023

#[macro_export]
macro_rules! with_dollar_sign {
    ($($body:tt)*) => {
        macro_rules! __with_dollar_sign { $($body)* }
        __with_dollar_sign!($);
    }
}

macro_rules! make_error {
    ($NAME:ident, $ERR:ident) => {
        with_dollar_sign! {
            ($d:tt) => {
                #[macro_export]
                macro_rules! $NAME {
                    ($d($d args:expr),*) => {           
                        Err(datafusion_common::DataFusionError::$ERR(format!($d($d args),*).into()))
                    }
                }
            }
        }
    };
}

make_error!(plan_err, Plan);
            if !val.is_null() {
                plan_err!("value is not null {}", val)?
            }

@comphead
Copy link
Contributor

@alamb let me know what do you think.

@comphead
Copy link
Contributor

@alamb I can take all the findings and create a PR replacing all plan errors for now, if you think it is a right direction to go

@alamb
Copy link
Contributor Author

alamb commented Jun 24, 2023

Sorry for the late reply @comphead

@alamb let me know what do you think.

I think it looks great to me.

@alamb I can take all the findings and create a PR replacing all plan errors for now, if you think it is a right direction to go

I think that would be good. Perhaps we you could make the macros and do a few changes and we can make sure it looks reasonable before spending the time to migrate the whole codebase?

Thanks again @comphead

@tustvold tustvold changed the base branch from main to backup-main June 27, 2023 11:13
@tustvold tustvold changed the base branch from backup-main to main June 27, 2023 11:13
@alamb
Copy link
Contributor Author

alamb commented Jul 25, 2023

I have no plans to pursue this PR at this time, so closing it

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.

2 participants