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

Gen4: groupby support #8453

Merged
merged 22 commits into from
Jul 15, 2021
Merged

Gen4: groupby support #8453

merged 22 commits into from
Jul 15, 2021

Conversation

harshit-gangal
Copy link
Member

@harshit-gangal harshit-gangal commented Jul 12, 2021

Description

This PR adds support for grouping and aggregations.
Gen4 still needs to handle grouping with distinct.

Related Issue(s)

#7280

Checklist

  • Tests were added or are not required
  • Documentation was added or is not required

Deployment Notes

harshit-gangal and others added 9 commits July 9, 2021 17:05
Signed-off-by: Harshit Gangal <harshit@planetscale.com>
Signed-off-by: Andres Taylor <andres@planetscale.com>
Signed-off-by: Andres Taylor <andres@planetscale.com>
Signed-off-by: Andres Taylor <andres@planetscale.com>
Signed-off-by: Harshit Gangal <harshit@planetscale.com>
Signed-off-by: Harshit Gangal <harshit@planetscale.com>
Signed-off-by: Harshit Gangal <harshit@planetscale.com>
…if required after horizon planning at top most plan. all select columns to be available irrepective of if they are same expression (for correctness)

Signed-off-by: Harshit Gangal <harshit@planetscale.com>
Signed-off-by: Harshit Gangal <harshit@planetscale.com>
@harshit-gangal harshit-gangal added Component: Query Serving Type: Enhancement Logical improvement (somewhere between a bug and feature) labels Jul 13, 2021
…eycol in group by params when weight_string is required

Signed-off-by: Harshit Gangal <harshit@planetscale.com>
Signed-off-by: Harshit Gangal <harshit@planetscale.com>
@systay systay force-pushed the gen4-groupby branch 3 times, most recently from e5fd7fb to 4b3a655 Compare July 13, 2021 16:05
Signed-off-by: Andres Taylor <andres@planetscale.com>
frouioui and others added 5 commits July 14, 2021 08:28
Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
Signed-off-by: Andres Taylor <andres@planetscale.com>
…group on aggregate functions

Signed-off-by: Andres Taylor <andres@planetscale.com>
Signed-off-by: Andres Taylor <andres@planetscale.com>
@harshit-gangal harshit-gangal marked this pull request as ready for review July 14, 2021 17:49
Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
Copy link
Member

@GuptaManan100 GuptaManan100 left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: Andres Taylor <andres@planetscale.com>
@systay systay merged commit 1312ec8 into vitessio:main Jul 15, 2021
@systay systay deleted the gen4-groupby branch July 15, 2021 07:21
Comment on lines +1369 to +1387

// ContainsAggregation returns true if the expression contains aggregation
func ContainsAggregation(e Expr) bool {
hasAggregates := false
_ = Walk(func(node SQLNode) (kontinue bool, err error) {
switch node := node.(type) {
case *FuncExpr:
if node.IsAggregate() {
hasAggregates = true
return false, nil
}
case *GroupConcatExpr:
hasAggregates = true
return false, nil
}
return true, nil
}, e)
return hasAggregates
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Similar method exists in ordered_aggregates

func nodeHasAggregates(node sqlparser.SQLNode) bool {
	hasAggregates := false
	_ = sqlparser.Walk(func(node sqlparser.SQLNode) (kontinue bool, err error) {
		switch node := node.(type) {
		case *sqlparser.FuncExpr:
			if node.IsAggregate() {
				hasAggregates = true
				return false, errors.New("unused error")
			}
		case *sqlparser.GroupConcatExpr:
			hasAggregates = true
			return false, errors.New("unused error")
		case *sqlparser.Subquery:
			// Subqueries are analyzed by themselves.
			return false, nil
		}
		return true, nil
	}, node)
	return hasAggregates
}

Comment on lines +141 to +146
# join order by with ambiguous column reference ; valid in MySQL
"select name, name from user, music order by name"
"ambiguous symbol reference: `name`"
{
"QueryType": "SELECT",
"Original": "select name, name from user, music order by name",
Copy link
Member Author

Choose a reason for hiding this comment

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

this can be moved out from unsupported_cases

Comment on lines 130 to 137
"Name": "user",
"Sharded": true
},
"FieldQuery": "select id, weight_string(id) from `user` where 1 != 1",
"OrderBy": "0 ASC",
"Query": "select id, weight_string(id) from `user` order by id asc",
"ResultColumns": 1,
"FieldQuery": "select id, id, weight_string(id) from `user` where 1 != 1",
"OrderBy": "(0|2) ASC",
"Query": "select id, id, weight_string(id) from `user` order by id asc",
"ResultColumns": 2,
"Table": "`user`"
Copy link
Member Author

Choose a reason for hiding this comment

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

this can be moved out from unsupported_cases

@@ -1705,10 +1705,10 @@ Gen4 plan same as above
}

# select from unsharded keyspace into outfile s3
"select * from main.unsharded into outfile s3 'out_file_name' character set binary format csv header fields terminated by 'term' optionally enclosed by 'c' escaped by 'e' lines starting by 'a' terminated by '\\n' manifest on overwrite off"
Copy link
Member Author

Choose a reason for hiding this comment

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

This looks like an unintentional change.

@@ -1687,10 +1687,10 @@ Gen4 plan same as above
}

# select from unsharded keyspace into outfile
"select * from main.unsharded into outfile 'x.txt' character set binary fields terminated by 'term' optionally enclosed by 'c' escaped by 'e' lines starting by 'a' terminated by '\\n'"
Copy link
Member Author

Choose a reason for hiding this comment

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

This looks like an unintentional change.

Comment on lines +40 to +43
HasAggr bool
GroupByExprs []GroupBy
OrderExprs []OrderBy
CanPushDownSorting bool
Copy link
Member Author

Choose a reason for hiding this comment

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

hasAggr and CanPushDownSorting should be added in toString method and a test can be added as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Query Serving Type: Enhancement Logical improvement (somewhere between a bug and feature)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants