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

Window function frames, extent, and bounds #104

Merged
merged 6 commits into from
Dec 16, 2021
Merged

Conversation

max-hoffman
Copy link

@max-hoffman max-hoffman commented Dec 14, 2021

Adds parsing capabilities for window frames, including one sided frames, two sided frames, RANGE value frames, integral and interval based frames, and errors for semantically incorect combinations of frame options. Only value based intervals are currently supported. No prepared statement support for expression bounds.

@max-hoffman max-hoffman changed the title Max/basic window frames Window function frames, extent, and bounds Dec 15, 2021
Adds parsing capabilities for window frames, including
one sided frames, two sided frames, RANGE value frames,
integral and interval based frames, and errors for
semantically incorect combinations of frame options.
Only value based intervals are currently supported.
Prepared bound expressions are not supported.
@max-hoffman max-hoffman marked this pull request as ready for review December 15, 2021 18:54
}

// Format formats the node.
func (node *FrameBound) print(buf *TrackedBuffer) {
Copy link
Author

Choose a reason for hiding this comment

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

should FrameBound implement Node? the Format function would be empty, but it's useful in the AST

Copy link
Member

Choose a reason for hiding this comment

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

Why would the format function be empty? It has a textual representation, right?

Copy link
Author

Choose a reason for hiding this comment

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

was confused, realize now that node printing is recursive

Copy link
Member

@zachmu zachmu left a comment

Choose a reason for hiding this comment

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

You were tricked by a trap in the parse testing framework, see comments

}

// Format formats the node.
func (node *Frame) Format(buf *TrackedBuffer) {
Copy link
Member

Choose a reason for hiding this comment

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

Not printing Extent here is a red flag

Copy link
Member

Choose a reason for hiding this comment

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

I don't actually know how these tests pass

Copy link
Member

Choose a reason for hiding this comment

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

The reason they pass is because you have been fooled: in these tests, select expressions echo their textual input unless you tell the test case serializeSelectExprs: true

When you add that to one of these cases, you can see they don't actually work, no range info is printed:

expected: "select name, dense_rank() over (partition by x order by y RANGE BETWEEN 1 PRECEDING AND 1 FOLLOWING) from t"
actual : "select name, dense_rank() over (partition by x order by y asc) from t"

From a brief look, it seems most of the OVER tests have this issue

Copy link
Author

Choose a reason for hiding this comment

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

the opposite is now default, need to specify useSelectExpressionLiteral to sidestep parsing

}

// Format formats the node.
func (node *FrameBound) print(buf *TrackedBuffer) {
Copy link
Member

Choose a reason for hiding this comment

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

Why would the format function be empty? It has a textual representation, right?

Copy link
Member

@zachmu zachmu left a comment

Choose a reason for hiding this comment

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

LGTM!

@max-hoffman max-hoffman merged commit daa0dcc into main Dec 16, 2021
@max-hoffman max-hoffman deleted the max/basic-window-frames branch December 16, 2021 21:59
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