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

Change limit to expression. #2878

Closed
wants to merge 8 commits into from

Conversation

Shylock-Hg
Copy link
Contributor

@Shylock-Hg Shylock-Hg commented Sep 16, 2021

For the limit push down in Loop

Subjob of #2602

@Shylock-Hg Shylock-Hg added the ready-for-testing PR: ready for the CI test label Sep 16, 2021

Limit(QueryContext* qctx, PlanNode* input, int64_t offset, Expression* count)
: SingleInputNode(qctx, Kind::kLimit, input) {
offset_ = offset;
count_ = count;
}

void cloneMembers(const Limit&);

private:
int64_t offset_{-1};
Copy link
Contributor

@czpmango czpmango Sep 16, 2021

Choose a reason for hiding this comment

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

Need change offset_ into an expression?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

offset won't push down

Copy link
Contributor

Choose a reason for hiding this comment

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

It is better to ensure consistency in use.

go from ....| offset abs(1)+3 limit abs(3)+1
go from ....| limit abs(3)+1,abs(3)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is used for new syntax for go ... limit [..]

Comment on lines +696 to +703
int64_t count() const {
if (count_ == nullptr) {
return -1;
}
DCHECK(ExpressionUtils::isEvaluableExpr(count_));
QueryExpressionContext ctx;
return count_->eval(ctx).getInt();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this interface necessary?

Copy link
Contributor Author

@Shylock-Hg Shylock-Hg Sep 16, 2021

Choose a reason for hiding this comment

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

Compatible.

@Shylock-Hg
Copy link
Contributor Author

Subjob of #2602

@Shylock-Hg Shylock-Hg requested a review from czpmango September 16, 2021 06:49
Copy link
Contributor

@yixinglu yixinglu left a comment

Choose a reason for hiding this comment

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

I don't know what this PR is for? could you give some description?

@@ -50,6 +50,10 @@ StatusOr<OptRule::TransformResult> LimitPushDownRule::transform(
const auto proj = static_cast<const Project *>(projGroupNode->node());
const auto gn = static_cast<const GetNeighbors *>(gnGroupNode->node());

DCHECK(graph::ExpressionUtils::isEvaluableExpr(limit->countExpr()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't use DCHECK to debug your code if it's the regular branch you need to handle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This won't happen in correct state.

Copy link
Contributor

Choose a reason for hiding this comment

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

So why did you do the same check in line 54?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Handle exception scenes

@Shylock-Hg
Copy link
Contributor Author

I don't know what this PR is for? could you give some description?

Push down limit to GN in Loop for each step.

@Shylock-Hg Shylock-Hg requested a review from yixinglu September 17, 2021 01:58
nevermore3
nevermore3 previously approved these changes Sep 17, 2021
@Sophie-Xie Sophie-Xie added this to the v2.6.0 milestone Sep 22, 2021
@Sophie-Xie Sophie-Xie removed the request for review from CPWstatic September 23, 2021 06:44
@Sophie-Xie
Copy link
Contributor

Include in #2853

@Sophie-Xie Sophie-Xie closed this Sep 28, 2021
@Shylock-Hg Shylock-Hg deleted the feature/limit-expr branch February 9, 2022 07:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-testing PR: ready for the CI test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants