Copilot commented on code in PR #13284:
URL: https://github.com/apache/gluten/pull/13284#discussion_r4271805153
##########
gluten-substrait/src/test/scala/org/apache/gluten/execution/PushAggregateThroughJoinSuite.scala:
##########
@@ -93,89 +94,99 @@ class PushAggregateThroughJoinSuite extends PlanTest with
SharedSparkSession {
testCase: PushdownCase,
maxDepth: Int,
expectedPushCount: Int,
- expectedAggCount: Int,
- partialMergeEnabled: Boolean): Unit = {
- withSQLConf(
- GlutenConfig.PUSH_AGGREGATE_THROUGH_JOIN_ENABLED.key -> "true",
- GlutenConfig.PUSH_AGGREGATE_THROUGH_JOIN_MAX_DEPTH.key ->
maxDepth.toString,
- GlutenConfig.PUSH_AGGREGATE_THROUGH_JOIN_PARTIAL_MERGE_ENABLED.key ->
- partialMergeEnabled.toString
- ) {
- val (withoutRuleRows, withoutRuleLogicalPlan, withoutRulePhysicalPlan) =
- withExtraPlanning(Nil, Nil) {
- val df = spark.sql(testCase.inputSql)
- (
- df.collect().toSeq.sortBy(_.toString()),
- df.queryExecution.optimizedPlan,
- finalExecutedPlan(df.queryExecution.executedPlan)
- )
- }
-
- val (withRuleRows, withRuleLogicalPlan, withRulePhysicalPlan) =
- withExtraPlanning(Seq(joinAggregateRule), Nil) {
- joinAggregateRule.resetSuccessfulPushCount()
- val df = spark.sql(testCase.inputSql)
- val withRuleRows = df.collect().toSeq.sortBy(_.toString())
- val withRulePlan = df.queryExecution.optimizedPlan
- val withRulePhysicalPlan =
finalExecutedPlan(df.queryExecution.executedPlan)
- val aggregateNodeCount = withRulePlan.collect { case _: Aggregate =>
1 }.size
- val nodesWithMissingInput = withRulePlan.collect {
- case p if p.missingInput.nonEmpty => p
+ expectedAggCounts: ExpectedAggCounts): Unit = {
+ val modes = Seq(
+ "none" -> expectedAggCounts.none,
+ "top" -> expectedAggCounts.top,
+ "all" -> expectedAggCounts.all)
+ modes.foreach {
+ case (partialMergeMode, expectedAggCount) =>
+ withClue(s"partialMergeMode=$partialMergeMode, maxDepth=$maxDepth: ") {
+ withSQLConf(
+ GlutenConfig.PUSH_AGGREGATE_THROUGH_JOIN_ENABLED.key -> "true",
+ GlutenConfig.PUSH_AGGREGATE_THROUGH_JOIN_MAX_DEPTH.key ->
maxDepth.toString,
+ GlutenConfig.PUSH_AGGREGATE_THROUGH_JOIN_PARTIAL_MERGE_MODE.key ->
+ partialMergeMode
+ ) {
+ val (withoutRuleRows, withoutRuleLogicalPlan,
withoutRulePhysicalPlan) =
+ withExtraPlanning(Nil, Nil) {
+ val df = spark.sql(testCase.inputSql)
+ (
+ df.collect().toSeq.sortBy(_.toString()),
+ df.queryExecution.optimizedPlan,
+ finalExecutedPlan(df.queryExecution.executedPlan)
+ )
+ }
Review Comment:
The baseline computation (`withoutRuleRows` / `withoutRuleLogicalPlan` /
`withoutRulePhysicalPlan`) is repeated for every `partialMergeMode`, even
though it’s independent of the mode (it runs with no rule/strategy). This
increases runtime noticeably (each test case now does 3× the baseline work).
Consider hoisting the baseline computation outside `modes.foreach` (run it once
per `testCase`/`maxDepth`) and only varying the “with rule” and “with rule +
strategy” parts inside the loop.
##########
backends-velox/src/test/scala/org/apache/gluten/execution/StarSchemaJoinAggregateSuite.scala:
##########
@@ -73,7 +73,14 @@ class StarSchemaJoinAggregateSingleDepthSuite extends
StarSchemaJoinAggregateSui
class StarSchemaJoinAggregatePartialMergeEnabledSuite extends
StarSchemaJoinAggregateSuite {
override protected def sparkConf: SparkConf = {
super.sparkConf
- .set(GlutenConfig.PUSH_AGGREGATE_THROUGH_JOIN_PARTIAL_MERGE_ENABLED.key,
"true")
+ .set(GlutenConfig.PUSH_AGGREGATE_THROUGH_JOIN_PARTIAL_MERGE_MODE.key,
"all")
+ }
+}
Review Comment:
The suite name still references the removed boolean “enabled” config, but it
now sets the enum mode to `"all"`. Renaming the suite to reflect the actual
mode (e.g., `StarSchemaJoinAggregateAllPartialMergeSuite`) would make it
clearer and avoid confusion now that `TOP` also exists.
##########
gluten-core/src/main/scala/org/apache/gluten/config/ConfigBuilder.scala:
##########
@@ -122,6 +127,16 @@ private[gluten] case class ConfigBuilder(key: String) {
}
private object ConfigHelpers {
+ def toEnum[E <: Enumeration](s: String, enumClass: E, key: String):
enumClass.Value = {
+ try {
+ enumClass.withName(s.trim.toUpperCase(Locale.ROOT))
+ } catch {
+ case _: NoSuchElementException =>
+ throw new IllegalArgumentException(
+ s"$key should be one of ${enumClass.values.mkString(", ")}, but was
$s")
+ }
+ }
Review Comment:
Since parsing uses `s.trim`, the exception message can be misleading when
the user passes values with leading/trailing whitespace (it prints the
untrimmed original). Consider reporting the trimmed value and/or quoting it
(e.g., `but was '...'`) for clearer diagnostics.
##########
gluten-substrait/src/test/scala/org/apache/gluten/execution/PushAggregateThroughJoinSuite.scala:
##########
@@ -93,89 +94,99 @@ class PushAggregateThroughJoinSuite extends PlanTest with
SharedSparkSession {
testCase: PushdownCase,
maxDepth: Int,
expectedPushCount: Int,
- expectedAggCount: Int,
- partialMergeEnabled: Boolean): Unit = {
- withSQLConf(
- GlutenConfig.PUSH_AGGREGATE_THROUGH_JOIN_ENABLED.key -> "true",
- GlutenConfig.PUSH_AGGREGATE_THROUGH_JOIN_MAX_DEPTH.key ->
maxDepth.toString,
- GlutenConfig.PUSH_AGGREGATE_THROUGH_JOIN_PARTIAL_MERGE_ENABLED.key ->
- partialMergeEnabled.toString
- ) {
- val (withoutRuleRows, withoutRuleLogicalPlan, withoutRulePhysicalPlan) =
- withExtraPlanning(Nil, Nil) {
- val df = spark.sql(testCase.inputSql)
- (
- df.collect().toSeq.sortBy(_.toString()),
- df.queryExecution.optimizedPlan,
- finalExecutedPlan(df.queryExecution.executedPlan)
- )
- }
-
- val (withRuleRows, withRuleLogicalPlan, withRulePhysicalPlan) =
- withExtraPlanning(Seq(joinAggregateRule), Nil) {
- joinAggregateRule.resetSuccessfulPushCount()
- val df = spark.sql(testCase.inputSql)
- val withRuleRows = df.collect().toSeq.sortBy(_.toString())
- val withRulePlan = df.queryExecution.optimizedPlan
- val withRulePhysicalPlan =
finalExecutedPlan(df.queryExecution.executedPlan)
- val aggregateNodeCount = withRulePlan.collect { case _: Aggregate =>
1 }.size
- val nodesWithMissingInput = withRulePlan.collect {
- case p if p.missingInput.nonEmpty => p
+ expectedAggCounts: ExpectedAggCounts): Unit = {
+ val modes = Seq(
+ "none" -> expectedAggCounts.none,
+ "top" -> expectedAggCounts.top,
+ "all" -> expectedAggCounts.all)
Review Comment:
These mode strings are manually duplicated and can drift from the actual
enum (`PartialMergeMode`) (e.g., if new modes are added/renamed). Consider
generating `modes` from the enum values (and mapping to the corresponding
expected count), or at least referencing the enum’s names (e.g.,
`PartialMergeMode.NONE.toString.toLowerCase(Locale.ROOT)`) to avoid typos and
keep tests aligned with the supported set.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
