Copilot commented on code in PR #13284:
URL: https://github.com/apache/gluten/pull/13284#discussion_r4271587958
##########
gluten-core/src/main/scala/org/apache/gluten/config/ConfigBuilder.scala:
##########
@@ -97,6 +98,10 @@ private[gluten] case class ConfigBuilder(key: String) {
new TypedConfigBuilder(this, identity)
}
+ def enumConf(e: Enumeration): TypedConfigBuilder[e.Value] = {
Review Comment:
`enumConf(e: Enumeration): TypedConfigBuilder[e.Value]` relies on a
path-dependent return type while accepting an argument typed as the widened
`Enumeration`. This can make typing/inference brittle when callers store the
enum in a non-singleton-typed value (e.g., `val e: Enumeration =
PartialMergeMode`). A more robust API is to make `enumConf` generic (e.g., `def
enumConf[E <: Enumeration](e: E): TypedConfigBuilder[e.Value]`) so the enum
instance type is preserved consistently at call sites.
##########
gluten-substrait/src/test/scala/org/apache/gluten/execution/PushAggregateThroughJoinSuite.scala:
##########
@@ -93,89 +97,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,
Review Comment:
The baseline `(withoutRuleRows, withoutRuleLogicalPlan,
withoutRulePhysicalPlan)` is recomputed for each `partialMergeMode`, even
though it should be identical within a `runCaseWithMaxDepth` invocation (the
rule/strategy lists are empty). Consider computing the baseline once outside
`modes.foreach` and reusing it across modes to reduce test runtime and
redundant query executions.
##########
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:
This error message would be more actionable if it quoted and normalized the
offending value (e.g., showing the trimmed value and/or the uppercased value
actually used for lookup). As-is, values containing leading/trailing whitespace
are hard to diagnose because the message prints the raw input without quotes.
--
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]
