datafusion icon indicating copy to clipboard operation
datafusion copied to clipboard

Display: Support `preserve_partitioning` on SortExec physical plan.

Open kavirajk opened this issue 1 year ago • 2 comments

Which issue does this PR close?

Closes #10138

Rationale for this change

Display of SortExec execution plan doesn't have visibility on preserve_partitioning field

SortExec: TopK(fetch=10000), expr=[NASME@0 ASC,VISITS@1 DESC NULLS LAST]

What changes are included in this PR?

Only important change made in this PR is

Display of SortExec execution plan now support preserve_partitioning field

SortExec: TopK(fetch=10000), expr=[NASME@0 ASC,VISITS@1 DESC NULLS LAST], preserve_partitioning=[true] 

Everything else is either updating tests manually or via some script. Following are the steps I used to update tests automatically (thanks @alamb for the tip)

  1. cargo test --test sqllogictests -- --complete - that updates all .slt files except ones from tpch benchmarks
  2. Generate tpch benchmarks data using steps mentioned here.
  3. INCLUDE_TPCH=true cargo test --test sqllogictests -- --complete - to update tpch benchmarks tests outputs.

Are these changes tested?

Yes

Are there any user-facing changes?

May be, if user try to format the Sort execution plan.

kavirajk avatar Apr 20 '24 18:04 kavirajk

I think you can update the tests using completion mode: https://github.com/apache/datafusion/tree/main/datafusion/sqllogictest#updating-tests-completion-mode

That probably will get this PR mostly working

alamb avatar Apr 23 '24 22:04 alamb

@alamb thanks for the suggestion 👍 . Quick follow up. That command you mentioned sqllogictests --complete only updates the .slt files that are part of datafusion/sqllogictest crate itself.

I don't see it updates any of the other test cases where we are asserting the physical plan string representation. That still needs manual updates correct (around 79 tests were failing)?.

I started updating those, just wanted to check if there are any easy/automated way that I'm not aware of :)

kavirajk avatar Apr 28 '24 15:04 kavirajk

I took the liberty of merging up from main to this branch to ensure we didn't have any logical conflicts (e.g. newly added explain plans for example). I think we can merge it once CI passes

alamb avatar Apr 29 '24 19:04 alamb

Thanks again @kavirajk

alamb avatar Apr 30 '24 18:04 alamb