alamb commented on code in PR #8758:
URL: https://github.com/apache/arrow-datafusion/pull/8758#discussion_r1445116776
##########
datafusion/core/tests/user_defined/user_defined_scalar_functions.rs:
##########
@@ -43,7 +43,7 @@ async fn csv_query_custom_udf_with_cast() -> Result<()> {
"+------------------------------------------+",
"| AVG(custom_sqrt(aggregate_test_100.c11)) |",
"+------------------------------------------+",
- "| 0.6584408483418833 |",
+ "| 0.6584408483418835 |",
Review Comment:
this differs in the last decimal place and thus I think is related to
floating point stability and thus this change is fine
##########
datafusion/core/Cargo.toml:
##########
@@ -47,7 +47,6 @@ parquet = ["datafusion-common/parquet", "dep:parquet"]
pyarrow = ["datafusion-common/pyarrow", "parquet"]
regex_expressions = ["datafusion-physical-expr/regex_expressions",
"datafusion-optimizer/regex_expressions"]
serde = ["arrow-schema/serde"]
-simd = ["arrow/simd"]
Review Comment:
Can we also remove the `simd` feature from the main readme as well?
https://github.com/apache/arrow-datafusion/blob/746988a7e5c9f256c3b24ab7e3f30ffd90d542a0/README.md?plain=1#L72
##########
datafusion/physical-expr/src/expressions/case.rs:
##########
@@ -151,16 +151,16 @@ impl CaseExpr {
let then_value = self.when_then_expr[i]
.1
.evaluate_selection(batch, &when_match)?;
- let then_value = match then_value {
- ColumnarValue::Scalar(value) if value.is_null() => {
- new_null_array(&return_type, batch.num_rows())
+
+ current_value = match then_value {
Review Comment:
does `to_scalar` handle the null array case now, so this is no longer needed?
##########
datafusion/core/tests/dataframe/describe.rs:
##########
@@ -40,12 +40,12 @@ async fn describe() -> Result<()> {
"+------------+-------------------+----------+--------------------+--------------------+--------------------+--------------------+--------------------+--------------------+-----------------+------------+-------------------------+--------------------+-------------------+",
Review Comment:
This seems like a better result to me (less numeric stability)
##########
datafusion/physical-plan/src/aggregates/mod.rs:
##########
@@ -1746,6 +1746,7 @@ mod tests {
}
#[tokio::test]
+ #[ignore]
Review Comment:
We should figure out how to update the test to avoid the error, thought --
@kazuyukitanimura do you have any thoughts on how to do so?
##########
datafusion/sql/tests/sql_integration.rs:
##########
@@ -451,10 +451,6 @@ Dml: op=[Insert Into] table=[test_decimal]
"INSERT INTO test_decimal (nonexistent, price) VALUES (1, 2), (4, 5)",
"Schema error: No field named nonexistent. Valid fields are id, price."
)]
-#[case::type_mismatch(
- "INSERT INTO test_decimal SELECT '2022-01-01',
to_timestamp('2022-01-01T12:00:00')",
- "Error during planning: Cannot automatically convert Timestamp(Nanosecond,
None) to Decimal128(10, 2)"
Review 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
