Skip to content

Upgrade DataFusion to version 56 - #1826

Open
nuno-faria wants to merge 3 commits into
apache:mainfrom
nuno-faria:upgrade_datafusion_56
Open

nuno-faria wants to merge 3 commits into
apache:mainfrom
nuno-faria:upgrade_datafusion_56

Conversation

@nuno-faria

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

N/A.

Rationale for this change

A new DataFusion version is being released: apache/datafusion#24461

What changes are included in this PR?

There are new types like Show (which replaced DescribeTable), AsOfJoin, and MaterializedCte, as well as some other minor changes. I will add them as comments.

Are there any user-facing changes?

New DF version. Other than that the main breaking change is that DescribeTable has been removed. I added a note to the upgrade guide.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}

/// Retrieves the right input `LogicalPlan` to this `Join` node
/// Retrieves the equality keys of this `Join` node

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noticed when creating the ASOF join that the prev comment was wrong.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

&self,
_ctx: &dyn Session,
projection: Option<&Vec<usize>>,
projection: Option<&[usize]>,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines 5352 to +5388
@@ -5385,7 +5385,7 @@ def map_entries(map: Expr) -> Expr:
def element_at(map: Expr, key: Expr) -> Expr:
"""Returns the value for a given key in the map.

Returns ``[None]`` if the key is absent.
Returns ``[]`` if the key is absent.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


def test_sql_with_options_no_statements(ctx):
sql = "SET time zone = 1;"
sql = "SET time zone = 'UTC';"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Timezone is now validated.

Comment on lines 1792 to +1814
@@ -1811,7 +1811,7 @@ def test_read_csv_with_options(tmp_path, as_read, global_ctx):
[
pa.array(["Alice", "Bob", "Phil", "Karin"]),
pa.array([30.0, 25.0, 75.0, 50.0]),
pa.array(["New York; NY", None, "Detroit' MI", "Stockholm\nSweden"]),
pa.array(["New York; NY", "", "Detroit' MI", "Stockholm\nSweden"]),

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This now appears as an empty string since it does not match the null_regex. From apache/datafusion#25254

Comment on lines +2722 to +2724
with pytest.raises(
Exception, match=r"Invalid compression level \d+ for \w+\. Expected a level"
):

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nuno-faria

Copy link
Copy Markdown
Contributor Author

@timsaucer I think the problem with the _test_three_library_query_planner comes from the FFI crate upstream:

Exception: DataFusion error: FFI error: SanityCheckPlan: the plan produced by physical optimizer
rule 'EnsureRequirements' does not satisfy the order and distribution requirements

caused by

Error during planning:

Plan:
  AggregateExec: mode=FinalPartitioned, gby=[numbers.A % Int64(2)@0 as numbers.A % Int64(2)], aggr=[count(Int64(1))]
    FFI_ExecutionPlan: RepartitionExec, number_of_children=1
      AggregateExec: mode=Partial, gby=[CAST(A@0 AS Int64) % 2 as numbers.A % Int64(2)], aggr=[count(Int64(1))]
        FFI_ExecutionPlan: DataSourceExec, number_of_children=0

does not satisfy distribution requirements: KeyPartitioned[[numbers.A % Int64(2)@0]])

Child-0 output partitioning: RoundRobinBatch(4)

It appears that the problem is now shown by apache/datafusion#26461.

@andygrove

Copy link
Copy Markdown
Member

@nuno-faria you were right that the _test_three_library_query_planner failure comes from upstream. I filed apache/datafusion#27427 and opened a fix in apache/datafusion#27428.

FFI_ExecutionPlan never carried a plan's input requirements: input distribution, required ordering, maintains_input_order. When the host's optimizer rules run across FFI on the planner library's plan, EnsureRequirements can't see that FinalPartitioned needs hash-partitioned input, so it adds only a round-robin repartition. On 55, SanityCheckPlan also ran as a host-side rule and was just as blind, so the test passed. apache/datafusion#26461 moved the check into DefaultPhysicalPlanner, so 56 rejects the plan.

I cherry-picked the fix onto the branch-56 commit this PR pins (0f3b5a08) and built this branch against it. The query-planner example suite passes (51/51, including test_spawning_plan_across_three_libraries), and so does the FFI example suite (59/59). Once the fix lands on branch-56 and the pin here is bumped, this test should pass without changes on our side.

@andygrove andygrove left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an AI generated review. Feel free to ignore any points that you do not agree with.

Blocking

  1. test_spawning_plan_across_three_libraries fails because of an upstream FFI bug. FFI_ExecutionPlan doesn't carry input requirements across the boundary. Tracked in apache/datafusion#27427, with a fix in apache/datafusion#27428. With the fix applied to branch-56, this PR passes all FFI example tests (51/51 and 59/59).
  2. CI's TPC-H step will fail next. tpchgen-cli 4.0 removed --format. Here the step was skipped only because the FFI step failed first. #1832 fixes it, and this PR will need to rebase onto it.
  3. Dependency source. [patch.crates-io] points at a moving git branch-56, and the comment above it says to remove it before a release candidate. For now it could bump to the current branch-56 tip (cce6374e6, which includes the 56.0.0 version bump). I ran the full suite against that tip with the FFI fix applied, and it is clean: main suite with doctests, both FFI example suites, TPC-H 22/22 and clippy. Once 56.0.0 is on crates.io it should switch to that.

Should fix

  1. Show.schema() returns None for DESCRIBE <table> (crates/core/src/expr/show.rs). Previously DescribeTable.schema() returned the table's schema in this case. Part of this comes from upstream: ShowKind::Columns now holds only table_ref and detail, with no schema. But the binding exposes no table_ref and none of the other kinds' payloads (variable name, function filter, etc.), so users can't get the table back. The upgrade guide shows kind() == "columns" but doesn't mention that schema() is now None. It could suggest ctx.table(name).schema() instead.
  2. Some user-visible behavior changes are missing from upgrade-guides.md. The tests and docstrings were updated for these, but the guide wasn't:
    • map_extract / element_at with a missing key now returns [] instead of [None].
    • CSV null_regex now also applies to the data, not only to schema inference (apache/datafusion#25254). Fields that don't match the regex, including the empty fields of truncated rows, come back as "" instead of null. This looks like intended upstream behavior and the test update is correct, but it is worth a note.
    • Time zone values in SET are now validated.
  3. The new bindings aren't covered from Python. test_asof_join and test_materialized_cte only compare display_indent() output, which is upstream's Rust Display. No test reaches AsOfJoin, MaterializedCte, MaterializedCteScan, CreateExternalCatalog or DropCatalog through to_variant() or their accessors (match_condition(), on(), etc.). A wrong match arm or a broken accessor in PyLogicalPlan::to_variant would still pass.
  4. crates/core/src/expr/describe_table.rs is orphaned. Its mod line was removed, but the file is still tracked and still references the removed upstream DescribeTable. It can be deleted.
  5. MSRV. The workspace has rust-version = "1.88", but DataFusion 56 declares rust-version = "1.95.0" (and rust-toolchain.toml is now 1.99.0). It should be raised to at least 1.95.

Minor

  1. The Python pin also applies to Windows. In .github/workflows/build.yml, uv sync --python 3.14 is in the job whose matrix is os: [macos-latest, windows-latest], so the Windows abi3 build is pinned too, while the commit message says macOS only. The pin is also repeated in two jobs.
  2. Show(schema, output_schema) can only build ShowKind::Describe, and kind() returns bare strings. That's fine for now, but a docstring note listing the possible kind() values would help users who match on them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants