Skip to content

ARROW-11369: [DataFusion] Split physical_plan/expressions.rs - #9312

Closed
jorgecarleitao wants to merge 16 commits into
apache:masterfrom
jorgecarleitao:refactor2
Closed

ARROW-11369: [DataFusion] Split physical_plan/expressions.rs#9312
jorgecarleitao wants to merge 16 commits into
apache:masterfrom
jorgecarleitao:refactor2

Conversation

@jorgecarleitao

@jorgecarleitaojorgecarleitao commented Jan 25, 2021

Copy link
Copy Markdown
Member

This PR splits the +4k LOC expressions.rs module in modules, one per expression, thereby making it easier to navigate and work with.

No backward incompatible changes are expected from this.

Each commit is an independent expression / logical change.

@github-actions

Copy link
Copy Markdown

@codecov-io

codecov-io commented Jan 25, 2021

Copy link
Copy Markdown

Codecov Report

Merging #9312 (446d5ad) into master (437c8c9) will increase coverage by 0.00%.
The diff coverage is 81.25%.

Impacted file tree graph

@@ Coverage Diff @@## master #9312 +/- ##
=======================================
Coverage 81.88% 81.89% =======================================
Files 215 230 +15 Lines 52988 53004 +16 =======================================
+ Hits 43391 43407 +16 
Misses 9597 9597 
Impacted FilesCoverage Δ
...tafusion/src/physical_plan/expressions/negative.rs42.85% <42.85%> (ø)
...datafusion/src/physical_plan/expressions/column.rs70.00% <70.00%> (ø)
...st/datafusion/src/physical_plan/expressions/not.rs70.83% <70.83%> (ø)
...usion/src/physical_plan/expressions/is_not_null.rs72.41% <72.41%> (ø)
...atafusion/src/physical_plan/expressions/is_null.rs72.41% <72.41%> (ø)
...t/datafusion/src/physical_plan/expressions/case.rs73.29% <73.29%> (ø)
...atafusion/src/physical_plan/expressions/in_list.rs75.37% <75.37%> (ø)
...st/datafusion/src/physical_plan/expressions/sum.rs78.28% <78.28%> (ø)
...tafusion/src/physical_plan/expressions/coercion.rs78.65% <78.65%> (ø)
...atafusion/src/physical_plan/expressions/average.rs83.78% <83.78%> (ø)
... and 22 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 437c8c9...a2d5b8e. Read the comment docs.

@jorgecarleitao

Copy link
Copy Markdown
MemberAuthor

The clippy error is unrelated.

@alambalamb changed the title ARROW-11369: [DataFusion] Split expressions.rsARROW-11369: [DataFusion] Split physical_plan/expressions.rsJan 25, 2021

@alambalamb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @jorgecarleitao -- this is great. ❤️

I went through the PR commit by commit spot checked things -- everything looked good to me though clearly I did not go through all the changes in detail.

FYI @seddonm1 in case this conflicts with work you are working on

@alamb

Copy link
Copy Markdown
Contributor

Clippy error is addressed here: #9314

@seddonm1

Copy link
Copy Markdown
Contributor

Thanks @alamb and @jorgecarleitao . Good idea. I'll rebase and apply these changes. I think the next work will be to address the signatures so I'll be starting work again tomorrow.

@andygroveandygrove 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.

Thanks a lot for doing this @jorgecarleitao

@jorgecarleitao
jorgecarleitao deleted the refactor2 branch January 30, 2021 06:20
@alamb

Copy link
Copy Markdown
Contributor

🎉

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@jorgecarleitao@codecov-io@alamb@seddonm1@andygrove