alamb commented on code in PR #7282:
URL: https://github.com/apache/arrow-datafusion/pull/7282#discussion_r1305659102
##########
datafusion/sqllogictest/test_files/ddl.slt:
##########
@@ -302,7 +302,7 @@ CREATE TABLE my_table(c1 float, c2 double, c3 boolean, c4
varchar) AS SELECT *,c
query RRBT rowsort
SELECT * FROM my_table order by c1 LIMIT 1
----
-0.00001 0.000000000001 true 1
+0.00001 0.000000000001 true true
Review Comment:
converting boolean to `"true"` seems like an improvement from `1` to me
##########
datafusion/physical-expr/src/expressions/binary.rs:
##########
@@ -218,21 +132,6 @@ macro_rules! compute_utf8_op {
}};
}
-/// Invoke a compute kernel on a pair of binary data arrays
-macro_rules! compute_binary_op {
- ($LEFT:expr, $RIGHT:expr, $OP:ident, $DT:ident) => {{
- let ll = $LEFT
- .as_any()
- .downcast_ref::<$DT>()
- .expect("compute_op failed to downcast left side array");
- let rr = $RIGHT
- .as_any()
- .downcast_ref::<$DT>()
- .expect("compute_op failed to downcast right side array");
- Ok(Arc::new(paste::expr! {[<$OP _binary>]}(&ll, &rr)?))
- }};
-}
-
/// Invoke a compute kernel on a data array and a scalar value
macro_rules! compute_utf8_op_scalar {
Review Comment:
it seems like eventually we could make a `ColumnarValue::to_arrow_datum`
type function that would allow direct invocation of many of these kernels from
DataFusion without needing layers of dispatch
##########
datafusion/core/src/physical_plan/joins/hash_join.rs:
##########
@@ -851,19 +851,8 @@ fn eq_dyn_null(
null_equals_null: bool,
) -> Result<BooleanArray, ArrowError> {
match (left.data_type(), right.data_type()) {
- (DataType::Null, DataType::Null) => Ok(BooleanArray::new(
- BooleanBuffer::collect_bool(left.len(), |_| null_equals_null),
- None,
- )),
- _ if null_equals_null => {
- let eq: BooleanArray = eq_dyn(left, right)?;
-
- let left_is_null = is_null(left)?;
- let right_is_null = is_null(right)?;
-
- or_kleene(&and(&left_is_null, &right_is_null)?, &eq)
- }
- _ => eq_dyn(left, right),
+ _ if null_equals_null => not_distinct(&left, &right),
Review Comment:
FYI @Dandandan
##########
datafusion/physical-expr/src/expressions/binary/kernels_arrow.rs:
##########
@@ -1,404 +0,0 @@
-// 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/LICENSE-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.
-
-//! This module contains computation kernels that are eventually
-//! destined for arrow-rs but are in datafusion until they are ported.
-
-use arrow::{array::*, datatypes::ArrowNumericType};
Review Comment:
thank you so much @tustvold -- this is great to see this gone
--
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]
