[GitHub] [arrow-datafusion] alamb commented on a change in pull request #984: Update DataFusion to arrow 6.0 (WIP)

[GitBox <gi...@apache.org>]

alamb commented on a change in pull request #984:
URL: https://github.com/apache/arrow-datafusion/pull/984#discussion_r731149120



##########
File path: datafusion/src/physical_plan/sort_preserving_merge.rs
##########
@@ -963,6 +963,10 @@ mod tests {
                 expr: col("c7", &schema).unwrap(),
                 options: SortOptions::default(),
             },
+            PhysicalSortExpr {
+                expr: col("c12", &schema).unwrap(),

Review comment:
       added to make the sort output deterministic
   
   Without this change, there is one row that comes out in a slightly different order (same values of `c2` and `c7`)
   
   ![Screen Shot 2021-10-18 at 12 55 18 PM](https://user-images.githubusercontent.com/490673/137777649-b26f63ab-c84d-40b7-8202-1061fe091cbb.png)
   

##########
File path: datafusion/src/physical_plan/expressions/in_list.rs
##########
@@ -47,15 +47,17 @@ macro_rules! compare_op_scalar {
         // same as $left.len()
         let buffer = unsafe { MutableBuffer::from_trusted_len_iter_bool(comparison) };
 
-        let data = ArrayData::new(
-            DataType::Boolean,
-            $left.len(),
-            None,
-            null_bit_buffer,
-            0,
-            vec![Buffer::from(buffer)],
+        let data = unsafe {

Review comment:
       Due to https://github.com/apache/arrow-rs/pull/822 (this can lead to unsafe behavior here if the buffers are not correctly created). The alternate is to use the `try_new` function and skip the (eventual) validation required.
   
   Thoughts?

##########
File path: datafusion/src/physical_plan/sort_preserving_merge.rs
##########
@@ -1183,7 +1191,7 @@ mod tests {
     async fn test_async() {
         let schema = test::aggr_test_schema();
         let sort = vec![PhysicalSortExpr {
-            expr: col("c7", &schema).unwrap(),
+            expr: col("c12", &schema).unwrap(),

Review comment:
       this is needed because `c7` is not unique and this since `arrow-rs` 6.0 sort is no longer stable the output is not deterministic. 




-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org