Update DataFusion to arrow 6.0 #984

alamb · 2021-09-09T16:49:55Z

Which issue does this PR close?

Rationale for this change

Pickup improvements in upstream arrow-rs crate and allow projects downstream of datafusion to use new arrow-rs

~~This PR is intended for demonstration only; I don't intend to merge this PR as is, but I plan to create a real PR once arrow 6.0 has been released officially.~~

What changes are included in this PR?

Upgrade to arrow-rs to arrow 6.0.0
Change DataFusion to pass tests with arrow update

Are there any user-facing changes?

The biggest user facing change I think is that sort in arrow-rs is no longer stable, so thus sort in DataFusion will no longer be stable either

alamb · 2021-09-09T16:50:20Z

datafusion/src/execution/context.rs

            execute("SELECT c1, AVG(c2) FROM test WHERE c1 = 123 GROUP BY c1", 4).await?;

-        let expected = vec!["++", "||", "++", "++"];
+        let expected = vec![


Required due to apache/arrow-rs#656

alamb · 2021-09-09T17:34:47Z

datafusion/src/physical_plan/sort_preserving_merge.rs

-            expr: col("c7", &schema).unwrap(),
-            options: SortOptions::default(),
-        }];
+        let sort = vec![


This is required to ensure a consistent sort between the two sorting strategies due to the change to use unstable sorting, introduced in apache/arrow-rs#552. The SortPreservingMerge operator happens to be a stable sort but the sort kernel used by the Sort Operator no longer is.

The issue here is that the original sort key, c7 has duplicated values as can be seen in this screenshot (e.g the value 18 is repeated in several rows):

The full output is in basic.txt
and partition.txt

FYI @tustvold

@alamb FYI, @yjshen proposed a slightly cleaner fix for this failure by switching to column c12: houqp#4

Sounds like a good plan if it works 👍

Updated this PR to use c12

do we need to add a stable sort option in arrow sort kernel?

I am not sure @jimexist -- I think it is fairly common that analytic systems don't really have the notion of "stable" sort (because the data doesn't have any well defined sort order in storage).

In DataFusion, for example, the order that the rows are produced (and how they are partitioned) depends on the DataSource (as well as they may be re-arranged by a repartition / coalesce operator). This stable sorting really only is useful for testing when the output has a single RecordBatch I think

It may be best not to get used to / rely on that stable sorting

alamb · 2021-09-09T17:54:29Z

datafusion/src/physical_plan/sort_preserving_merge.rs

-            expr: col("c7", &schema).unwrap(),
-            options: SortOptions::default(),
-        }];
+        let sort = vec![


FYI @tustvold

alamb · 2021-09-09T17:56:11Z

datafusion/tests/sql.rs

-               first_value(cast(c4 as Int)) over (partition by c3), \
-               last_value(cast(c4 as Int)) over (partition by c3), \
-               nth_value(cast(c4 as Int), 2) over (partition by c3) \
+               first_value(cast(c4 as Int)) over (partition by c3 order by c3, c4), \


This change is also required due to the change to use unstable sorting in apache/arrow-rs#552 but the test is non deterministic according to the sql spec (the query output depends on implementation details).

Specifically, it is computing first_value last_value and nth_value for partitions that have more than one value of the partition by value c3 but does not specify an order by clause to determine how those values should be sorted. :

fyi @jimexist

alamb · 2021-09-09T18:05:47Z

ballista/rust/core/src/serde/logical_plan/to_proto.rs

                })
            }
+            DataType::Map(_, _) => {
+                unimplemented!("Ballista does not yet support Map data type")


Needed due to apache/arrow-rs#491 -- I am not enough of an expert to implement protobuf serialization in Ballista for a new DataType at this time but I suspect it is not very hard

alamb · 2021-10-18T16:51:21Z

datafusion/src/physical_plan/expressions/in_list.rs

-            null_bit_buffer,
-            0,
-            vec![Buffer::from(buffer)],
+        let data = unsafe {


Due to apache/arrow-rs#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?

alamb · 2021-10-18T17:16:36Z

datafusion/src/physical_plan/sort_preserving_merge.rs

        let schema = test::aggr_test_schema();
        let sort = vec![PhysicalSortExpr {
-            expr: col("c7", &schema).unwrap(),
+            expr: col("c12", &schema).unwrap(),


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.

alamb · 2021-10-18T17:17:12Z

datafusion/src/physical_plan/sort_preserving_merge.rs

                options: SortOptions::default(),
            },
+            PhysicalSortExpr {
+                expr: col("c12", &schema).unwrap(),


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)

houqp

LGTM!

Dandandan · 2021-10-19T09:58:28Z

🎉

… of time (apache#984)

github-actions bot added ballista labels Sep 9, 2021

alamb commented Sep 9, 2021

View reviewed changes

alamb mentioned this pull request Sep 9, 2021

Rework the python bindings using conversion traits from arrow-rs #873

Merged

alamb commented Sep 9, 2021

View reviewed changes

alamb force-pushed the alamb/update_to_arrow_6.0 branch from 6122563 to 281d6bc Compare September 9, 2021 17:58

github-actions bot added the python label Sep 9, 2021

alamb commented Sep 9, 2021

View reviewed changes

alamb force-pushed the alamb/update_to_arrow_6.0 branch from 8231f95 to f089217 Compare September 9, 2021 20:58

alamb mentioned this pull request Sep 9, 2021

Minimal MapArray support apache/arrow-rs#491

Merged

9 tasks

houqp mentioned this pull request Sep 13, 2021

Bump arrow to 6.0.0-SNAPSHOT and bring map support to schema delta-io/delta-rs#375

Merged

alamb force-pushed the alamb/update_to_arrow_6.0 branch from f089217 to 5971729 Compare September 19, 2021 10:52

alamb force-pushed the alamb/update_to_arrow_6.0 branch from 5971729 to 89d214b Compare October 13, 2021 19:28

alamb added 6 commits October 18, 2021 12:47

Update to arrow 6.0

7f6a02e

Update empty result

ef57066

Fix up sort_preserving_merge test

819ce3b

Use column c12, per @yjshen

9ab55fb

Add minimal ballista support

537552a

Update to use new arrow apis

95a8c58

alamb force-pushed the alamb/update_to_arrow_6.0 branch from 5375d91 to 95a8c58 Compare October 18, 2021 16:48

alamb added 3 commits October 18, 2021 12:53

Fixup sort preserving merge test again

b30a35f

make sort deterministic

4e39f6d

less whitespace

f2b04ad

alamb commented Oct 18, 2021

View reviewed changes

alamb marked this pull request as ready for review October 18, 2021 17:21

alamb changed the title ~~Update DataFusion to arrow 6.0 (WIP)~~ Update DataFusion to arrow 6.0 Oct 18, 2021

patch up avro

2899c49

alamb requested review from Dandandan and houqp October 18, 2021 18:28

alamb requested a review from jimexist October 18, 2021 18:28

houqp approved these changes Oct 18, 2021

View reviewed changes

houqp mentioned this pull request Oct 19, 2021

Add support for parsing timestamps from CSV files #958

Closed

jimexist approved these changes Oct 19, 2021

View reviewed changes

Dandandan approved these changes Oct 19, 2021

View reviewed changes

Dandandan merged commit 57d7777 into apache:master Oct 19, 2021

alamb deleted the alamb/update_to_arrow_6.0 branch October 19, 2021 10:10

houqp added the enhancement New feature or request label Nov 4, 2021

unkloud pushed a commit to unkloud/datafusion that referenced this pull request Mar 23, 2025

fix: The spilled_bytes metric of CometSortExec should be size instead…

dcc4a8a

… of time (apache#984)

Update DataFusion to arrow 6.0 #984

Update DataFusion to arrow 6.0 #984

Uh oh!

Conversation

alamb commented Sep 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb Sep 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

houqp left a comment

Choose a reason for hiding this comment

Uh oh!

Dandandan commented Oct 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

alamb commented Sep 9, 2021 •

edited

Loading

alamb Sep 9, 2021 •

edited

Loading