Upgrade to DataFusion 17.0.0 #998

andygrove · 2023-01-18T15:53:45Z

Changes in DataFusion that are causing tests to fail:

Window frame offsets can have different data types depending on the context but Dask SQL currently expects UInt64
Join conditions are no longer limited to Column but can be any Expr

andygrove · 2023-01-18T17:04:02Z

current failures:

FAILED tests/integration/test_compatibility.py::test_single_agg_count_no_group_by - ValueError: conflicting aggregation functions: [('count', 'a'), ('count', 'a')]
FAILED tests/integration/test_compatibility.py::test_multi_agg_count_no_group_by - ValueError: conflicting aggregation functions: [('count', 'a'), ('count', 'a')]
FAILED tests/integration/test_compatibility.py::test_agg_count_distinct_group_by - AssertionError: DataFrame.iloc[:, 1] (column name="cd_b") are different
FAILED tests/integration/test_compatibility.py::test_agg_count_distinct_no_group_by - AssertionError: DataFrame.iloc[:, 0] (column name="cd_a") are different
FAILED tests/integration/test_join.py::test_join_literal - pyo3_runtime.PanicException: assertion failed: !filters.is_empty()
FAILED tests/integration/test_join.py::test_join_on_unary_cond_only - pyo3_runtime.PanicException: assertion failed: !filters.is_empty()
FAILED tests/integration/test_join.py::test_intersect - TypeError: "unsupported join condition"

…datafusion-16

…ounds

…s other aggregates (columns followed by agg columns)

ayushdg · 2023-01-20T17:52:09Z

dask_planner/src/sql/logical/window.rs

+        match &self.frame_bound {
+            WindowFrameBound::Preceding(val) | WindowFrameBound::Following(val) => match val {
+                x if x.is_null() => Ok(None),
+                ScalarValue::UInt64(v) => Ok(*v),
+                ScalarValue::Int64(v) => Ok(v.map(|n| n as u64)),
+                ref x => Err(DaskPlannerError::Internal(format!(
+                    "Unexpected window frame bound: {:?}",
+                    x
+                ))
+                .into()),
+            },
+            // The below is only safe because window bounds cannot be negative


There's probably a better way of writing the same logic win rust. cc @andygrove if you have suggestions

… dnf based filter for some cases

charlesbluca · 2023-01-25T21:32:42Z

dask_sql/physical/rex/core/alias.py

+class RexAliasPlugin(BaseRexPlugin):
+    """
+    A RexAliasPlugin is an expression, which references a Subquery.
+    This plugin is thin on logic, however keeping with previous patterns
+    we use the plugin approach instead of placing the logic inline
+    """
+
+    class_name = "RexAlias"
+
+    def convert(
+        self,
+        rel: "LogicalPlan",
+        rex: "Expression",
+        dc: DataContainer,
+        context: "dask_sql.Context",
+    ) -> Union[dd.Series, Any]:
+        # extract the operands; there should only be a single underlying Expression
+        operands = rex.getOperands()
+        assert len(operands) == 1
+
+        sub_rex = operands[0]
+
+        value = RexConverter.convert(rel, sub_rex, dc, context=context)
+
+        if isinstance(value, DataContainer):
+            return value.df
+
+        return value


Added this to resolve the query failures coming from our lack of handling for rex aliases; for now I've kept this separate from the relatively similar RexScalarSubqueryPlugin, not sure if there's a case to have these merged into one plugin that then conditionally grabs and converts the sub-rel/rex

…tafusion-16

codecov-commenter · 2023-01-26T14:44:25Z

Codecov Report

Merging #998 (4cadf93) into main (4f7e7f3) will increase coverage by 0.06%.
The diff coverage is 84.37%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main     #998      +/-   ##
==========================================
+ Coverage   81.09%   81.16%   +0.06%     
==========================================
  Files          77       78       +1     
  Lines        4338     4358      +20     
  Branches      779      782       +3     
==========================================
+ Hits         3518     3537      +19     
+ Misses        648      643       -5     
- Partials      172      178       +6

Impacted Files	Coverage Δ
dask_sql/physical/rex/convert.py	`88.00% <ø> (ø)`
dask_sql/physical/rex/core/alias.py	`72.22% <72.22%> (ø)`
dask_sql/context.py	`94.23% <100.00%> (-1.22%)`	⬇️
dask_sql/physical/rel/logical/join.py	`96.03% <100.00%> (+14.58%)`	⬆️
dask_sql/physical/rel/logical/window.py	`95.32% <100.00%> (-1.79%)`	⬇️
dask_sql/physical/rex/core/__init__.py	`100.00% <100.00%> (ø)`
dask_sql/physical/rex/core/subquery.py	`57.14% <100.00%> (ø)`
dask_sql/physical/rex/core/literal.py	`58.09% <0.00%> (-2.86%)`	⬇️
... and 3 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

…tafusion-16

…in temporarily

jdye64

LGTM. This was a long haul of a PR

.github/workflows/conda.yml

continuous_integration/recipe/meta.yaml

dask_planner/src/sql/optimizer.rs

charlesbluca

Thanks @andygrove and @jdye64 for taking the time to unblock CI 🎉 couple of comments/questions but things mostly look good:

.github/workflows/conda.yml

continuous_integration/recipe/meta.yaml

charlesbluca · 2023-02-13T14:27:02Z

tests/integration/test_filter.py

-            [
-                [("a", "==", 1), ("b", "<", 10)],
-                [("a", "==", 1), ("b", ">", 5)],
-                [("b", ">", 5), ("b", "<", 10)],
-                [("a", "==", 1)],
-            ],
+            [[("b", ">", 5), ("b", "<", 10)], [("a", "==", 1)]],


Just checking; are these param changes related to disabling FilterColumnsPostJoin?

I don't know why those changed? I don't think it was me that changed them unless I just don't remember. It should not be related to FilterColumnsPostJoin however.

I will say the new change seems more correct than the original way anyway.

I can comment a bit more on this. This isn't related to FilterColumnsPostJoin, but how datafusion optimizes predicates for filters/table_scan operations. Starting df14, they started using cnf (conjunctive normal form) for the expressions being passed as filters. (See #903 (comment)).
Since this particular test already had filters in dnf, the conversion of dnf->cnf (by df) -> dnf (by dask-sql since that's what arrow expects) resulted in an overly complex filter.

df16/17 seems to simplify some of these expressions and doesn't convert everything to cnf.

ayushdg

Generally lgtm! Just minor comments/questions that should be a quick fix

dask_planner/Cargo.toml

ayushdg · 2023-02-13T18:31:18Z

dask_planner/src/sql/optimizer.rs

    optimizer: Optimizer,
 }

 impl DaskSqlOptimizer {
    /// Creates a new instance of the DaskSqlOptimizer with all the DataFusion desired
    /// optimizers as well as any custom `OptimizerRule` trait impls that might be desired.
-    pub fn new(skip_failing_rules: bool) -> Self {
+    pub fn new() -> Self {


Curious why we removed this

That configuration value is no longer exposed via datafusion therefore this was "dead_code" and the compiler issuing warnings and cargo clippy was failing

Ah they might have renamed that one to skip_failed_rules. https://docs.rs/datafusion-common/17.0.0/datafusion_common/config/struct.OptimizerOptions.html

We can update in a followup if needed.

tests/integration/test_join.py

jdye64 · 2023-02-13T19:50:15Z

@ayushdg how do the changes suit you?

andygrove added 3 commits January 18, 2023 08:53

Upgrade to DataFusion 16.0.0

e142cd2

fix

0d038b2

fix todo

f03818d

ayushdg added 4 commits January 20, 2023 23:04

Merge branch 'main' of https://github.com/dask-contrib/dask-sql into …

c5f06b4

…datafusion-16

Fix clippy style errors

a0fbcbf

Update getOffset and isUnbounded to better handle ScalarValue windowB…

711efb3

…ounds

Window Plan: Fix field name order handling since the order now matche…

3e97e09

…s other aggregates (columns followed by agg columns)

ayushdg reviewed Jan 20, 2023

View reviewed changes

ayushdg added 2 commits January 20, 2023 23:23

Fix comment location

06c4951

Predicate Filters: Df16 reverts changes in df15 and returns a simpler…

b4e8b0a

… dnf based filter for some cases

charlesbluca mentioned this pull request Jan 23, 2023

Remove EliminateAggDistinct optimizer rule, implement distinct aggs in Python #1006

Merged

jdye64 and others added 10 commits January 24, 2023 09:08

testing

c3c526b

Merge remote-tracking branch 'origin/main' into datafusion-16

eced31c

bump datafusion -> 16.1.0

68cf931

merge update

64f6338

Fix 2 failing join pytests

8e01ac5

cargo fmt --all

b6e3146

Ok really fix cargo fmt this time

9c2923e

Update join.rs to accept CAST as a JOIN condition type

3e7acc8

Merge remote-tracking branch 'origin/main' into datafusion-16

dc57fb9

Add explicit handling for RexAlias

61857bc

charlesbluca reviewed Jan 25, 2023

View reviewed changes

jdye64 added 2 commits January 26, 2023 09:23

merge with upstream/main

345449d

Merge branch 'datafusion-16' of github.com:andygrove/dask-sql into da…

1cf9b5d

…tafusion-16

sarahyurick mentioned this pull request Jan 26, 2023

[DF] Support CREATE TABLE x AS TABLE y #771

Open

Bump DataFusion -> 17.0.0

2af8d66

ayushdg mentioned this pull request Feb 1, 2023

[BUG] projection_push_down optimizer rule fails in some queries #1012

Closed

jdye64 changed the title ~~Upgrade to DataFusion 16.0.0~~ Upgrade to DataFusion 17.0.0 Feb 9, 2023

charlesbluca and others added 5 commits February 9, 2023 08:22

Fix clippy warnings

1c61577

Add protoc action to workflows for rust and style

cfc3c21

use arrow re-exported from datafusion

62eee30

Add protoc action tot conda.yml

2987e7c

Add protobuf install to import / conda build testing

d1949fc

charlesbluca mentioned this pull request Feb 9, 2023

Install rustfmt in dask-sql images rapidsai/dask-build-environment#56

Merged

jdye64 and others added 10 commits February 9, 2023 16:11

Enable RUST_BACKTRACE=1

bd92280

Add RUST_BACKTRACE=1 to meta.yaml

955e53b

Add Rust Toolchain action to conda.yml

5718d6d

Merge branch 'datafusion-16' of github.com:andygrove/dask-sql into da…

4877e5e

…tafusion-16

Bump setuptools-rust version to 1.5.2 in hopes that helps

f224ab0

Enable RUST_BACKTRACE=full

ea5f5b8

Remove substrait package for now

2fae72a

Updates for BinaryExpr in join.rs and comment out FilterColumnsPostJo…

e324a79

…in temporarily

Remove protoc github action

7503e38

Disable pytest that was using a disabled optimizer rule

41ab6d1

jdye64 marked this pull request as ready for review February 12, 2023 21:58

jdye64 requested review from galipremsagar and jdye64 as code owners February 12, 2023 21:58

jdye64 approved these changes Feb 13, 2023

View reviewed changes

.github/workflows/conda.yml Outdated Show resolved Hide resolved

continuous_integration/recipe/meta.yaml Show resolved Hide resolved

dask_planner/src/sql/optimizer.rs Show resolved Hide resolved

charlesbluca reviewed Feb 13, 2023

View reviewed changes

charlesbluca and others added 3 commits February 13, 2023 07:06

Add libprotobuf=3 dependency

477345d

Remove libprotobuf dep

14b2537

Remove Rust action from conda.yml

3c68b53

ayushdg requested changes Feb 13, 2023

View reviewed changes

Address review

4cadf93

ayushdg approved these changes Feb 13, 2023

View reviewed changes

charlesbluca approved these changes Feb 13, 2023

View reviewed changes

jdye64 merged commit f895346 into dask-contrib:main Feb 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to DataFusion 17.0.0 #998

Upgrade to DataFusion 17.0.0 #998

andygrove commented Jan 18, 2023 •

edited

andygrove commented Jan 18, 2023 •

edited by ayushdg

ayushdg Jan 20, 2023 •

edited

charlesbluca Jan 25, 2023

codecov-commenter commented Jan 26, 2023 •

edited

jdye64 left a comment

charlesbluca left a comment •

edited

charlesbluca Feb 13, 2023

jdye64 Feb 13, 2023

jdye64 Feb 13, 2023

ayushdg Feb 13, 2023 •

edited

ayushdg left a comment

ayushdg Feb 13, 2023

jdye64 Feb 13, 2023

ayushdg Feb 13, 2023

jdye64 commented Feb 13, 2023

Upgrade to DataFusion 17.0.0 #998

Upgrade to DataFusion 17.0.0 #998

Conversation

andygrove commented Jan 18, 2023 • edited

andygrove commented Jan 18, 2023 • edited by ayushdg

ayushdg Jan 20, 2023 • edited

Choose a reason for hiding this comment

charlesbluca Jan 25, 2023

Choose a reason for hiding this comment

codecov-commenter commented Jan 26, 2023 • edited

Codecov Report

jdye64 left a comment

Choose a reason for hiding this comment

charlesbluca left a comment • edited

Choose a reason for hiding this comment

charlesbluca Feb 13, 2023

Choose a reason for hiding this comment

jdye64 Feb 13, 2023

Choose a reason for hiding this comment

jdye64 Feb 13, 2023

Choose a reason for hiding this comment

ayushdg Feb 13, 2023 • edited

Choose a reason for hiding this comment

ayushdg left a comment

Choose a reason for hiding this comment

ayushdg Feb 13, 2023

Choose a reason for hiding this comment

jdye64 Feb 13, 2023

Choose a reason for hiding this comment

ayushdg Feb 13, 2023

Choose a reason for hiding this comment

jdye64 commented Feb 13, 2023

andygrove commented Jan 18, 2023 •

edited

andygrove commented Jan 18, 2023 •

edited by ayushdg

ayushdg Jan 20, 2023 •

edited

codecov-commenter commented Jan 26, 2023 •

edited

charlesbluca left a comment •

edited

ayushdg Feb 13, 2023 •

edited