[GLUTEN-11635][FOLLOW-UP] Respect partial fallback configs when checking node support#11988
Conversation
philo-he
left a comment
philo-he
left a comment
There was a problem hiding this comment.
One comment. Please check if it makes sense. Thank you.
| plan.isInstanceOf[ProjectExec] || | ||
| plan.isInstanceOf[GenerateExec] | ||
| def supportPartialFallback(plan: SparkPlan): Boolean = plan match { | ||
| case _: ProjectExec => GlutenConfig.get.enableColumnarPartialProject |
There was a problem hiding this comment.
I noticed the following code already performs this check before PartialFallback.supportPartialFallback is called. Is this a duplicate?
There was a problem hiding this comment.
I think this is to handle situations where the parent and child are different; one is ProjectExec, and the other is GenerateExec.
There was a problem hiding this comment.
I meant the check for config. Am I missing something?
There was a problem hiding this comment.
The rule-level check only controls whether that specific rule runs. supportPartialFallback is used when checking whether a parent SparkPlan can be used as a partial-fallback boundary by another rule.
In nested cases, PartialGenerateRule may inspect a ProjectExec parent, and PartialProjectRule may inspect a GenerateExec parent. Without checking the corresponding config in supportPartialFallback, a disabled partial fallback type can still be considered a supported boundary indirectly.
There was a problem hiding this comment.
I see. It's cross referenced. Thanks.
| plan.isInstanceOf[GenerateExec] | ||
| def supportPartialFallback(plan: SparkPlan): Boolean = plan match { | ||
| case _: ProjectExec => GlutenConfig.get.enableColumnarPartialProject | ||
| case _: GenerateExec => GlutenConfig.get.enableColumnarPartialGenerate |
| executedPlan.exists(_.isInstanceOf[ColumnarPartialProjectExec]) == | ||
| enablePartialProject) | ||
| assert( | ||
| executedPlan.exists(_.isInstanceOf[ColumnarPartialGenerateExec]) == |
There was a problem hiding this comment.
Can we also test the case that ENABLE_COLUMNAR_PARTIAL_GENERATE is disabled?
3 participants
What changes are proposed in this pull request?
Minor follow up for #11637.
How was this patch tested?
Add UT.
Was this patch authored or co-authored using generative AI tooling?
No.
Related issue: #11635