Update arrow 47.0.0 in DataFusion #7587

tustvold · 2023-09-18T12:14:07Z

Which issue does this PR close?

Closes #.

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Removes the dictionary_expressions feature as no longer necessary

tustvold · 2023-09-18T13:27:33Z

datafusion/substrait/Cargo.toml

@@ -35,7 +35,7 @@ itertools = "0.11"
 object_store = "0.7.0"
 prost = "0.11"
 prost-types = "0.11"
-substrait = "0.13.1"
+substrait = "0.14.0"


Note: unfortunately even the latest subtrait is not yet upgraded to prost 0.12

alamb

Looks great to me -- thank you @tustvold

cc @viirya and @sunchao -- I think this PR changes how expressions with dictionaries are handled (though I think the same underlying code is still called).

alamb · 2023-09-22T13:40:09Z

datafusion/core/tests/memory_limit.rs

+                "| logical_plan  | Sort: t.a ASC NULLS LAST, t.b DESC NULLS FIRST                                                         |",
+                "|               |   TableScan: t projection=[a, b]                                                                       |",
+                "| physical_plan | SortExec: expr=[a@0 ASC NULLS LAST,b@1 DESC]                                                           |",
+                "|               |   MemoryExec: partitions=1, partition_sizes=[5], output_ordering=a@0 ASC NULLS LAST,b@1 ASC NULLS LAST |",


this is because we no longer need the small batch size, right?

alamb · 2023-09-22T13:43:18Z

datafusion/physical-expr/src/expressions/datum.rs

+use std::sync::Arc;
+
+/// Applies a binary [`Datum`] kernel `f` to `lhs` and `rhs`
+pub(crate) fn apply(


I know this is likely obvious to you, but I think it would help to provide some context here of why this function exists

Something like

Suggested change

pub(crate) fn apply(

///

/// This is used to provide a single function implementation that works for all combinations of

/// 2 `ColumnarValue` inputs, and maps the [`ColumnarValue`] abstraction of DataFusion to

/// the [`Datum`] abstraction of arrow-rs

pub(crate) fn apply(

alamb · 2023-09-22T13:45:17Z

datafusion/physical-plan/src/aggregates/group_values/row.rs

    /// Converter for the group values
-    row_converter: CardinalityAwareRowConverter,
+    row_converter: RowConverter,


this was removed as the upstream interning was removed in apache/arrow-rs#4811

cc @JayjeetAtGithub

alamb · 2023-09-22T13:46:06Z

datafusion/physical-plan/src/aggregates/group_values/row.rs

@@ -199,6 +203,20 @@ impl GroupValues for GroupValuesRows {
            }
        };

+        // TODO: Materialize dictionaries in group keys


Do you know if this is described in a ticket yet (I think the answer is no, but I am not 100% sure)

No a ticket would be a good addition

Created #7647

sunchao · 2023-09-22T15:53:35Z

Thanks for the ping @alamb . So just trying to understand the implication of this PR on dictionary.

Removes the dictionary_expressions feature as no longer necessary

This is the only thing that's changed? and we should expect expressions operating on dictionary arrays still behave the same?

Thanks!

tustvold · 2023-09-22T17:39:06Z

There should be no externally visible changes, correct

* Update arrow 47.0.0 * Downgrade prost substrait * Fix deprecations * Prost deprecations * Update pbjson-build * Format * Remove CardinalityAwareRowConverter * Further fixes * Ignore spill tests * Fix tests * Fix Clippy * Update pin * Review feedback * Clippy

github-actions bot added physical-expr Physical Expressions optimizer Optimizer rules core Core DataFusion crate substrait labels Sep 18, 2023

tustvold force-pushed the prepare-arrow-47 branch from e9a91dc to 0a7a1f9 Compare September 18, 2023 12:15

tustvold added the api change Changes the API exposed to users of the crate label Sep 18, 2023

Update arrow 47.0.0

c00628d

tustvold force-pushed the prepare-arrow-47 branch from 0a7a1f9 to c00628d Compare September 18, 2023 13:04

tustvold commented Sep 18, 2023

View reviewed changes

Downgrade prost substrait

48a47ef

tustvold force-pushed the prepare-arrow-47 branch from 2805834 to 48a47ef Compare September 18, 2023 13:28

tustvold added 3 commits September 18, 2023 14:58

Fix deprecations

c07d5d7

Prost deprecations

97e04d2

Update pbjson-build

b37c90b

tustvold mentioned this pull request Sep 18, 2023

Prepare arrow 47.0.0 apache/arrow-rs#4827

Merged

Merge remote-tracking branch 'upstream/main' into prepare-arrow-47

3b5c082

github-actions bot removed the substrait label Sep 18, 2023

tustvold added 3 commits September 18, 2023 16:20

Format

65061ed

Merge remote-tracking branch 'upstream/main' into prepare-arrow-47

16132fe

Remove CardinalityAwareRowConverter

935906b

This was referenced Sep 18, 2023

Fix like scalar null apache/arrow-rs#4832

Merged

Fix merge_dictionary_values in selection kernels apache/arrow-rs#4833

Merged

tustvold added 2 commits September 18, 2023 21:56

Further fixes

bf95a0b

Ignore spill tests

f93d255

tustvold force-pushed the prepare-arrow-47 branch from f48848d to f93d255 Compare September 19, 2023 10:02

tustvold added 3 commits September 22, 2023 11:32

Fix tests

485e180

Fix Clippy

6d84edf

Update pin

3f308d0

tustvold marked this pull request as ready for review September 22, 2023 13:31

alamb approved these changes Sep 22, 2023

View reviewed changes

alamb changed the title ~~Update arrow 47.0.0~~ Update arrow 47.0.0 in DataFusion Sep 25, 2023

alamb assigned tustvold Sep 25, 2023

Merge remote-tracking branch 'upstream/main' into prepare-arrow-47

c20945d

tustvold mentioned this pull request Sep 25, 2023

Materialize Dictionaries in Group Keys #7647

Open

tustvold added 2 commits September 25, 2023 16:45

Review feedback

334500e

Clippy

5102e5b

tustvold merged commit 5f38135 into apache:main Sep 25, 2023
21 checks passed

alamb mentioned this pull request Sep 27, 2023

CREATE TABLE DDL does not save correct schema, resulting in mismatched plan vs execution (record batch) schema #7636

Open

kazuyukitanimura mentioned this pull request Jan 8, 2024

Upgrade to object_store 0.9.0 and arrow 50.0.0 #8758

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update arrow 47.0.0 in DataFusion #7587

Update arrow 47.0.0 in DataFusion #7587

tustvold commented Sep 18, 2023 •

edited

Loading

tustvold Sep 18, 2023

alamb left a comment

alamb Sep 22, 2023

alamb Sep 22, 2023

alamb Sep 22, 2023

alamb Sep 22, 2023

tustvold Sep 22, 2023

tustvold Sep 25, 2023

sunchao commented Sep 22, 2023

tustvold commented Sep 22, 2023

-pub(crate) fn apply(
+///
+/// This is used to provide a single function implementation that works for all combinations of
+/// 2 `ColumnarValue` inputs, and maps the [`ColumnarValue`] abstraction of DataFusion to
+/// the [`Datum`]  abstraction of arrow-rs
+pub(crate) fn apply(

Update arrow 47.0.0 in DataFusion #7587

Update arrow 47.0.0 in DataFusion #7587

Conversation

tustvold commented Sep 18, 2023 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

tustvold Sep 18, 2023

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb Sep 22, 2023

Choose a reason for hiding this comment

alamb Sep 22, 2023

Choose a reason for hiding this comment

alamb Sep 22, 2023

Choose a reason for hiding this comment

alamb Sep 22, 2023

Choose a reason for hiding this comment

tustvold Sep 22, 2023

Choose a reason for hiding this comment

tustvold Sep 25, 2023

Choose a reason for hiding this comment

sunchao commented Sep 22, 2023

tustvold commented Sep 22, 2023

tustvold commented Sep 18, 2023 •

edited

Loading