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

Make Expr enum consistent with LogicalPlan by using structs for each variant #3807

Closed
17 tasks done
andygrove opened this issue Oct 12, 2022 · 10 comments · Fixed by #6304
Closed
17 tasks done

Make Expr enum consistent with LogicalPlan by using structs for each variant #3807

andygrove opened this issue Oct 12, 2022 · 10 comments · Fixed by #6304
Labels
enhancement New feature or request

Comments

@andygrove
Copy link
Member

andygrove commented Oct 12, 2022

Is your feature request related to a problem or challenge? Please describe what you are trying to do.
See the discussion at #2175 for background context, but the basic issue is that there are different styles between LogicalPlan and Expr.

LogicalPlan

Each variant in LogicalPlan wraps a struct. The structs typically have try_new methods with some validation logic.

pub enum LogicalPlan {
    Projection(Projection),
    Filter(Filter),
    ..
}

Expr

Expr has a mixed style. Some variants wrap structs and some do not.

pub enum Expr {
    Alias(Box<Expr>, String),
    Column(Column),
    Between {
        /// The value to compare
        expr: Box<Expr>,
        /// Whether the expression is negated
        negated: bool,
        /// The low end of the range
        low: Box<Expr>,
        /// The high end of the range
        high: Box<Expr>,
    },
    Like {
        negated: bool,
        expr: Box<Expr>,
        pattern: Box<Expr>,
        escape_char: Option<char>,
    },

Describe the solution you'd like
We would like each variant in Expr to wrap a struct (except perhaps for the very simple ones that contain one or two items) and add new and/or try_new methods to those structs as appropriate.

Describe alternatives you've considered
Do nothing

Additional context
None

@jackwener
Copy link
Member

jackwener commented May 8, 2023

I prepare to promote this task. I invite some contributor.
if anyone are interested in this issue. Please reply the part which you want to do.

@SkyFan2002
Copy link

I prepare to finish this task. I invite some contributor, if you are interested in this issue. Please reply the part which you want to do.

Hi, I am new to this project. Can I help with InList?

@jackwener
Copy link
Member

jackwener commented May 8, 2023

Hi, I am new to this project. Can I help with InList?

It's OK, Thank you @SkyFan2002

@QuenKar
Copy link
Contributor

QuenKar commented May 8, 2023

Hello,I‘m a noob to this project and interested in Exists part.May I have a try?

@jackwener
Copy link
Member

Hello,I‘m a noob to this project and interested in Exists part.May I have a try?

It's OK, Thank you @QuenKar

@gitccl
Copy link
Contributor

gitccl commented May 8, 2023

Hi, I'd like to try InSubquery.

@jackwener
Copy link
Member

Hi, I'd like to try InSubquery.

@gitccl welcome, thanks

@my-vegetable-has-exploded
Copy link
Contributor

I'd like to try AggregateUDF. Thanks.

@jackwener
Copy link
Member

Already finish it, thanks everyone!❤️

@alamb
Copy link
Contributor

alamb commented May 10, 2023

Pretty amazing @jackwener and everyone else who helped out here -- very nice

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants