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

Refactor aggregation function structs #10113

Closed
3 tasks
systay opened this issue Apr 20, 2022 · 5 comments
Closed
3 tasks

Refactor aggregation function structs #10113

systay opened this issue Apr 20, 2022 · 5 comments
Assignees

Comments

@systay
Copy link
Collaborator

systay commented Apr 20, 2022

Our parser outputs all aggregations functions using the general sqlparser.FuncExpr. This is a little clunky and requires us to do additional checks before we are sure that the aggregation we have is valid and what it is aggregating.

I suggest we instead introduce one struct type per aggregation, and have them all implement some interface. This would help clean up the code.

Example:

type (
	AggrFunc interface {
		iAggrFunc()
	}

	CountStar struct {}

	Count struct {
		Arg Expr
	}
        // and the rest
)

Tasks:

  • Add AST structs
  • Update the parser to use these structs
  • Find and replace uses of the old representation and use the new structs instead
@skant7
Copy link

skant7 commented May 7, 2022

Hi @systay is there any chance to include this issue as one of the projects for summer 2022 lfx mentorship ?

@rsajwani rsajwani self-assigned this May 9, 2022
@AllMight2099
Copy link

Hey @rsajwani, I'd like to work on this issue if that's alright with you

@GuptaManan100
Copy link
Member

GuptaManan100 commented Aug 10, 2022

I think @rsajwani already started working on it and the associated PR is #10347. @rsajwani can update on the progress, and whether we can close this issue or if some part of the work is left.

@AllMight2099
Copy link

Oh okay. @GuptaManan100 Are there any good first issues that you'd recommend?

@GuptaManan100
Copy link
Member

@AllMight2099 We have a Good First Issue label in Vitess. You can try filtering out issues with that, or you could pick up something for the list ☝️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants