You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@gluten.apache.org by "zhli1142015 (via GitHub)" <gi...@apache.org> on 2024/05/15 13:44:25 UTC
[PR] [VL] Enable more types and partial merge mode for HLL [incubator-gluten]
zhli1142015 opened a new pull request, #5754:
URL: https://github.com/apache/incubator-gluten/pull/5754
## What changes were proposed in this pull request?
Enable more types and partial merge mode for HLL offload.
(Fixes: \#ISSUE-ID)
## How was this patch tested?
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] [VL] Enable partial merge mode for HLL [incubator-gluten]
Posted by "zhli1142015 (via GitHub)" <gi...@apache.org>.
zhli1142015 commented on code in PR #5754:
URL: https://github.com/apache/incubator-gluten/pull/5754#discussion_r1618082997
##########
backends-velox/src/main/scala/org/apache/gluten/execution/HashAggregateExecTransformer.scala:
##########
@@ -241,21 +226,21 @@ abstract class HashAggregateExecTransformer(
}
aggregateFunction match {
- case hllAdapter: HLLAdapter =>
+ case _ if aggregateFunction.aggBufferAttributes.size > 1 =>
+ generateMergeCompanionNode()
+ case _ =>
aggregateMode match {
- case Partial =>
- // For Partial mode output type is binary.
+ case Partial | PartialMerge =>
val partialNode = ExpressionBuilder.makeAggregateFunction(
VeloxAggregateFunctionsBuilder.create(args, aggregateFunction, aggregateMode),
childrenNodeList,
modeKeyWord,
ConverterUtils.getTypeNode(
- hllAdapter.inputAggBufferAttributes.head.dataType,
- hllAdapter.inputAggBufferAttributes.head.nullable)
+ aggregateFunction.inputAggBufferAttributes.head.dataType,
Review Comment:
Spark HLL `inputAggBufferAttributes` is `Seq[Long]`. We rewrite HLL in Gluten to adapt Velox's function signature, which immediate type is `varbinary`. https://github.com/facebookincubator/velox/blob/main/velox/functions/prestosql/aggregates/ApproxDistinctAggregate.cpp#L476C33-L476C49.
--
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] [VL] Enable more types and partial merge mode for HLL [incubator-gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #5754:
URL: https://github.com/apache/incubator-gluten/pull/5754#issuecomment-2112582372
<!--
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/LICEGJ-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.
-->
Thanks for opening a pull request!
Could you open an issue for this pull request on Github Issues?
https://github.com/apache/incubator-gluten/issues
Then could you also rename ***commit message*** and ***pull request title*** in the following format?
[GLUTEN-${ISSUES_ID}][COMPONENT]feat/fix: ${detailed message}
See also:
* [Other pull requests](https://github.com/apache/incubator-gluten/pulls/)
--
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] [VL] Enable partial merge mode for HLL [incubator-gluten]
Posted by "liujiayi771 (via GitHub)" <gi...@apache.org>.
liujiayi771 commented on code in PR #5754:
URL: https://github.com/apache/incubator-gluten/pull/5754#discussion_r1618075938
##########
backends-velox/src/main/scala/org/apache/gluten/execution/HashAggregateExecTransformer.scala:
##########
@@ -241,21 +226,21 @@ abstract class HashAggregateExecTransformer(
}
aggregateFunction match {
- case hllAdapter: HLLAdapter =>
+ case _ if aggregateFunction.aggBufferAttributes.size > 1 =>
+ generateMergeCompanionNode()
+ case _ =>
aggregateMode match {
- case Partial =>
- // For Partial mode output type is binary.
+ case Partial | PartialMerge =>
val partialNode = ExpressionBuilder.makeAggregateFunction(
VeloxAggregateFunctionsBuilder.create(args, aggregateFunction, aggregateMode),
childrenNodeList,
modeKeyWord,
ConverterUtils.getTypeNode(
- hllAdapter.inputAggBufferAttributes.head.dataType,
- hllAdapter.inputAggBufferAttributes.head.nullable)
+ aggregateFunction.inputAggBufferAttributes.head.dataType,
Review Comment:
I think we should use `dataType`, but for HLL, the `dataType` and `inputAggBufferAttributes.head` should also be the same, both being `LongType`, right?
--
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] [VL] Enable partial merge mode for HLL [incubator-gluten]
Posted by "liujiayi771 (via GitHub)" <gi...@apache.org>.
liujiayi771 commented on code in PR #5754:
URL: https://github.com/apache/incubator-gluten/pull/5754#discussion_r1618062532
##########
backends-velox/src/main/scala/org/apache/gluten/execution/HashAggregateExecTransformer.scala:
##########
@@ -241,21 +226,21 @@ abstract class HashAggregateExecTransformer(
}
aggregateFunction match {
- case hllAdapter: HLLAdapter =>
+ case _ if aggregateFunction.aggBufferAttributes.size > 1 =>
+ generateMergeCompanionNode()
+ case _ =>
aggregateMode match {
- case Partial =>
- // For Partial mode output type is binary.
+ case Partial | PartialMerge =>
val partialNode = ExpressionBuilder.makeAggregateFunction(
VeloxAggregateFunctionsBuilder.create(args, aggregateFunction, aggregateMode),
childrenNodeList,
modeKeyWord,
ConverterUtils.getTypeNode(
- hllAdapter.inputAggBufferAttributes.head.dataType,
- hllAdapter.inputAggBufferAttributes.head.nullable)
+ aggregateFunction.inputAggBufferAttributes.head.dataType,
Review Comment:
@zhli1142015 Gluten searches agg functions in Velox based on the input and output type signatures of the agg function. I think using `inputAggBufferAttributes.head` to construct the `aggFuncNode` for partial agg is inaccurate. It would be more appropriate to follow the original `dataType`.
I searched through the Spark code and found that, for most agg functions, the `dataType` and `inputAggBufferAttributes.head` are the same. However, for decimal avg, the `inputAggBufferAttributes.head` is sum `DecimalType`, and while the final output `resultType` is also `DecimalType`, the precision and scale are different.
Even though Velox does not consider precision and scale when searching agg functions, there might be other agg function types that are completely different. For avg, if `inputAggBufferAttributes` contains count before sum, then the two are entirely inconsistent.
Therefore, I believe using `dataType` is more appropriate as it aligns with most agg 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] [VL] Enable partial merge mode for HLL [incubator-gluten]
Posted by "zhli1142015 (via GitHub)" <gi...@apache.org>.
zhli1142015 commented on PR #5754:
URL: https://github.com/apache/incubator-gluten/pull/5754#issuecomment-2134957304
cc @liujiayi771 and @PHILO-HE , thanks.
--
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] [VL] Enable partial merge mode for HLL [incubator-gluten]
Posted by "zhli1142015 (via GitHub)" <gi...@apache.org>.
zhli1142015 merged PR #5754:
URL: https://github.com/apache/incubator-gluten/pull/5754
--
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] [VL] Enable partial merge mode for HLL [incubator-gluten]
Posted by "GlutenPerfBot (via GitHub)" <gi...@apache.org>.
GlutenPerfBot commented on PR #5754:
URL: https://github.com/apache/incubator-gluten/pull/5754#issuecomment-2136495566
===== Performance report for TPCH SF2000 with Velox backend, for reference only ====
<table border="1" cellpadding="0" cellspacing="0">
<tbody>
<tr>
<td><font color=#000000>query</font></td>
<td><font color=#000000>log/native_5754_time.csv</font></td>
<td><font color=#000000>log/native_master_05_28_2024_d7374bd8f_time.csv</font></td>
<td><font color=#000000>difference</font></td>
<td><font color=#000000>percentage</font></td>
</tr>
<tr>
<td><font color=#6F9915>q1</font></td>
<td><font color=#6F9915>35.57</font></td>
<td><font color=#6F9915>34.04</font></td>
<td><font color=#6F9915>-1.531</font></td>
<td><font color=#6F9915>95.70%</font></td>
</tr>
<tr>
<td><font color=#6F9915>q2</font></td>
<td><font color=#6F9915>23.79</font></td>
<td><font color=#6F9915>23.62</font></td>
<td><font color=#6F9915>-0.169</font></td>
<td><font color=#6F9915>99.29%</font></td>
</tr>
<tr>
<td><font color=#6F9915>q3</font></td>
<td><font color=#6F9915>37.27</font></td>
<td><font color=#6F9915>36.93</font></td>
<td><font color=#6F9915>-0.337</font></td>
<td><font color=#6F9915>99.10%</font></td>
</tr>
<tr>
<td><font color=#6F9915>q4</font></td>
<td><font color=#6F9915>32.98</font></td>
<td><font color=#6F9915>32.10</font></td>
<td><font color=#6F9915>-0.882</font></td>
<td><font color=#6F9915>97.33%</font></td>
</tr>
<tr>
<td><font color=#F92663>q5</font></td>
<td><font color=#F92663>67.92</font></td>
<td><font color=#F92663>69.91</font></td>
<td><font color=#F92663>1.992</font></td>
<td><font color=#F92663>102.93%</font></td>
</tr>
<tr>
<td><font color=#6F9915>q6</font></td>
<td><font color=#6F9915>7.57</font></td>
<td><font color=#6F9915>5.97</font></td>
<td><font color=#6F9915>-1.602</font></td>
<td><font color=#6F9915>78.83%</font></td>
</tr>
<tr>
<td><font color=#6F9915>q7</font></td>
<td><font color=#6F9915>82.53</font></td>
<td><font color=#6F9915>80.25</font></td>
<td><font color=#6F9915>-2.273</font></td>
<td><font color=#6F9915>97.25%</font></td>
</tr>
<tr>
<td><font color=#6F9915>q8</font></td>
<td><font color=#6F9915>85.26</font></td>
<td><font color=#6F9915>84.79</font></td>
<td><font color=#6F9915>-0.477</font></td>
<td><font color=#6F9915>99.44%</font></td>
</tr>
<tr>
<td><font color=#F92663>q9</font></td>
<td><font color=#F92663>120.70</font></td>
<td><font color=#F92663>121.55</font></td>
<td><font color=#F92663>0.849</font></td>
<td><font color=#F92663>100.70%</font></td>
</tr>
<tr>
<td><font color=#6F9915>q10</font></td>
<td><font color=#6F9915>44.04</font></td>
<td><font color=#6F9915>43.64</font></td>
<td><font color=#6F9915>-0.402</font></td>
<td><font color=#6F9915>99.09%</font></td>
</tr>
<tr>
<td><font color=#F92663>q11</font></td>
<td><font color=#F92663>21.62</font></td>
<td><font color=#F92663>22.27</font></td>
<td><font color=#F92663>0.651</font></td>
<td><font color=#F92663>103.01%</font></td>
</tr>
<tr>
<td><font color=#F92663>q12</font></td>
<td><font color=#F92663>25.70</font></td>
<td><font color=#F92663>26.90</font></td>
<td><font color=#F92663>1.199</font></td>
<td><font color=#F92663>104.66%</font></td>
</tr>
<tr>
<td><font color=#6F9915>q13</font></td>
<td><font color=#6F9915>52.98</font></td>
<td><font color=#6F9915>52.40</font></td>
<td><font color=#6F9915>-0.580</font></td>
<td><font color=#6F9915>98.91%</font></td>
</tr>
<tr>
<td><font color=#F92663>q14</font></td>
<td><font color=#F92663>20.60</font></td>
<td><font color=#F92663>21.59</font></td>
<td><font color=#F92663>0.987</font></td>
<td><font color=#F92663>104.79%</font></td>
</tr>
<tr>
<td><font color=#6F9915>q15</font></td>
<td><font color=#6F9915>34.48</font></td>
<td><font color=#6F9915>31.46</font></td>
<td><font color=#6F9915>-3.021</font></td>
<td><font color=#6F9915>91.24%</font></td>
</tr>
<tr>
<td><font color=#F92663>q16</font></td>
<td><font color=#F92663>13.96</font></td>
<td><font color=#F92663>13.97</font></td>
<td><font color=#F92663>0.017</font></td>
<td><font color=#F92663>100.12%</font></td>
</tr>
<tr>
<td><font color=#F92663>q17</font></td>
<td><font color=#F92663>101.88</font></td>
<td><font color=#F92663>102.83</font></td>
<td><font color=#F92663>0.949</font></td>
<td><font color=#F92663>100.93%</font></td>
</tr>
<tr>
<td><font color=#6F9915>q18</font></td>
<td><font color=#6F9915>147.99</font></td>
<td><font color=#6F9915>145.92</font></td>
<td><font color=#6F9915>-2.077</font></td>
<td><font color=#6F9915>98.60%</font></td>
</tr>
<tr>
<td><font color=#F92663>q19</font></td>
<td><font color=#F92663>13.48</font></td>
<td><font color=#F92663>14.82</font></td>
<td><font color=#F92663>1.338</font></td>
<td><font color=#F92663>109.93%</font></td>
</tr>
<tr>
<td><font color=#F92663>q20</font></td>
<td><font color=#F92663>26.57</font></td>
<td><font color=#F92663>31.06</font></td>
<td><font color=#F92663>4.485</font></td>
<td><font color=#F92663>116.88%</font></td>
</tr>
<tr>
<td><font color=#F92663>q21</font></td>
<td><font color=#F92663>259.97</font></td>
<td><font color=#F92663>261.69</font></td>
<td><font color=#F92663>1.723</font></td>
<td><font color=#F92663>100.66%</font></td>
</tr>
<tr>
<td><font color=#6F9915>q22</font></td>
<td><font color=#6F9915>12.62</font></td>
<td><font color=#6F9915>12.27</font></td>
<td><font color=#6F9915>-0.354</font></td>
<td><font color=#6F9915>97.20%</font></td>
</tr>
<tr>
<td><font color=#F92663>total</font></td>
<td><font color=#F92663>1269.49</font></td>
<td><font color=#F92663>1269.97</font></td>
<td><font color=#F92663>0.484</font></td>
<td><font color=#F92663>100.04%</font></td>
</tr>
</tbody>
</table>
--
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] [VL] Enable partial merge mode for HLL [incubator-gluten]
Posted by "liujiayi771 (via GitHub)" <gi...@apache.org>.
liujiayi771 commented on code in PR #5754:
URL: https://github.com/apache/incubator-gluten/pull/5754#discussion_r1617565686
##########
backends-velox/src/main/scala/org/apache/gluten/execution/HashAggregateExecTransformer.scala:
##########
@@ -241,21 +226,21 @@ abstract class HashAggregateExecTransformer(
}
aggregateFunction match {
- case hllAdapter: HLLAdapter =>
+ case _ if aggregateFunction.aggBufferAttributes.size > 1 =>
+ generateMergeCompanionNode()
+ case _ =>
aggregateMode match {
- case Partial =>
- // For Partial mode output type is binary.
+ case Partial | PartialMerge =>
val partialNode = ExpressionBuilder.makeAggregateFunction(
VeloxAggregateFunctionsBuilder.create(args, aggregateFunction, aggregateMode),
childrenNodeList,
modeKeyWord,
ConverterUtils.getTypeNode(
- hllAdapter.inputAggBufferAttributes.head.dataType,
- hllAdapter.inputAggBufferAttributes.head.nullable)
+ aggregateFunction.inputAggBufferAttributes.head.dataType,
Review Comment:
The original logic for handling `inputAggBufferAttributes.head` in the `Partial` and `PartialMerge` was only applicable to HLL. Why can this now be applied to all aggregations?
--
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] [VL] Enable partial merge mode for HLL [incubator-gluten]
Posted by "liujiayi771 (via GitHub)" <gi...@apache.org>.
liujiayi771 commented on code in PR #5754:
URL: https://github.com/apache/incubator-gluten/pull/5754#discussion_r1618122674
##########
backends-velox/src/main/scala/org/apache/gluten/execution/HashAggregateExecTransformer.scala:
##########
@@ -241,21 +226,21 @@ abstract class HashAggregateExecTransformer(
}
aggregateFunction match {
- case hllAdapter: HLLAdapter =>
+ case _ if aggregateFunction.aggBufferAttributes.size > 1 =>
+ generateMergeCompanionNode()
+ case _ =>
aggregateMode match {
- case Partial =>
- // For Partial mode output type is binary.
+ case Partial | PartialMerge =>
val partialNode = ExpressionBuilder.makeAggregateFunction(
VeloxAggregateFunctionsBuilder.create(args, aggregateFunction, aggregateMode),
childrenNodeList,
modeKeyWord,
ConverterUtils.getTypeNode(
- hllAdapter.inputAggBufferAttributes.head.dataType,
- hllAdapter.inputAggBufferAttributes.head.nullable)
+ aggregateFunction.inputAggBufferAttributes.head.dataType,
Review Comment:
@zhli1142015 I see, I misread some of the code, and your modification is correct.
--
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] [VL] Enable partial merge mode for HLL [incubator-gluten]
Posted by "zhli1142015 (via GitHub)" <gi...@apache.org>.
zhli1142015 commented on code in PR #5754:
URL: https://github.com/apache/incubator-gluten/pull/5754#discussion_r1618023007
##########
backends-velox/src/main/scala/org/apache/gluten/execution/HashAggregateExecTransformer.scala:
##########
@@ -241,21 +226,21 @@ abstract class HashAggregateExecTransformer(
}
aggregateFunction match {
- case hllAdapter: HLLAdapter =>
+ case _ if aggregateFunction.aggBufferAttributes.size > 1 =>
+ generateMergeCompanionNode()
+ case _ =>
aggregateMode match {
- case Partial =>
- // For Partial mode output type is binary.
+ case Partial | PartialMerge =>
val partialNode = ExpressionBuilder.makeAggregateFunction(
VeloxAggregateFunctionsBuilder.create(args, aggregateFunction, aggregateMode),
childrenNodeList,
modeKeyWord,
ConverterUtils.getTypeNode(
- hllAdapter.inputAggBufferAttributes.head.dataType,
- hllAdapter.inputAggBufferAttributes.head.nullable)
+ aggregateFunction.inputAggBufferAttributes.head.dataType,
Review Comment:
I think the problem here is we assume the `inputAggBufferAttributes.head.dataType` is same with `aggregateFunction.dataType` for partial mode an partialMerge mode. This is not true for HLL. Before we add the special handling for partial mode only and disabled HLL for partialMerge mode.
Thanks.
--
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] [VL] Enable partial merge mode for HLL [incubator-gluten]
Posted by "liujiayi771 (via GitHub)" <gi...@apache.org>.
liujiayi771 commented on code in PR #5754:
URL: https://github.com/apache/incubator-gluten/pull/5754#discussion_r1618062532
##########
backends-velox/src/main/scala/org/apache/gluten/execution/HashAggregateExecTransformer.scala:
##########
@@ -241,21 +226,21 @@ abstract class HashAggregateExecTransformer(
}
aggregateFunction match {
- case hllAdapter: HLLAdapter =>
+ case _ if aggregateFunction.aggBufferAttributes.size > 1 =>
+ generateMergeCompanionNode()
+ case _ =>
aggregateMode match {
- case Partial =>
- // For Partial mode output type is binary.
+ case Partial | PartialMerge =>
val partialNode = ExpressionBuilder.makeAggregateFunction(
VeloxAggregateFunctionsBuilder.create(args, aggregateFunction, aggregateMode),
childrenNodeList,
modeKeyWord,
ConverterUtils.getTypeNode(
- hllAdapter.inputAggBufferAttributes.head.dataType,
- hllAdapter.inputAggBufferAttributes.head.nullable)
+ aggregateFunction.inputAggBufferAttributes.head.dataType,
Review Comment:
@zhli1142015 Gluten searches agg functions in Velox based on the raw input and final output signatures of the agg function. I think using `inputAggBufferAttributes.head` to construct the `aggFuncNode` for partial agg is inaccurate. It would be more appropriate to follow the original `dataType`.
I searched through the Spark code and found that, for most agg functions, the `dataType` and `inputAggBufferAttributes.head` are the same. However, for decimal avg, the `inputAggBufferAttributes.head` is sum `DecimalType`, and while the final output `resultType` is also `DecimalType`, the precision and scale are different.
Even though Velox does not consider precision and scale when searching agg functions, there might be other agg function types that are completely different. For avg, if `inputAggBufferAttributes` contains count before sum, then the two are entirely inconsistent.
Therefore, I believe using `dataType` is more appropriate as it aligns with most agg functions.
cc @zhztheplayer.
--
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] [VL] Enable partial merge mode for HLL [incubator-gluten]
Posted by "zhli1142015 (via GitHub)" <gi...@apache.org>.
zhli1142015 commented on code in PR #5754:
URL: https://github.com/apache/incubator-gluten/pull/5754#discussion_r1618068576
##########
backends-velox/src/main/scala/org/apache/gluten/execution/HashAggregateExecTransformer.scala:
##########
@@ -241,21 +226,21 @@ abstract class HashAggregateExecTransformer(
}
aggregateFunction match {
- case hllAdapter: HLLAdapter =>
+ case _ if aggregateFunction.aggBufferAttributes.size > 1 =>
+ generateMergeCompanionNode()
+ case _ =>
aggregateMode match {
- case Partial =>
- // For Partial mode output type is binary.
+ case Partial | PartialMerge =>
val partialNode = ExpressionBuilder.makeAggregateFunction(
VeloxAggregateFunctionsBuilder.create(args, aggregateFunction, aggregateMode),
childrenNodeList,
modeKeyWord,
ConverterUtils.getTypeNode(
- hllAdapter.inputAggBufferAttributes.head.dataType,
- hllAdapter.inputAggBufferAttributes.head.nullable)
+ aggregateFunction.inputAggBufferAttributes.head.dataType,
Review Comment:
I'm little confused about this:
> @zhli1142015 Gluten searches agg functions in Velox based on the raw input and final output signatures of the agg function. I think using `inputAggBufferAttributes.head` to construct the `aggFuncNode` for partial agg is inaccurate. It would be more appropriate to follow the original `dataType`.
Do you mean here I still need to use `dataType` for partial mode even `dataType` and `inputAggBufferAttributes.head` are not same?
--
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] [VL] Enable partial merge mode for HLL [incubator-gluten]
Posted by "zhli1142015 (via GitHub)" <gi...@apache.org>.
zhli1142015 commented on code in PR #5754:
URL: https://github.com/apache/incubator-gluten/pull/5754#discussion_r1618124871
##########
backends-velox/src/main/scala/org/apache/gluten/execution/HashAggregateExecTransformer.scala:
##########
@@ -241,21 +226,21 @@ abstract class HashAggregateExecTransformer(
}
aggregateFunction match {
- case hllAdapter: HLLAdapter =>
+ case _ if aggregateFunction.aggBufferAttributes.size > 1 =>
+ generateMergeCompanionNode()
+ case _ =>
aggregateMode match {
- case Partial =>
- // For Partial mode output type is binary.
+ case Partial | PartialMerge =>
val partialNode = ExpressionBuilder.makeAggregateFunction(
VeloxAggregateFunctionsBuilder.create(args, aggregateFunction, aggregateMode),
childrenNodeList,
modeKeyWord,
ConverterUtils.getTypeNode(
- hllAdapter.inputAggBufferAttributes.head.dataType,
- hllAdapter.inputAggBufferAttributes.head.nullable)
+ aggregateFunction.inputAggBufferAttributes.head.dataType,
Review Comment:
Sure, thanks for your review.
--
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