Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support modulus op #510

Closed
wants to merge 26 commits into from
Closed

Support modulus op #510

wants to merge 26 commits into from

Conversation

gangliao
Copy link
Contributor

@gangliao gangliao commented Jun 5, 2021

Which issue does this PR close?

apache/arrow-rs#98
#99
apache/arrow-rs#317

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

no

@gangliao
Copy link
Contributor Author

gangliao commented Jun 5, 2021

@alamb Is that possible to let datafusion use the latest code from arrow-rs?

@alamb
Copy link
Contributor

alamb commented Jun 5, 2021

Thank you for your contribution @gangliao !

@alamb Is that possible to let datafusion use the latest code from arrow-rs?

As of now, we are only using "released" versions of arrow.rs.

Your change in apache/arrow-rs#317 is currently slated to be released in 4.3.0 (eta ~ 10 or so days) at which time it will be available for datafusion to use without any additional action

Until 4.3.0 is released, for your branch you can point datafusion to a sha from the arrow github to make sure the code works (e.g. how it was done prior to #395)

Basically we should wait until 4.3.0 is released and then this PR can be merged into datafusion

@gangliao
Copy link
Contributor Author

gangliao commented Jun 5, 2021

Got it! Thank you for the clarification.

msathis and others added 11 commits June 5, 2021 23:34
Co-authored-by: Sathis Kumar <sathis.kumar@udemy.com>
)

* refactor sort exec stream and combine batches

* refactor async function
* 110 support group by positions

* try resolve positions via array, not map

* Add comment for i64 and simplify the pattern match

* combine match and if condition, add more test cases

* replace '0 as i64' with 0_i64
* Fix aggregate fn with invalid column

* Fix error message

* Fix error message

* fix clippy

* fix message and test

* avoid unwrap in test_aggregation_with_bad_arguments

* Update datafusion/tests/sql.rs

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

* Fix test_aggregation_with_bad_arguments

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
* Use pytest

* Formatting

* Update GHA conf

* Remove TODO note

* Format

* Test requirements file

* Update workflow file

* Merge requirements file

* Update workflow file
@alamb alamb added the datafusion Changes in the datafusion crate label Jun 10, 2021
jimexist and others added 12 commits June 10, 2021 10:55
…pache#532)

* use logical planner in ballista building

* simplify statement

* fix unit test

* fix per comment
…he#541)

* ShuffleReaderExec now supports multiple locations per partition

* Remove TODO

* avoid clone
* refactor hash aggregates

* remove stale comments
* turn on clippy rule for needless borrow

* do a format round

* use warn not deny
* Cleanup RepartitionExec code

* cleanup metric handling

* Add elapsed_nanos
* support table alias in join clause

* Update datafusion/src/sql/planner.rs

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
@alamb
Copy link
Contributor

alamb commented Jun 14, 2021

Arrow 4.3.0 has been released so if you rebase this PR it will likely be ready to go

@codecov-commenter
Copy link

Codecov Report

Merging #510 (f63c8fe) into master (a9d04ca) will increase coverage by 0.03%.
The diff coverage is 72.05%.

❗ Current head f63c8fe differs from pull request most recent head 73902b6. Consider uploading reports for the commit 73902b6 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #510      +/-   ##
==========================================
+ Coverage   76.07%   76.11%   +0.03%     
==========================================
  Files         155      156       +1     
  Lines       26544    27061     +517     
==========================================
+ Hits        20194    20597     +403     
- Misses       6350     6464     +114     
Impacted Files Coverage Δ
ballista/rust/client/src/context.rs 0.00% <0.00%> (ø)
...ta/rust/core/src/execution_plans/shuffle_reader.rs 0.00% <0.00%> (ø)
...sta/rust/core/src/serde/logical_plan/from_proto.rs 35.04% <0.00%> (-0.88%) ⬇️
...lista/rust/core/src/serde/logical_plan/to_proto.rs 61.41% <0.00%> (-1.00%) ⬇️
...ista/rust/core/src/serde/physical_plan/to_proto.rs 49.38% <0.00%> (-0.93%) ⬇️
ballista/rust/core/src/utils.rs 25.53% <0.00%> (-2.06%) ⬇️
ballista/rust/executor/src/flight_service.rs 0.00% <ø> (ø)
ballista/rust/scheduler/src/planner.rs 66.91% <ø> (ø)
ballista/rust/scheduler/src/state/mod.rs 70.49% <0.00%> (ø)
datafusion-cli/src/lib.rs 0.00% <0.00%> (ø)
... and 44 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 a9d04ca...73902b6. Read the comment docs.

@alamb
Copy link
Contributor

alamb commented Jun 16, 2021

I am not sure what has happened to this PR but the diff looks not right:
Screen Shot 2021-06-16 at 12 38 29 PM

Where I would expect to only see the changes from 97fd9ff

@gangliao
Copy link
Contributor Author

Yeah. Looks strange. I will resubmit a new PR tonight.

@gangliao
Copy link
Contributor Author

#577

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
datafusion Changes in the datafusion crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.