Skip to content

[CALCITE-3330] propagateCostImprovements() could result in stack over… #1445

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

Closed
wants to merge 1 commit into from

Conversation

xndai
Copy link
Contributor

@xndai xndai commented Sep 9, 2019

…flow

Current implementation uses depth first approach for propagating cost improvements to parent rel nodes. This could lead to stack overflow if the rel node hierarchy is very deep. Fix this by using breath first approach for cost propagation.

…flow

Current implementation uses depth first approach for propagating cost improvements to parent rel nodes. This could lead to stack overflow if the rel node hierarchy is very deep. Fix this by using breath first approach for cost propagation.
@xndai
Copy link
Contributor Author

xndai commented Sep 9, 2019

I have a UT failure at MongoAdapterTest.testGroupByAvgSumCount() due to the operator sequence change. My understand is this does not actually affect the plan. But would appreciate if someone is more familiar with the background can review it. Thx.

Copy link
Member

@hsyuan hsyuan left a comment

Choose a reason for hiding this comment

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

+1

@hsyuan hsyuan closed this in e435954 Oct 9, 2019
"{$sort: {STATE: 1}}"));
"{$sort: {STATE: 1}}",
"{$project: {STATE: 1, A: {$divide: [{$cond:[{$eq: ['$_2', {$literal: 0}]},null,'$_1']}, '$_2']}, S: {$cond:[{$eq: ['$_2', {$literal: 0}]},null,'$_1']}, C: '$_2'}}"
));
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little confused why the plan has changed, because this patch seems have equivalent logic function as before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's possible because the sequence of cost propagation is changed now thus the memo can be different. But anyway the plan is equivalent.

Copy link
Contributor

@danny0405 danny0405 Oct 10, 2019

Choose a reason for hiding this comment

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

You mean the planner choose a same cost plan with before but with different structure ?

XuQianJin-Stars pushed a commit to XuQianJin-Stars/calcite that referenced this pull request Nov 17, 2019
…ements

Current implementation uses depth first approach for propagating cost
improvements to parent rel nodes. This could lead to stack overflow if the rel
node hierarchy is very deep. Fix this by using breath first approach for cost
propagation.

Close apache#1445
XuQianJin-Stars pushed a commit to XuQianJin-Stars/calcite that referenced this pull request Nov 20, 2019
…ements

Current implementation uses depth first approach for propagating cost
improvements to parent rel nodes. This could lead to stack overflow if the rel
node hierarchy is very deep. Fix this by using breath first approach for cost
propagation.

Close apache#1445
XuQianJin-Stars pushed a commit to XuQianJin-Stars/calcite that referenced this pull request Nov 20, 2019
…ements

Current implementation uses depth first approach for propagating cost
improvements to parent rel nodes. This could lead to stack overflow if the rel
node hierarchy is very deep. Fix this by using breath first approach for cost
propagation.

Close apache#1445
wangxlong pushed a commit to wangxlong/calcite that referenced this pull request Feb 13, 2020
…ements

Current implementation uses depth first approach for propagating cost
improvements to parent rel nodes. This could lead to stack overflow if the rel
node hierarchy is very deep. Fix this by using breath first approach for cost
propagation.

Close apache#1445
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.

3 participants