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.

Sign up for GitHub

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

Jump to bottom

sql: add support for sorted GROUP BY results #3542

Closed
dt opened this issue Dec 28, 2015 · 2 comments
Closed

sql: add support for sorted GROUP BY results #3542

dt opened this issue Dec 28, 2015 · 2 comments
Assignees
@dt
Labels
C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)

Comments

@dt
Copy link
Member

dt commented Dec 28, 2015

Currently, even once #3494 lands with initial GROUP BYsupport, we still error with unimplemented ORDER BY with GROUP BY/aggregation if both a GROUP BY and ORDER BY are used.

As noted in the code, adding support will require some changes to how we do sorting (as it currently hooks into the scan node).
@dt dt added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) SQL labels Dec 28, 2015
@dt dt self-assigned this Dec 28, 2015
@dt
Copy link
Member Author

dt commented Dec 28, 2015

@petermattis At first glance, I appear to have two options for making sortNode work with groupNode:

  1. make groupNode support calling addRender and similar, such that it becomes interchangeable with a plain scanNode for the purposes of a sortNode (pulling the common methods into an interface).
  2. let the existing sortNode wrap the scanNode before the groupNode does, so that when it iterates over the render expressions to extract aggregates, it also includes any that the sortNode added during wrapping.

At present I think I'm leaning towards 2), after toying with 1), but wanted to solicit feedback before I got too invested in either approach.

@petermattis
Copy link
Collaborator

I was expecting we would have to do something like option 1. Option 2 seems a bit messier, though perhaps easier to get working. I think it would be nice if neither sortNode or groupNode took a scanNode but instead there was some sort of renderNode interface with methods for looping over the render targets and adding new ones.

@dt dt mentioned this issue Dec 30, 2015
Merged
dt added a commit to dt/cockroach that referenced this issue Dec 30, 2015
@dt
support using ORDER BY with GROUP BY
37971f0 
Fixes cockroachdb#3542.

Unlike either approach (for nesting order of sortNode and groupNode) mentioned there, this PR just passes both sortBy and orderBy the original scanNode, letting them inspect and modify it directly, rather than having one delegate to the to the other and extracting some common interface for inspecting and modifying the render expressions.

Note that sortBy, despite wrapping after groupBy, is given the first chance at the scanNode: this is important for two reasons: first, so that any aggregate expressions it adds will be properly transformed along with the rest of render by groupBy and second so that when looking for existing expressions that match, it does so before the their transformation into aggregatefuncs.
dt added a commit to dt/cockroach that referenced this issue Dec 30, 2015
@dt
support using ORDER BY with GROUP BY
88e659a 
Fixes cockroachdb#3542.

Unlike either approach (for nesting order of sortNode and groupNode) mentioned there, this PR just passes both sortBy and orderBy the original scanNode, letting them inspect and modify it directly, rather than having one delegate to the to the other and extracting some common interface for inspecting and modifying the render expressions.

Note that sortBy, despite wrapping after groupBy, is given the first chance at the scanNode: this is important for two reasons: first, so that any aggregate expressions it adds will be properly transformed along with the rest of render by groupBy and second so that when looking for existing expressions that match, it does so before the their transformation into aggregatefuncs.
@dt dt closed this as completed in #3564 Dec 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dt dt

Labels
C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@dt @petermattis