[VL] Disable FlushableHashAggreagte when aggregates contains sum/avg for floating type#8986
Conversation
|
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? See also: |
|
@zhztheplayer Can you help review this PR? |
There was a problem hiding this comment.
Some users who don't need 100% alignment with Spark may still tend to turn on flushing in this case to speed up queries.
I would suggest having an individual config option like s.g.floatingPointMode=strict/loose to control the tolerance of this kind of diffs in Gluten. While the mode is set to strict, we disable flushing for sum(float), etc.
| "partial aggregation is enabled. Ignored when spark.gluten.sql.columnar.backend." + | ||
| "velox.flushablePartialAggregation=false." | ||
| ) | ||
| .bytesConf(ByteUnit.BYTE) |
There was a problem hiding this comment.
Should it be
.byteConf(...)
.createOptional
kecookier
force-pushed
the
fix-double-sum
branch
from
4176119 to
cc15f83
Compare
|
@zhztheplayer I've updated the code as suggested. Please take a look. |
|
Hi @kecookier sorry for the late response. Missed the notification. Given the purpose is to disable flushing in some cases, do we have to add new option |
@zhztheplayer For easier control of flushable memory during unit tests. |
…or floating type (apache#8986) (cherry picked from commit f667e81) Change-Id: I74a595766972f8b561c98ae45632788a2bdd705f Reviewed-on: https://bigdataoss-internal-review.googlesource.com/c/third_party/apache/incubator-gluten/+/115777 Reviewed-by: Preetesh Verma <preeteshverma@google.com> Reviewed-by: Revanth Venkat Mikkilineni <revanthvenkat@google.com> Tested-by: Srinivas S T <srst@google.com>
2 participants
What changes were proposed in this pull request?
(Fixes: #8985)
FlushableHashAggregatewhen aggregates containsum/avgfor floating types.s.g.s.c.b.v.maxPartialAggregationMemoryto set PartialAgg memory, which has higher priority thans.g.s.c.b.v.maxPartialAggregationMemoryRatio.How was this patch tested?
unit tests