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

zml1206 · 2024-11-20T12:00:13Z

What changes were proposed in this pull request?

(Fixes: #8010)

How was this patch tested?

Local test.
Before PR:

After PR:

…ition is null

github-actions · 2024-11-20T12:00:32Z

#8010

github-actions · 2024-11-20T12:00:45Z

Run Gluten Clickhouse CI on x86

github-actions · 2024-11-20T13:50:59Z

Run Gluten Clickhouse CI on x86

github-actions · 2024-11-20T22:36:49Z

Run Gluten Clickhouse CI on x86

zhztheplayer · 2024-11-21T02:55:33Z

Thanks!

Though it feels like a workaround since there is no real filter node in Velox query plan? Do we have other better options?

zml1206 · 2024-11-21T03:26:25Z

Though it feels like a workaround since there is no real filter node in Velox query plan? Do we have other better options?

How about set metricsUpdater to MetricsUpdater.None when getRemainingCondition is null?
In this way, there is no metrics information.

zhztheplayer · 2024-11-21T05:21:24Z

How about set metricsUpdater to MetricsUpdater.None when getRemainingCondition is null?

Sounds fair. Let's see if it works.

BTW I think one of the promising ways is to exclude the filter node from Spark query plan as well. Though I remember there were some issues with an relevant attempt.

zml1206 · 2024-11-21T05:36:52Z

BTW I think one of the promising ways is to exclude the filter node from Spark query plan as well. Though I remember there were some issues with an relevant attempt.

We can't exclude the filter node, because it's output may differ from child's output.

github-actions · 2024-11-21T05:46:12Z

Run Gluten Clickhouse CI on x86

github-actions · 2024-11-21T08:18:57Z

Run Gluten Clickhouse CI on x86

zml1206 · 2024-11-21T09:00:55Z

backends-velox/src/main/scala/org/apache/gluten/metrics/MetricsUtil.scala

@@ -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)


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

zml1206 · 2024-11-21T10:22:24Z

@zhztheplayer Can you help take a look again? Thanks.

zhztheplayer · 2024-11-22T03:24:26Z

backends-velox/src/main/scala/org/apache/gluten/metrics/MetricsUtil.scala

@@ -219,6 +216,7 @@ object MetricsUtil extends Logging {
        })

    mutNode.updater match {
+      case MetricsUpdater.None =>


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?

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.

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.

github-actions · 2024-11-22T08:53:50Z

Run Gluten Clickhouse CI on x86

zhztheplayer · 2024-11-25T04:03:30Z

gluten-substrait/src/main/scala/org/apache/gluten/execution/ExpandExecTransformer.scala

@@ -112,13 +115,12 @@ case class ExpandExecTransformer(

  override protected def doTransform(context: SubstraitContext): TransformContext = {
    val childCtx = child.asInstanceOf[TransformSupport].transform(context)
-    val operatorId = context.nextOperatorId(this.nodeName)
-    if (projections == null || projections.isEmpty) {
+    if (metricsUpdater == MetricsUpdater.None) {


Hi @zml1206

Can we create a new utility method, e.g, isNoop that can be called both by doTransform and metricUpdater in each operator? Thanks.

zhztheplayer

Other code looks good to me.

github-actions · 2024-11-25T05:33:34Z

Run Gluten Clickhouse CI on x86

github-actions · 2024-11-25T05:46:03Z

Run Gluten Clickhouse CI on x86

zhztheplayer

One nit. Thanks!

zhztheplayer · 2024-11-25T06:07:22Z

...-substrait/src/main/scala/org/apache/gluten/execution/BasicPhysicalOperatorTransformer.scala

@@ -61,7 +61,9 @@ abstract class FilterExecTransformerBase(val cond: Expression, val input: SparkP
    case _ => false
  }

-  override def metricsUpdater(): MetricsUpdater = if (getRemainingCondition == null) {
+  override def isLoop: Boolean = getRemainingCondition == null


s/isLoop/isNoop/

zhztheplayer · 2024-11-25T06:07:30Z

gluten-substrait/src/main/scala/org/apache/gluten/execution/ExpandExecTransformer.scala

@@ -48,7 +48,9 @@ case class ExpandExecTransformer(
    AttributeSet.fromAttributeSets(projections.flatten.map(_.references))
  }

-  override def metricsUpdater(): MetricsUpdater = if (projections == null || projections.isEmpty) {
+  override def isLoop: Boolean = projections == null || projections.isEmpty


s/isLoop/isNoop/

zhztheplayer · 2024-11-25T06:07:34Z

gluten-substrait/src/main/scala/org/apache/gluten/execution/SortExecTransformer.scala

@@ -44,7 +44,9 @@ case class SortExecTransformer(
  @transient override lazy val metrics =
    BackendsApiManager.getMetricsApiInstance.genSortTransformerMetrics(sparkContext)

-  override def metricsUpdater(): MetricsUpdater = if (sortOrder == null || sortOrder.isEmpty) {
+  override def isLoop: Boolean = sortOrder == null || sortOrder.isEmpty


s/isLoop/isNoop/

zhztheplayer · 2024-11-25T06:07:40Z

gluten-substrait/src/main/scala/org/apache/gluten/execution/WindowExecTransformer.scala

-  override def metricsUpdater(): MetricsUpdater = if (
-    windowExpression == null || windowExpression.isEmpty
-  ) {
+  override def isLoop: Boolean = windowExpression == null || windowExpression.isEmpty


s/isLoop/isNoop/

[VL] Inherit child metrics when FilterExecTransformer's remainingCond…

2336fb4

…ition is null

github-actions bot added CORE works for Gluten Core VELOX labels Nov 20, 2024

fix ut

416ca53

zml1206 force-pushed the GLUTEN-8010 branch from 6e6de36 to 416ca53 Compare November 20, 2024 22:36

zml1206 requested a review from zhztheplayer November 21, 2024 01:05

Don't generate native metrics if filter's remainingCondition is null

9bafdd8

github-actions bot removed the VELOX label Nov 21, 2024

zml1206 changed the title ~~[GLUTEN-8010][VL] Inherit child metrics when FilterExecTransformer's remainingCondition is null~~ [GLUTEN-8010][VL] Don't generate native metrics if filter's remainingCondition is null Nov 21, 2024

fix

78f2c93

github-actions bot added the VELOX label Nov 21, 2024

fix

0ddc7b3

github-actions bot removed the CORE works for Gluten Core label Nov 21, 2024

zml1206 commented Nov 21, 2024

View reviewed changes

zhztheplayer reviewed Nov 22, 2024

View reviewed changes

zml1206 force-pushed the GLUTEN-8010 branch from 9fd91b2 to 0ddc7b3 Compare November 22, 2024 05:37

fix

9e4b75f

github-actions bot added CORE works for Gluten Core and removed VELOX labels Nov 22, 2024

github-actions bot added the CLICKHOUSE label Nov 22, 2024

zml1206 changed the title ~~[GLUTEN-8010][VL] Don't generate native metrics if filter's remainingCondition is null~~ [GLUTEN-8010][CORE] Don't generate native metrics if transformer don't generate relNode Nov 22, 2024

zml1206 requested a review from zhztheplayer November 25, 2024 01:19

zhztheplayer reviewed Nov 25, 2024

View reviewed changes

update

3affe1c

update

a0d15f1

zhztheplayer approved these changes Nov 25, 2024

View reviewed changes

zml1206 merged commit e3682fd into apache:main Nov 25, 2024
46 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

zml1206 commented Nov 20, 2024 •

edited

Loading

github-actions bot commented Nov 20, 2024

github-actions bot commented Nov 20, 2024

github-actions bot commented Nov 20, 2024

github-actions bot commented Nov 20, 2024

zhztheplayer commented Nov 21, 2024

zml1206 commented Nov 21, 2024 •

edited

Loading

zhztheplayer commented Nov 21, 2024 •

edited

Loading

zml1206 commented Nov 21, 2024

github-actions bot commented Nov 21, 2024

github-actions bot commented Nov 21, 2024

zml1206 Nov 21, 2024 •

edited

Loading

zml1206 commented Nov 21, 2024

zhztheplayer Nov 22, 2024

zml1206 Nov 22, 2024

zhztheplayer Nov 22, 2024

github-actions bot commented Nov 22, 2024

zhztheplayer Nov 25, 2024

zhztheplayer left a comment

github-actions bot commented Nov 25, 2024

github-actions bot commented Nov 25, 2024

zhztheplayer left a comment

zhztheplayer Nov 25, 2024

zhztheplayer Nov 25, 2024

zhztheplayer Nov 25, 2024

zhztheplayer Nov 25, 2024

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

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

Conversation

zml1206 commented Nov 20, 2024 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

github-actions bot commented Nov 20, 2024

github-actions bot commented Nov 20, 2024

github-actions bot commented Nov 20, 2024

github-actions bot commented Nov 20, 2024

zhztheplayer commented Nov 21, 2024

zml1206 commented Nov 21, 2024 • edited Loading

zhztheplayer commented Nov 21, 2024 • edited Loading

zml1206 commented Nov 21, 2024

github-actions bot commented Nov 21, 2024

github-actions bot commented Nov 21, 2024

zml1206 Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

zml1206 commented Nov 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 22, 2024

Choose a reason for hiding this comment

zhztheplayer left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 25, 2024

github-actions bot commented Nov 25, 2024

zhztheplayer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zml1206 commented Nov 20, 2024 •

edited

Loading

zml1206 commented Nov 21, 2024 •

edited

Loading

zhztheplayer commented Nov 21, 2024 •

edited

Loading

zml1206 Nov 21, 2024 •

edited

Loading