Skip to content

ARROW-11220: [Rust] Implement GROUP BY support for Boolean - #9174

Closed
ovr wants to merge 1 commit into
apache:masterfrom
ovr:issue-11220
Closed

ARROW-11220: [Rust] Implement GROUP BY support for Boolean#9174
ovr wants to merge 1 commit into
apache:masterfrom
ovr:issue-11220

Conversation

@ovr

@ovrovr commented Jan 12, 2021

Copy link
Copy Markdown
Contributor

Introduce support in DataFushion for GROUP BY on boolean values. Boolean type in Rust implements Eq and Hash traits which allow us to use GroupByScalar.

@github-actions

github-actionsBot commented Jan 12, 2021

Copy link
Copy Markdown

https://issues.apache.org/jira/browse/ARROW-11220

andygrove
andygrove approved these changes Jan 12, 2021
View reviewed changes

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

LGTM

Dandandan
Dandandan reviewed Jan 12, 2021
View reviewed changes
Comment threadrust/datafusion/src/physical_plan/hash_join.rs Outdated

@DandandanDandandanJan 12, 2021

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.

This could probably also use array.value(row) as u8?

@ovrovrJan 16, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Thanks

Dandandan
Dandandan approved these changes Jan 12, 2021
View reviewed changes

@DandandanDandandan left a comment •
edited
Loading

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.

LGTM. I added one style comment. Thanks @ovr !

alamb
alamb reviewed Jan 13, 2021
View reviewed changes

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

I suggest adding an end-to-end sql test in https://github.com/apache/arrow/blob/master/rust/datafusion/tests/sql.rs to make sure the plumbing is all hooked up correctly

@alamb

alamb commented Jan 15, 2021

Copy link
Copy Markdown
Contributor

I think this PR needs a rebase and perhaps an end-to-end test for grouping (as you did on #9175) and it will be ready to go. Thanks again @ovr !

@ovr
ovrforce-pushed the issue-11220 branch 3 times, most recently from 9eb40b5 to 3db6560CompareJanuary 16, 2021 13:33
@ovr

ovr commented Jan 16, 2021

Copy link
Copy Markdown
ContributorAuthor

@alamb Rebased, I added test. Thanks

@codecov-io

codecov-io commented Jan 16, 2021•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #9174 (3db6560) into master (1393188) will decrease coverage by 0.00%.
The diff coverage is 85.71%.

Impacted file tree graph

@@ Coverage Diff @@## master #9174 +/- ##
==========================================
- Coverage 81.61% 81.60% -0.01% 
==========================================
Files 215 215 Lines 51867 51885 +18 ==========================================
+ Hits 42329 42343 +14 - Misses 9538 9542 +4
Impacted FilesCoverage Δ
rust/datafusion/src/physical_plan/group_scalar.rs68.00% <0.00%> (-2.84%)⬇️
...ust/datafusion/src/physical_plan/hash_aggregate.rs84.91% <100.00%> (+0.11%)⬆️
rust/datafusion/src/physical_plan/hash_join.rs84.78% <100.00%> (+0.07%)⬆️
rust/datafusion/tests/sql.rs99.84% <100.00%> (+<0.01%)⬆️
rust/parquet/src/encodings/encoding.rs94.86% <0.00%> (-0.20%)⬇️

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 eaa7b7a...3db6560. Read the comment docs.

alamb
alamb approved these changes Jan 17, 2021
View reviewed changes

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

Looks great -- thanks @ovr

@alamb

alamb commented Jan 17, 2021

Copy link
Copy Markdown
Contributor

I apologize for the delay in merging Rust PRs -- the 3.0 release is being finalized now and are planning to minimize entropy by postponing merging changes not critical for the release until the process was complete. I hope the process is complete in the next few days. There is more discussion in the mailing list

@jorgecarleitaojorgecarleitao added the needs-rebase A PR that needs to be rebased by the author label Jan 20, 2021
@alambalamb closed this in 8b56f85Jan 21, 2021
@alamb

alamb commented Jan 21, 2021

Copy link
Copy Markdown
Contributor

Thanks again for the contribution @ovr

kszucs pushed a commit that referenced this pull request Jan 25, 2021
@ovr@kszucs
ARROW-11220: [Rust] Implement GROUP BY support for Boolean
0c3eb74
Introduce support in DataFushion for GROUP BY on boolean values. Boolean type in Rust implements Eq and Hash traits which allow us to use GroupByScalar.
Closes#9174 from ovr/issue-11220
Authored-by: Dmitry Patsura <zaets28rus@gmail.com>
Signed-off-by: Andrew Lamb <andrew@nerdnetworks.org>
@asfimportasfimport mentioned this pull request Jan 22, 2021
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DandandanDandandanDandandan approved these changes
@alambalambalamb approved these changes
@andygroveandygroveandygrove approved these changes

Assignees

No one assigned

Labels

Component: Rust - DataFusionComponent: Rustneeds-rebaseA PR that needs to be rebased by the author

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@ovr@alamb@codecov-io@Dandandan@andygrove@jorgecarleitao