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.

Sign up for GitHub

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

Jump to bottom

[GLUTEN-7581][CORE] Code cleanup for GlutenColumnarRule #7582

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+21 −38
Open

[GLUTEN-7581][CORE] Code cleanup for GlutenColumnarRule #7582

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 21 additions & 38 deletions gluten-core/src/main/scala/org/apache/gluten/extension/GlutenColumnarRule.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,38 +58,6 @@ object GlutenColumnarRule {
override protected def withNewChildInternal(newChild: SparkPlan): SparkPlan =
copy(child = newChild)
}

object OutputsColumnarTester {
def wrap(plan: SparkPlan): SparkPlan = {
if (plan.supportsColumnar) {
DummyColumnarOutputExec(plan)
} else {
DummyRowOutputExec(plan)
}
}

def inferOutputsColumnar(plan: SparkPlan): Boolean = plan match {
case DummyRowOutputExec(_) => false
case RowToColumnarExec(DummyRowOutputExec(_)) => true
case DummyColumnarOutputExec(_) => true
case ColumnarToRowExec(DummyColumnarOutputExec(_)) => false
case _ =>
throw new IllegalStateException(
"This should not happen. Please leave a issue at" +
" https://github.com/apache/incubator-gluten.")
}

def unwrap(plan: SparkPlan): SparkPlan = plan match {
case DummyRowOutputExec(child) => child
case RowToColumnarExec(DummyRowOutputExec(child)) => child
case DummyColumnarOutputExec(child) => child
case ColumnarToRowExec(DummyColumnarOutputExec(child)) => child
case _ =>
throw new IllegalStateException(
"This should not happen. Please leave a issue at" +
" https://github.com/apache/incubator-gluten.")
}
}
}

case class GlutenColumnarRule(
Expand All @@ -109,16 +77,31 @@ case class GlutenColumnarRule(
*/
final override def preColumnarTransitions: Rule[SparkPlan] = plan => {
// To infer caller's property: ApplyColumnarRulesAndInsertTransitions#outputsColumnar.
OutputsColumnarTester.wrap(plan)
if (plan.supportsColumnar) {
DummyColumnarOutputExec(plan)
} else {
DummyRowOutputExec(plan)
}
}

override def postColumnarTransitions: Rule[SparkPlan] = plan => {
val outputsColumnar = OutputsColumnarTester.inferOutputsColumnar(plan)
val unwrapped = OutputsColumnarTester.unwrap(plan)
val vanillaPlan = Transitions.insertTransitions(unwrapped, outputsColumnar)
val (originalPlan, outputsColumnar) = plan match {
case DummyRowOutputExec(child) =>
(child, false)
case RowToColumnarExec(DummyRowOutputExec(child)) =>
(child, true)
case DummyColumnarOutputExec(child) =>
(child, true)
case ColumnarToRowExec(DummyColumnarOutputExec(child)) =>
(child, false)
case _ =>
throw new IllegalStateException(
"This should not happen. Please leave an issue at" +
" https://github.com/apache/incubator-gluten.")
}
val vanillaPlan = Transitions.insertTransitions(originalPlan, outputsColumnar)
val applier = applierBuilder.apply(session)
val out = applier.apply(vanillaPlan, outputsColumnar)
out
applier.apply(vanillaPlan, outputsColumnar)
Copy link
Member

Choose a reason for hiding this comment

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

val out = ...
out

This code pattern is used usually because out allows developer to place a breakpoint for debugging against the output object easily. Do you happen to know an alternative way if we simplify the code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I understood it. We usually avoid this code pattern in Spark. Developers could evaluate the expression (e.g. out) if who want know the value.
Of course, I will respect the code pattern if Gluten has the preference.

Copy link
Member

Choose a reason for hiding this comment

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

Let's do the simplification. Just found there is an alternative way while using Intellij IDE. Thanks.

}

}
Expand Down
Loading