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-8010][CORE] Don't generate native metrics if transformer don't generate relNode #8011

Merged
merged 8 commits into from
Nov 25, 2024
Merged

[GLUTEN-8010][CORE] Don't generate native metrics if transformer don't generate relNode #8011

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2336fb4
[VL] Inherit child metrics when FilterExecTransformer's remainingCond…
zml1206 Nov 20, 2024
416ca53
fix ut
zml1206 Nov 20, 2024
9bafdd8
Don't generate native metrics if filter's remainingCondition is null
zml1206 Nov 21, 2024
78f2c93
fix
zml1206 Nov 21, 2024
0ddc7b3
fix
zml1206 Nov 21, 2024
9e4b75f
fix
zml1206 Nov 22, 2024
3affe1c
update
zml1206 Nov 25, 2024
a0d15f1
update
zml1206 Nov 25, 2024
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
9 changes: 9 additions & 0 deletions backends-velox/src/main/scala/org/apache/gluten/execution/FilterExecTransformer.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,21 @@
*/
package org.apache.gluten.execution

import org.apache.gluten.backendsapi.BackendsApiManager
import org.apache.gluten.metrics.MetricsUpdater

import org.apache.spark.sql.catalyst.expressions.Expression
import org.apache.spark.sql.execution.SparkPlan

case class FilterExecTransformer(condition: Expression, child: SparkPlan)
extends FilterExecTransformerBase(condition, child) {

override def metricsUpdater(): MetricsUpdater = if (getRemainingCondition == null) {
MetricsUpdater.None
} else {
BackendsApiManager.getMetricsApiInstance.genFilterTransformerMetricsUpdater(metrics)
}

override protected def withNewChildInternal(newChild: SparkPlan): FilterExecTransformer =
copy(child = newChild)
}
4 changes: 1 addition & 3 deletions backends-velox/src/main/scala/org/apache/gluten/metrics/MetricsUtil.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,6 @@ object MetricsUtil extends Logging {
MetricsUpdaterTree(
smj.metricsUpdater(),
Seq(treeifyMetricsUpdaters(smj.bufferedPlan), treeifyMetricsUpdaters(smj.streamedPlan)))
case t: TransformSupport if t.metricsUpdater() == MetricsUpdater.None =>
assert(t.children.size == 1, "MetricsUpdater.None can only be used on unary operator")
treeifyMetricsUpdaters(t.children.head)
Copy link
Contributor Author

@zml1206 zml1206 Nov 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It will cause operatorId inconsistencies. It should be handled in updateTransformerMetricsInternal. cc @zhztheplayer

case t: TransformSupport =>
MetricsUpdaterTree(t.metricsUpdater(), t.children.map(treeifyMetricsUpdaters))
case _ =>
Expand Down Expand Up @@ -219,6 +216,7 @@ object MetricsUtil extends Logging {
})

mutNode.updater match {
case MetricsUpdater.None =>
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure about this?

The code is likely something done by MetricsUpdater.Todo. I remember MetricsUpdater.Todo and MetricsUpdater.None don't share the same semantic.

Did you check the UI? Are all the metrics still normal with this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

MetricsUpdater.Todo means that native metrics exist, but does not update them. MetricsUpdater.None means that native metrics do not exist, and updateNativeMetrics is not supported, so this is needed.

Copy link
Member

Choose a reason for hiding this comment

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

I see. My impressions about this part of code is:

  • When registerEmptyRelToOperator is called, a fake operator creation is actually emulated so MetricsUpdator.Todo should be considered
  • When child.asInstanceOf[TransformSupport].transform(context) is used, MetricsUpdator.None should be considered

Would you like to check whether the two rule apply?

And if we no longer need MetricsUpdater.None, we can just remove this type of metrics updater.

case ju: HashJoinMetricsUpdater =>
// JoinRel outputs two suites of metrics respectively for hash build and hash probe.
// Therefore, fetch one more suite of metrics here.
Expand Down
Loading