-
Notifications
You must be signed in to change notification settings - Fork 2.4k
[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
Conversation
…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.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
"{$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'}}" | ||
)); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ?
…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
…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
…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
…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
…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.