Jefffrey commented on code in PR #17888:
URL: https://github.com/apache/datafusion/pull/17888#discussion_r2458721759
##########
datafusion/functions-aggregate-common/src/utils.rs:
##########
@@ -145,8 +148,11 @@ impl<T: DecimalType> DecimalAverager<T> {
if let Ok(value) =
sum.mul_checked(self.target_mul.div_wrapping(self.sum_mul)) {
let new_value = value.div_wrapping(count);
- let validate =
- T::validate_decimal_precision(new_value,
self.target_precision);
+ let validate = T::validate_decimal_precision(
Review Comment:
I'll add "Closes #3666" to the PR body 👍
##########
datafusion/sqllogictest/test_files/dates.slt:
##########
@@ -85,9 +85,14 @@ g
h
## Plan error when compare Utf8 and timestamp in where clause
-statement error DataFusion error: type_coercion\ncaused by\nError during
planning: Cannot coerce arithmetic expression Timestamp\(Nanosecond,
Some\("\+00:00"\)\) \+ Utf8 to valid types
+statement error
select i_item_desc from test
where d3_date > now() + '5 days';
+----
+DataFusion error: type_coercion
+caused by
+Error during planning: Cannot coerce arithmetic expression Timestamp(ns,
"+00:00") + Utf8 to valid types
Review Comment:
I thought the expected error comes before the query not after, for SLTs 🤔
##########
datafusion/sqllogictest/test_files/arrow_typeof.slt:
##########
@@ -61,13 +61,13 @@ Decimal128(38, 10)
query T
SELECT arrow_typeof(now()::timestamp)
----
-Timestamp(Nanosecond, None)
+Timestamp(ns)
Review Comment:
I believe we'll need to update
https://github.com/apache/datafusion/blob/main/docs/source/user-guide/sql/data_types.md
and
https://github.com/apache/datafusion/blob/main/docs/source/user-guide/sql/scalar_functions.md
(the latter via updating the source docs in code) too, but can do in a follow
up
##########
datafusion/core/tests/parquet/filter_pushdown.rs:
##########
@@ -632,7 +632,7 @@ async fn predicate_cache_pushdown_default() ->
datafusion_common::Result<()> {
#[tokio::test]
async fn predicate_cache_pushdown_disable() -> datafusion_common::Result<()> {
// Can disable the cache even with filter pushdown by setting the size to
0. In this case we
- // expect the inner records are reported but no records are read from the
cache
Review Comment:
nit: wording a bit off here, since it reads as
> Can disable the cache even with filter pushdown by setting the size to 0.
In this case we no records are read from the cache and no metrics are reported
Should be this maybe?
> Can disable the cache even with filter pushdown by setting the size to 0.
This results in no records being read from the cache and no metrics being
reported
--
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]
