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

Add OFFSET implementation #732

Merged
merged 2 commits into from May 10, 2019

Conversation

2 participants
@kasiafi
Copy link
Member

commented May 9, 2019

Relates to #1

@cla-bot cla-bot bot added the cla-signed label May 9, 2019

@kasiafi kasiafi changed the title Offset implementation Add OFFSET implementation May 9, 2019

@kasiafi kasiafi force-pushed the kasiafi:OffsetImplementation branch from 0fee17d to 6f8e3d8 May 9, 2019

@martint
Copy link
Member

left a comment

Looks great! Just a few minor comments.

public class MergeLimitOverProjectWithSort
implements Rule<LimitNode>
{
private static final Capture<ProjectNode> PROJECT_CHILD = newCapture();

This comment has been minimized.

Copy link
@martint

martint May 9, 2019

Member

Just call these PROJECT and SORT? I don't think the _CHILD part adds any information. In fact, it's slightly confusing as it leads the reader to think it might refer to the "child of the project node".

throw new SemanticException(NOT_SUPPORTED, node, "OFFSET not yet implemented");
long rowCount;
try {
rowCount = Long.parseLong(node.getRowCount());

This comment has been minimized.

Copy link
@martint

martint May 9, 2019

Member

This should check that offset >= 0. Even though the parser guarantees the value will be >= 0, the API to the analyzer is an AST, which could be constructed by hand with an invalid offset.

public class ImplementOffsetOverProjectSort
implements Rule<OffsetNode>
{
private static final Capture<ProjectNode> PROJECT_CHILD = newCapture();

This comment has been minimized.

Copy link
@martint

martint May 9, 2019

Member

Same here, just name it PROJECT and SORT

@Override
public Result apply(LimitNode parent, Captures captures, Context context)
{
ProjectNode projectChild = captures.get(PROJECT_CHILD);

This comment has been minimized.

Copy link
@martint

martint May 9, 2019

Member

Rename these to project and sort

@Override
public Result apply(OffsetNode parent, Captures captures, Context context)
{
ProjectNode projectChild = captures.get(PROJECT_CHILD);

This comment has been minimized.

Copy link
@martint

martint May 9, 2019

Member

Rename these to project and sort. The same applies to all the other rules.

strictProject(
ImmutableMap.of("name", new ExpressionMatcher("name")),
filter(
"(\"row_num\" > BIGINT '2')",

This comment has been minimized.

Copy link
@martint

martint May 9, 2019

Member

Why the escaped \"?

This comment has been minimized.

Copy link
@kasiafi

kasiafi May 10, 2019

Author Member

not needed, deleted

{
String values = "(VALUES ('A', 3), ('D', 2), ('C', 1), ('B', 4)) AS t(x, y)";

assertQuery("SELECT x FROM " + values + " OFFSET 2 ROWS", "VALUES 'C', 'B'");

This comment has been minimized.

Copy link
@martint

martint May 9, 2019

Member

This is not guaranteed to produce 'C', 'B' since there's no ordering. Verify this query by computing the results and ensuring it contains:

  • 2 rows
  • they are different from each other
  • they are contained in the original set of values

See testLimit in this class as an example.

assertQuery("SELECT x FROM " + values + " OFFSET 2 ROWS", "VALUES 'C', 'B'");
assertQuery("SELECT x FROM " + values + " ORDER BY y OFFSET 2 ROWS", "VALUES 'A', 'B'");
assertQuery("SELECT x FROM " + values + " ORDER BY y OFFSET 2 ROWS FETCH NEXT 1 ROW ONLY", "VALUES 'A'");
assertQuery("SELECT x FROM " + values + " OFFSET 2 ROWS FETCH NEXT 1 ROW ONLY", "VALUES 'C'");

This comment has been minimized.

Copy link
@martint

martint May 9, 2019

Member

Same here

kasiafi added some commits May 7, 2019

Add MergeLimitOverProjectWithSort rule
This rule merges LimitNode with SortNode into TopNNode
in the case when there is identity ProjectNode
between LimitNode and SortNode.

@kasiafi kasiafi force-pushed the kasiafi:OffsetImplementation branch from 6f8e3d8 to a469575 May 10, 2019

@kasiafi

This comment has been minimized.

Copy link
Member Author

commented May 10, 2019

Applied comments.

@martint martint merged commit 4d6325b into prestosql:master May 10, 2019

2 checks passed

Travis CI - Pull Request Build Passed
Details
verification/cla-signed
Details
@martint

This comment has been minimized.

Copy link
Member

commented May 10, 2019

Merged, thanks!

@martint martint referenced this pull request May 10, 2019

Closed

Release notes for 311 #716

5 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.