[GLUTEN-8557][CH] Collapse nested function calls for `And`/`Or` for performance optimization #8558

KevinyhZou · 2025-01-17T09:10:57Z

What changes were proposed in this pull request?

(Please fill in changes proposed in this fix)

(Fixes: #8557)

How was this patch tested?

test by ut

github-actions · 2025-01-17T09:11:16Z

#8557

github-actions · 2025-01-17T09:11:29Z

Run Gluten Clickhouse CI on x86

github-actions · 2025-01-21T10:07:07Z

Run Gluten Clickhouse CI on x86

github-actions · 2025-01-21T11:06:58Z

Run Gluten Clickhouse CI on x86

github-actions · 2025-02-04T08:32:27Z

Run Gluten Clickhouse CI on x86

github-actions · 2025-02-04T08:34:25Z

Run Gluten Clickhouse CI on x86

github-actions · 2025-02-05T11:19:20Z

Run Gluten Clickhouse CI on x86

github-actions · 2025-02-10T01:47:46Z

Run Gluten Clickhouse CI on x86

github-actions · 2025-02-10T01:48:58Z

Run Gluten Clickhouse CI on x86

github-actions · 2025-02-11T03:28:49Z

Run Gluten Clickhouse CI on x86

KevinyhZou · 2025-02-11T09:08:48Z

性能测试

性能测试：测试数据量6千万行，分别执行三次，查看端到端执行时间

get_json_object: select count(1) from test_tbl1 where get_json_object(get_json_object(d, '$.a'), '$.b') = 'c';
优化前：53.194s, 52.18s, 51.603s
优化后：24.894s, 25.042s, 24.724s

and: select count(1) from test_tbl34 where id = 1 and d != 'axx' and d != 'cxx' and d != 'zxx';
优化前： 3.918s，4.106s，3.835s
优化后： 3.123s, 3.303s, 3.288s

or： select count(1) from test_tbl34 where id = 1 or d != 'axx' or d != 'cxx' or d != 'zxx';
优化前：2.611s，2.765s， 2.484s
优化后：2.3s, 2.409s, 2.283s;

get_struct_field: select count(1) from test_tbl31 where d.e.d.y = 'y123';
优化前：80.819s， 80.517s，83.3s
优化后：80.885s, 80.108s, 81.121s;

taiyang-li · 2025-02-12T02:14:10Z

shims/common/src/main/scala/org/apache/gluten/config/GlutenConfig.scala

+      .internal()
+      .doc("Collapse nested functions as one for optimization.")
+      .stringConf
+      .createWithDefault("get_struct_field,get_json_object");


what about and, or

taiyang-li · 2025-02-12T02:16:47Z

gluten-substrait/src/main/scala/org/apache/gluten/expression/ExpressionConverter.scala

@@ -97,7 +97,10 @@ object ExpressionConverter extends SQLConfHelper with Logging {
    if (udf.udfName.isEmpty) {
      throw new GlutenNotSupportException("UDF name is not found!")
    }
-    val substraitExprName = UDFMappings.scalaUDFMap.get(udf.udfName.get)
+    var substraitExprName = UDFMappings.scalaUDFMap.get(udf.udfName.get)


collapsedFunctionsMap和udf有什么关系？看起来逻辑上没必要耦合在一块，最好能解耦

taiyang-li · 2025-02-12T02:19:17Z

backends-clickhouse/src/main/scala/org/apache/gluten/extension/CollapseNestedExpressions.scala

+import org.apache.spark.sql.execution.SparkPlan
+import org.apache.spark.sql.types.{DataType, DataTypes}
+
+case class CollapseNestedExpressions(spark: SparkSession) extends Rule[SparkPlan] {


add necessary comment

taiyang-li · 2025-02-12T02:20:28Z

backends-clickhouse/src/main/scala/org/apache/gluten/extension/CollapseNestedExpressions.scala

+  }
+
+  private def canBeOptimized(plan: SparkPlan): Boolean = plan match {
+    case p: ProjectExecTransformer =>


can expression in generate operator be optimized ?

It seems can not

taiyang-li · 2025-02-12T02:32:56Z

cpp-ch/local-engine/Functions/SparkFunctionGetJsonObject.h

@@ -462,8 +463,55 @@ class GetJsonObjectImpl

    static size_t getNumberOfIndexArguments(const DB::ColumnsWithTypeAndName & arguments) { return arguments.size() - 1; }

+    bool insertResultToColumn(DB::IColumn & dest, typename JSONParser::Element & root, std::vector<std::shared_ptr<DB::GeneratorJSONPath<JSONParser>>> & generator_json_paths, size_t & json_path_pos) const


@lgbo-ustc pls review changes related to get_json_object

lgbo-ustc · 2025-02-12T06:03:46Z

cpp-ch/local-engine/Parser/scalar_function_parser/getJSONObject.cpp

@@ -59,9 +59,9 @@ class GetJSONObjectParser : public FunctionParser
        DB::ActionsDAG & actions_dag) const override
    {
        const auto & args = substrait_func.arguments();
-        if (args.size() != 2)
+        if (args.size() < 2)


get_json_object(get_json_object(d, '$.a'), '$.b') => optimize to get_json_object(d, '$.a', '$.b'), which may have more than 2 arguments.

lgbo-ustc · 2025-02-12T06:18:30Z

解释下get_json_object的优化思路，不是太明白为何在这里要改成多个路径参数。
已经有 #8305 优化的情况下，这个改动带来哪些变化？

lgbo-ustc · 2025-02-12T06:18:50Z

建议不同函数的优化拆分到不同的PR里面

lgbo-ustc · 2025-02-12T06:30:33Z

cpp-ch/local-engine/Functions/SparkFunctionGetJsonObject.h

+    mutable size_t total_normalized_rows = 0;
+
+    template<typename JSONParser, typename JSONStringSerializer>
+    void insertResultToColumn(


It seems to be complex, I guess there should be a simpler implement with less branches

Should explain which case it is for each branch

taiyang-li

LGTM

github-actions · 2025-02-19T07:02:20Z

Run Gluten Clickhouse CI on x86

github-actions · 2025-02-19T10:37:04Z

Run Gluten Clickhouse CI on x86

github-actions · 2025-02-19T10:42:14Z

Run Gluten Clickhouse CI on x86

github-actions · 2025-02-19T10:45:41Z

Run Gluten Clickhouse CI on x86

PHILO-HE

Thanks for the work!

PHILO-HE · 2025-02-21T05:21:23Z

shims/common/src/main/scala/org/apache/gluten/config/GlutenConfig.scala

+      .internal()
+      .doc("Collapse nested functions as one for optimization.")
+      .stringConf
+      .createWithDefault("and,or");


@KevinyhZou, do you find any unsuitable corner cases? E.g., wrong result, performance degradation. If no, can we always enable this optimization without introducing a config?

Not found unsuitable case for the ci testing and my own testing. But I'm afraid there maybe some unsuitable case in our online sqls, which the ci testing do not cover，So I think it‘s better to keep them

PHILO-HE · 2025-02-21T05:55:05Z

backends-clickhouse/src/main/scala/org/apache/gluten/extension/CollapseNestedExpressions.scala

+       * ScalaUDF at first, and then pass the ScalaUDF to clickhouse backend. e.g. And(And(a=1,
+       * b=2),c=3) can be optimized to And(a=1, b=2, c=3)，but And(a=1, b=2, c=3) can not be
+       * supported by spark `And` function, so we need to convert it to ScalaUDF, with name is
+       * `And`, and have 3 arguments, when pass the `ScalaUDF(#and(a=1,b=2,c=3))` to clickhouse


Must we need a ScalaUDF? Can we make an optimized plan just compatible with the backend? Regardless of that it's not compatible with Spark.

I have removed the ScalaUDF， and re-implement this by use CHCollapseExpressionTransformer, help to review this pr ? thanks @PHILO-HE

github-actions · 2025-03-03T12:37:17Z

Run Gluten Clickhouse CI on x86

github-actions · 2025-03-03T12:39:11Z

Run Gluten Clickhouse CI on x86

github-actions · 2025-03-03T12:50:20Z

Run Gluten Clickhouse CI on x86

github-actions · 2025-03-04T12:02:56Z

Run Gluten Clickhouse CI on x86

github-actions · 2025-03-06T13:16:06Z

Run Gluten Clickhouse CI on x86

github-actions · 2025-03-07T04:14:58Z

Run Gluten Clickhouse CI on x86

github-actions · 2025-03-07T12:37:22Z

Run Gluten Clickhouse CI on x86

github-actions · 2025-03-08T04:41:16Z

Run Gluten Clickhouse CI on x86

PHILO-HE

Can we also introduce a dedicated rule for this optimization? Maybe, a custom And/Or expression class should be defined when necessary?

PHILO-HE · 2025-03-19T07:03:07Z

...use/src/main/scala/org/apache/gluten/expression/CHCollapseNestedExpressionsTransformer.scala

+    case _ => Option.empty[String]
+  }
+
+  private def canBeOptimized(expr: Expression): Boolean = {


For no nested And/Or case, is it also viewed as an optimizable case?

PHILO-HE · 2025-03-19T07:05:21Z

...use/src/main/scala/org/apache/gluten/expression/CHCollapseNestedExpressionsTransformer.scala

+
+  def getExpressionName(expr: Expression): Option[String] = expr match {
+    case _: And => ExpressionMappings.expressionsMap.get(classOf[And])
+    case _: Or => ExpressionMappings.expressionsMap.get(classOf[Or])


This check seems redundant to the configuration based check.

PHILO-HE · 2025-03-19T07:11:30Z

...use/src/main/scala/org/apache/gluten/expression/CHCollapseNestedExpressionsTransformer.scala

+          case _ => exprCall.children.exists(c => canBeOptimized(c))
+        }
+      case Some(f) =>
+        GlutenConfig.get.getSupportedCollapsedExpressions.split(",").exists(c => c.equals(f))


For mixed And/Or case, is it viewed as optimizable case?

github-actions · 2025-03-19T09:37:40Z

Run Gluten Clickhouse CI on x86

github-actions · 2025-03-19T15:00:04Z

Run Gluten Clickhouse CI on x86

github-actions · 2025-03-20T01:50:12Z

Run Gluten Clickhouse CI on x86

KevinyhZou · 2025-03-20T06:07:25Z

Can we also introduce a dedicated rule for this optimization? Maybe, a custom And/Or expression class should be defined when necessary?

Now I have implement a dedicated rule for this, introduce the CHAnd, CHOr customized expression for optimization.
Mixed nested and/or would be optimized, I have add this case in the ut, non nested and/or would not. please help to review this . @PHILO-HE

github-actions · 2025-03-27T03:15:44Z

Run Gluten Clickhouse CI on x86

github-actions · 2025-03-27T03:52:16Z

Run Gluten Clickhouse CI on x86

KevinyhZou · 2025-03-28T07:21:13Z

cc @PHILO-HE

KevinyhZou marked this pull request as draft January 17, 2025 09:11

github-actions bot added CORE works for Gluten Core CLICKHOUSE labels Jan 17, 2025

KevinyhZou changed the title ~~[GLUTEN-8557][CH]Optimize nested function calls~~ [GLUTEN-8557][CH]Optimize nested function calls for And, GetStructField Jan 17, 2025

KevinyhZou changed the title ~~[GLUTEN-8557][CH]Optimize nested function calls for And, GetStructField~~ [GLUTEN-8557][CH]Optimize nested function calls for And/GetStructField Jan 17, 2025

KevinyhZou changed the title ~~[GLUTEN-8557][CH]Optimize nested function calls for And/GetStructField~~ [GLUTEN-8557][CH]Optimize nested function calls for And/Or/GetStructField/GetJsonObject Jan 21, 2025

KevinyhZou closed this Jan 21, 2025

KevinyhZou reopened this Jan 21, 2025

KevinyhZou marked this pull request as ready for review January 21, 2025 11:06

KevinyhZou force-pushed the opt_nested_funcs branch from b86d9e7 to 2cc64f2 Compare February 4, 2025 08:31

taiyang-li reviewed Feb 12, 2025

View reviewed changes

lgbo-ustc reviewed Feb 12, 2025

View reviewed changes

taiyang-li approved these changes Feb 19, 2025

View reviewed changes

KevinyhZou force-pushed the opt_nested_funcs branch from 9ad3176 to 6e59310 Compare February 19, 2025 10:41

PHILO-HE reviewed Feb 21, 2025

View reviewed changes

PHILO-HE changed the title ~~[GLUTEN-8557][CH]Optimize nested function calls for And/Or~~ [GLUTEN-8557][CH] Collapse nested function calls for And/Or for performance optimization Feb 21, 2025

KevinyhZou force-pushed the opt_nested_funcs branch from ba9497b to 72857c0 Compare March 4, 2025 12:02

KevinyhZou force-pushed the opt_nested_funcs branch from f4738d0 to 2c6d6f6 Compare March 7, 2025 12:36

PHILO-HE reviewed Mar 19, 2025

View reviewed changes

KevinyhZou force-pushed the opt_nested_funcs branch from fb730ea to 578808b Compare March 19, 2025 14:59

KevinyhZou force-pushed the opt_nested_funcs branch from 578808b to 2dbad5d Compare March 20, 2025 01:49

optimize nested function calls

68ad6fd

KevinyhZou force-pushed the opt_nested_funcs branch from 8ac225b to 68ad6fd Compare March 27, 2025 03:51

		@@ -462,8 +463,55 @@ class GetJsonObjectImpl

		static size_t getNumberOfIndexArguments(const DB::ColumnsWithTypeAndName & arguments) { return arguments.size() - 1; }

		bool insertResultToColumn(DB::IColumn & dest, typename JSONParser::Element & root, std::vector<std::shared_ptr<DB::GeneratorJSONPath<JSONParser>>> & generator_json_paths, size_t & json_path_pos) const

[GLUTEN-8557][CH] Collapse nested function calls for And/Or for performance optimization #8558

Are you sure you want to change the base?

[GLUTEN-8557][CH] Collapse nested function calls for And/Or for performance optimization #8558

Conversation

KevinyhZou commented Jan 17, 2025 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

github-actions bot commented Jan 17, 2025

github-actions bot commented Jan 17, 2025

github-actions bot commented Jan 21, 2025

github-actions bot commented Jan 21, 2025

github-actions bot commented Feb 4, 2025

github-actions bot commented Feb 4, 2025

github-actions bot commented Feb 5, 2025

github-actions bot commented Feb 10, 2025

github-actions bot commented Feb 10, 2025

github-actions bot commented Feb 11, 2025

KevinyhZou commented Feb 11, 2025

性能测试

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

taiyang-li Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgbo-ustc commented Feb 12, 2025 • edited Loading

lgbo-ustc commented Feb 12, 2025

Choose a reason for hiding this comment

lgbo-ustc Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

taiyang-li left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 19, 2025

github-actions bot commented Feb 19, 2025

github-actions bot commented Feb 19, 2025

github-actions bot commented Feb 19, 2025

PHILO-HE left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KevinyhZou Feb 24, 2025 • edited Loading

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 Mar 3, 2025

github-actions bot commented Mar 3, 2025

github-actions bot commented Mar 3, 2025

github-actions bot commented Mar 4, 2025

github-actions bot commented Mar 6, 2025

github-actions bot commented Mar 7, 2025

github-actions bot commented Mar 7, 2025

github-actions bot commented Mar 8, 2025

PHILO-HE 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

github-actions bot commented Mar 19, 2025

github-actions bot commented Mar 19, 2025

github-actions bot commented Mar 20, 2025

KevinyhZou commented Mar 20, 2025 • edited Loading

github-actions bot commented Mar 27, 2025

github-actions bot commented Mar 27, 2025

KevinyhZou commented Mar 28, 2025

[GLUTEN-8557][CH] Collapse nested function calls for `And`/`Or` for performance optimization #8558

[GLUTEN-8557][CH] Collapse nested function calls for `And`/`Or` for performance optimization #8558

KevinyhZou commented Jan 17, 2025 •

edited

Loading

taiyang-li Feb 12, 2025 •

edited

Loading

lgbo-ustc commented Feb 12, 2025 •

edited

Loading

lgbo-ustc Feb 12, 2025 •

edited

Loading

KevinyhZou Feb 24, 2025 •

edited

Loading

KevinyhZou commented Mar 20, 2025 •

edited

Loading