You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@gluten.apache.org by "loudongfeng (via GitHub)" <gi...@apache.org> on 2024/04/07 14:27:50 UTC
[PR] [GLUTEN-5215][CH]stable precision in statistical aggregation functions [incubator-gluten]
loudongfeng opened a new pull request, #5311:
URL: https://github.com/apache/incubator-gluten/pull/5311
1. use varSampStable,varPopStable ... to replace varSamp, varPop ...
2. use fixed nullable property for corr like aggregation's input
3. convert Infinite to Null to keep compatiable with Spark
## What changes were proposed in this pull request?
Fixes: \#5215
## How was this patch tested?
tested by UT
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-5215][CH]stable precision in statistical aggregation functions [incubator-gluten]
Posted by "lgbo-ustc (via GitHub)" <gi...@apache.org>.
lgbo-ustc commented on PR #5311:
URL: https://github.com/apache/incubator-gluten/pull/5311#issuecomment-2044094122
Is there any chance to modify the implementation in `CH` ? It will bring additional costs to add a`ifInfinite` at the end.
Or could you give some performance test result to know how much is the cost
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-5215][CH]stable precision in statistical aggregation functions [incubator-gluten]
Posted by "lgbo-ustc (via GitHub)" <gi...@apache.org>.
lgbo-ustc commented on code in PR #5311:
URL: https://github.com/apache/incubator-gluten/pull/5311#discussion_r1556831739
##########
backends-clickhouse/src/main/scala/org/apache/gluten/execution/CHHashAggregateExecTransformer.scala:
##########
@@ -349,13 +349,18 @@ case class CHHashAggregateExecTransformer(
(makeStructTypeSingleOne(child.dataType, child.nullable), child.nullable)
case covar: Covariance =>
var fields = Seq[(DataType, Boolean)]()
- fields = fields :+ (covar.left.dataType, covar.left.nullable)
- fields = fields :+ (covar.right.dataType, covar.right.nullable)
+ // when COVAR_SAMP's input is non-nullable , due to its output maybe null in spark,
+ // backend parser will cast input type to nullable for intermediate agg,
+ // While final stage still use input type no changed.
+ // It causes type miss match between partial and final AggregationFunction
+ // so directly assign attr.nullable here, which should always be nullable
+ fields = fields :+ (covar.left.dataType, attr.nullable)
+ fields = fields :+ (covar.right.dataType, attr.nullable)
Review Comment:
I am concerned about this change. If the nullability of input is different from result, it could cause an exception when CH parses the intemediate aggregate result in the final stage. How could this avoid ?
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-5215][CH]stable precision in statistical aggregation functions [incubator-gluten]
Posted by "loudongfeng (via GitHub)" <gi...@apache.org>.
loudongfeng commented on code in PR #5311:
URL: https://github.com/apache/incubator-gluten/pull/5311#discussion_r1557868604
##########
backends-clickhouse/src/main/scala/org/apache/gluten/execution/CHHashAggregateExecTransformer.scala:
##########
@@ -349,13 +349,18 @@ case class CHHashAggregateExecTransformer(
(makeStructTypeSingleOne(child.dataType, child.nullable), child.nullable)
case covar: Covariance =>
var fields = Seq[(DataType, Boolean)]()
- fields = fields :+ (covar.left.dataType, covar.left.nullable)
- fields = fields :+ (covar.right.dataType, covar.right.nullable)
+ // when COVAR_SAMP's input is non-nullable , due to its output maybe null in spark,
+ // backend parser will cast input type to nullable for intermediate agg,
+ // While final stage still use input type no changed.
+ // It causes type miss match between partial and final AggregationFunction
+ // so directly assign attr.nullable here, which should always be nullable
+ fields = fields :+ (covar.left.dataType, attr.nullable)
+ fields = fields :+ (covar.right.dataType, attr.nullable)
Review Comment:
Please refer to the newly added test case "covar_samp non-nullable input", which use a temp inline table and all columns are non-nullable.Before this fix, it will fail.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-5215][CH]stable precision in statistical aggregation functions [incubator-gluten]
Posted by "lgbo-ustc (via GitHub)" <gi...@apache.org>.
lgbo-ustc commented on code in PR #5311:
URL: https://github.com/apache/incubator-gluten/pull/5311#discussion_r1556817411
##########
backends-clickhouse/src/main/scala/org/apache/gluten/execution/CHHashAggregateExecTransformer.scala:
##########
@@ -349,13 +349,18 @@ case class CHHashAggregateExecTransformer(
(makeStructTypeSingleOne(child.dataType, child.nullable), child.nullable)
case covar: Covariance =>
var fields = Seq[(DataType, Boolean)]()
- fields = fields :+ (covar.left.dataType, covar.left.nullable)
- fields = fields :+ (covar.right.dataType, covar.right.nullable)
+ // when COVAR_SAMP's input is non-nullable , due to its output maybe null in spark,
+ // backend parser will cast input type to nullable for intermediate agg,
+ // While final stage still use input type no changed.
+ // It causes type miss match between partial and final AggregationFunction
+ // so directly assign attr.nullable here, which should always be nullable
+ fields = fields :+ (covar.left.dataType, attr.nullable)
+ fields = fields :+ (covar.right.dataType, attr.nullable)
Review Comment:
is there any case?
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-5215][CH]stable precision in statistical aggregation functions [incubator-gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #5311:
URL: https://github.com/apache/incubator-gluten/pull/5311#issuecomment-2041488772
Run Gluten Clickhouse CI
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-5215][CH]stable precision in statistical aggregation functions [incubator-gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #5311:
URL: https://github.com/apache/incubator-gluten/pull/5311#issuecomment-2041488717
https://github.com/apache/incubator-gluten/issues/5215
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-5215][CH]stable precision in statistical aggregation functions [incubator-gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #5311:
URL: https://github.com/apache/incubator-gluten/pull/5311#issuecomment-2041490832
Run Gluten Clickhouse CI
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-5215][CH]stable precision in statistical aggregation functions [incubator-gluten]
Posted by "taiyang-li (via GitHub)" <gi...@apache.org>.
taiyang-li commented on code in PR #5311:
URL: https://github.com/apache/incubator-gluten/pull/5311#discussion_r1556863971
##########
cpp-ch/local-engine/Parser/AggregateFunctionParser.cpp:
##########
@@ -174,6 +174,28 @@ const DB::ActionsDAG::Node * AggregateFunctionParser::convertNodeTypeIfNeeded(
return func_node;
}
+const DB::ActionsDAG::Node * AggregateFunctionParser::convertInfiniteIfNeeded(
+ const CommonFunctionInfo & func_info,
+ const DB::ActionsDAG::Node * func_node,
+ DB::ActionsDAGPtr & actions_dag) const
+{
+ static std::unordered_set<String> infinite_aggs{"varSampStable", "stddevSampStable", "covarSampStable"};
Review Comment:
It seems tricky. Pls Implement convertInfiniteIfNeeded for `infinite_aggs` and keep it dummy for other aggregate functions.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-5215][CH]stable precision in statistical aggregation functions [incubator-gluten]
Posted by "taiyang-li (via GitHub)" <gi...@apache.org>.
taiyang-li commented on code in PR #5311:
URL: https://github.com/apache/incubator-gluten/pull/5311#discussion_r1556865941
##########
cpp-ch/local-engine/Parser/AggregateFunctionParser.cpp:
##########
@@ -174,6 +174,28 @@ const DB::ActionsDAG::Node * AggregateFunctionParser::convertNodeTypeIfNeeded(
return func_node;
}
+const DB::ActionsDAG::Node * AggregateFunctionParser::convertInfiniteIfNeeded(
+ const CommonFunctionInfo & func_info,
+ const DB::ActionsDAG::Node * func_node,
+ DB::ActionsDAGPtr & actions_dag) const
+{
+ static std::unordered_set<String> infinite_aggs{"varSampStable", "stddevSampStable", "covarSampStable"};
Review Comment:
Maybe reusing `convertNodeTypeIfNeeded` interface is better choice than adding another not so common interface `convertInfiniteIfNeeded`
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-5215][CH]stable precision in statistical aggregation functions [incubator-gluten]
Posted by "loudongfeng (via GitHub)" <gi...@apache.org>.
loudongfeng closed pull request #5311: [GLUTEN-5215][CH]stable precision in statistical aggregation functions
URL: https://github.com/apache/incubator-gluten/pull/5311
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-5215][CH]stable precision in statistical aggregation functions [incubator-gluten]
Posted by "lgbo-ustc (via GitHub)" <gi...@apache.org>.
lgbo-ustc commented on code in PR #5311:
URL: https://github.com/apache/incubator-gluten/pull/5311#discussion_r1558508629
##########
backends-clickhouse/src/main/scala/org/apache/gluten/execution/CHHashAggregateExecTransformer.scala:
##########
@@ -349,13 +349,18 @@ case class CHHashAggregateExecTransformer(
(makeStructTypeSingleOne(child.dataType, child.nullable), child.nullable)
case covar: Covariance =>
var fields = Seq[(DataType, Boolean)]()
- fields = fields :+ (covar.left.dataType, covar.left.nullable)
- fields = fields :+ (covar.right.dataType, covar.right.nullable)
+ // when COVAR_SAMP's input is non-nullable , due to its output maybe null in spark,
+ // backend parser will cast input type to nullable for intermediate agg,
+ // While final stage still use input type no changed.
+ // It causes type miss match between partial and final AggregationFunction
+ // so directly assign attr.nullable here, which should always be nullable
+ fields = fields :+ (covar.left.dataType, attr.nullable)
+ fields = fields :+ (covar.right.dataType, attr.nullable)
Review Comment:
Is it that the intermediate result will be nullable any way in `CH` ?
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-5215][CH]stable precision in statistical aggregation functions [incubator-gluten]
Posted by "loudongfeng (via GitHub)" <gi...@apache.org>.
loudongfeng commented on PR #5311:
URL: https://github.com/apache/incubator-gluten/pull/5311#issuecomment-2058483332
Find that spark also return Infinity in some cases, so convert Infinity to null does not work.
spark-sql --master local[4] --conf spark.sql.shuffle.partitions=3
```sql
drop table if exists example_table1;
CREATE TABLE example_table1(id int, value double) USING parquet;
INSERT INTO example_table1 VALUES
(1, 4.9E-322),
(2, 1.7976931348623157E308),
(3, 1.7976931348623157E308);
SELECT var_samp(value) FROM example_table1;
```
the result will be Infinity., while return NaN for the following:
spark-sql --master local[2]
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-5215][CH]stable precision in statistical aggregation functions [incubator-gluten]
Posted by "taiyang-li (via GitHub)" <gi...@apache.org>.
taiyang-li commented on code in PR #5311:
URL: https://github.com/apache/incubator-gluten/pull/5311#discussion_r1556866284
##########
cpp-ch/local-engine/Parser/AggregateFunctionParser.h:
##########
@@ -117,6 +117,12 @@ class AggregateFunctionParser
DB::ActionsDAGPtr & actions_dag,
bool with_nullability) const;
+ /// Make a postprojection for the function result.
Review Comment:
Not a good comment.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-5215][CH]stable precision in statistical aggregation functions [incubator-gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #5311:
URL: https://github.com/apache/incubator-gluten/pull/5311#issuecomment-2043066617
Run Gluten Clickhouse CI
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-5215][CH]stable precision in statistical aggregation functions [incubator-gluten]
Posted by "loudongfeng (via GitHub)" <gi...@apache.org>.
loudongfeng commented on PR #5311:
URL: https://github.com/apache/incubator-gluten/pull/5311#issuecomment-2067612789
Nullable property issue only affect stable statistical aggregation functions, so no need to change for now.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-5215][CH]stable precision in statistical aggregation functions [incubator-gluten]
Posted by "taiyang-li (via GitHub)" <gi...@apache.org>.
taiyang-li commented on code in PR #5311:
URL: https://github.com/apache/incubator-gluten/pull/5311#discussion_r1556847102
##########
cpp-ch/local-engine/Parser/aggregate_function_parser/CommonAggregateFunctionParser.cpp:
##########
@@ -25,17 +25,17 @@ REGISTER_COMMON_AGGREGATE_FUNCTION_PARSER(Sum, sum, sum)
REGISTER_COMMON_AGGREGATE_FUNCTION_PARSER(Avg, avg, avg)
REGISTER_COMMON_AGGREGATE_FUNCTION_PARSER(Min, min, min)
REGISTER_COMMON_AGGREGATE_FUNCTION_PARSER(Max, max, max)
-REGISTER_COMMON_AGGREGATE_FUNCTION_PARSER(StdDev, stddev, stddev_samp)
Review Comment:
How much performance degradation if replace them all to stable implementations?
According to https://clickhouse.com/docs/en/sql-reference/aggregate-functions/reference/stddevsamp: use the stddevSampStable function. It works slower but provides a lower computational error.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-5215][CH]stable precision in statistical aggregation functions [incubator-gluten]
Posted by "taiyang-li (via GitHub)" <gi...@apache.org>.
taiyang-li commented on code in PR #5311:
URL: https://github.com/apache/incubator-gluten/pull/5311#discussion_r1556853664
##########
cpp-ch/local-engine/Functions/FunctionIfInfinite.cpp:
##########
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+#include <Functions/IFunction.h>
+#include <Functions/FunctionHelpers.h>
+#include <Functions/FunctionFactory.h>
+#include <DataTypes/DataTypesNumber.h>
+#include <DataTypes/getLeastSupertype.h>
+#include <Core/ColumnNumbers.h>
+
+
+namespace local_engine
+{
+using namespace DB;
+namespace
+{
+
+ /// IfInfinite(x, y) is equivalent to isInfinite(x) ? y : x.
Review Comment:
No need to implement a new function. It is too heavy. All we need is just update ActionsDAGs in convertInfiniteIfNeeded. Pls refer to cpp-ch/local-engine/Parser/scalar_function_parser/*.cpp
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org