alamb commented on code in PR #23849:
URL: https://github.com/apache/datafusion/pull/23849#discussion_r3648182949
##########
datafusion/physical-plan/src/aggregates/group_values/multi_group_by/mod.rs:
##########
@@ -1352,6 +1359,51 @@ mod tests {
}
}
+ /// Decimal256 GROUP BY on the column-wise fast path (mirroring
Decimal128):
Review Comment:
does this add additional coverage compared to the slt test? I feel like it
is a lot of new tests for a single line code change that just instantiates a
new accumulator.
In other words, think just the end to end test is enough 🤔
##########
datafusion/sqllogictest/test_files/aggregate.slt:
##########
@@ -2327,6 +2327,39 @@ statement ok
DROP TABLE approx_distinct_interval_test;
+# GROUP BY over Decimal256 group keys. Decimal256 columns route through the
+# GroupValuesColumn column-wise fast path (like Decimal128), so a Decimal256
key
Review Comment:
this is a bunch of implementation detail -- I think the comment can just
focus on what it is testing (aka Decimal256 keys) The rest is likely irrelevant
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
