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

[FEAT] connect: add modulus operator and withColumns support #3351

Merged
merged 1 commit into from
Dec 5, 2024

Conversation

andrewgazelka
Copy link
Member

@andrewgazelka andrewgazelka commented Nov 20, 2024

  • Add % operator and sum function to unresolved functions
  • Implement withColumns transformation
  • Add test coverage for group by with modulus operation

Copy link
Member Author

andrewgazelka commented Nov 20, 2024

@andrewgazelka andrewgazelka force-pushed the andrew/connect-binary-operators branch from 7f0c85d to 81d6894 Compare November 20, 2024 08:03
@andrewgazelka andrewgazelka force-pushed the andrew/connect-group-by branch from b7cdee2 to 7ee8125 Compare November 20, 2024 08:03
@andrewgazelka andrewgazelka marked this pull request as ready for review November 20, 2024 08:27
@andrewgazelka andrewgazelka force-pushed the andrew/connect-group-by branch from 7ee8125 to 0f17b20 Compare November 20, 2024 08:32
@andrewgazelka andrewgazelka force-pushed the andrew/connect-binary-operators branch from 81d6894 to 28e0edf Compare November 20, 2024 09:04
@andrewgazelka andrewgazelka force-pushed the andrew/connect-group-by branch from 0f17b20 to bc80ee7 Compare November 20, 2024 09:04
@andrewgazelka andrewgazelka force-pushed the andrew/connect-binary-operators branch from 28e0edf to b399d76 Compare November 20, 2024 09:45
@andrewgazelka andrewgazelka force-pushed the andrew/connect-group-by branch from bc80ee7 to 128d640 Compare November 20, 2024 09:45
@andrewgazelka andrewgazelka force-pushed the andrew/connect-binary-operators branch from b399d76 to 0eb7089 Compare November 20, 2024 09:52
@andrewgazelka andrewgazelka force-pushed the andrew/connect-group-by branch 2 times, most recently from 0de1230 to c466dd5 Compare November 20, 2024 09:56
@andrewgazelka andrewgazelka force-pushed the andrew/connect-binary-operators branch from 0eb7089 to b09fc18 Compare November 20, 2024 11:19
@andrewgazelka andrewgazelka force-pushed the andrew/connect-group-by branch from c466dd5 to 6483686 Compare November 20, 2024 11:19
@andrewgazelka andrewgazelka force-pushed the andrew/connect-binary-operators branch from b09fc18 to 0849d25 Compare November 20, 2024 18:25
@andrewgazelka andrewgazelka force-pushed the andrew/connect-group-by branch from 6483686 to 6dd398b Compare November 20, 2024 18:25
@andrewgazelka andrewgazelka force-pushed the andrew/connect-binary-operators branch from 0849d25 to eb1c9af Compare November 20, 2024 18:32
@andrewgazelka andrewgazelka force-pushed the andrew/connect-group-by branch from 6dd398b to 553ba66 Compare November 20, 2024 18:32
@andrewgazelka andrewgazelka force-pushed the andrew/connect-binary-operators branch from eb1c9af to 4bf4495 Compare November 20, 2024 18:43
@andrewgazelka andrewgazelka force-pushed the andrew/connect-group-by branch from 553ba66 to b6dcec1 Compare November 20, 2024 18:43
@andrewgazelka andrewgazelka force-pushed the andrew/connect-binary-operators branch from 4bf4495 to 548c302 Compare November 20, 2024 18:48
@andrewgazelka andrewgazelka force-pushed the andrew/connect-group-by branch from b6dcec1 to 892ff4f Compare November 20, 2024 18:48
@andrewgazelka andrewgazelka force-pushed the andrew/connect-binary-operators branch from 548c302 to 632c80c Compare November 20, 2024 19:32
@andrewgazelka andrewgazelka force-pushed the andrew/connect-group-by branch from 892ff4f to 6663753 Compare November 20, 2024 19:32
@andrewgazelka andrewgazelka force-pushed the andrew/connect-binary-operators branch from 632c80c to 8ace42d Compare November 20, 2024 22:12
@andrewgazelka andrewgazelka force-pushed the andrew/connect-group-by branch from eddb120 to 6d4ad74 Compare November 21, 2024 16:57
@andrewgazelka andrewgazelka mentioned this pull request Nov 21, 2024
1 task
@andrewgazelka andrewgazelka force-pushed the andrew/connect-binary-operators branch from 28173ab to cdca2e2 Compare November 21, 2024 18:43
@andrewgazelka andrewgazelka force-pushed the andrew/connect-group-by branch from 6d4ad74 to 6020c6b Compare November 21, 2024 18:43
@andrewgazelka andrewgazelka force-pushed the andrew/connect-binary-operators branch from cdca2e2 to fbc5c6e Compare November 27, 2024 07:42
@andrewgazelka andrewgazelka force-pushed the andrew/connect-group-by branch from 6020c6b to f7f7e1e Compare November 27, 2024 07:56
@andrewgazelka andrewgazelka marked this pull request as draft November 27, 2024 21:05
@andrewgazelka andrewgazelka force-pushed the andrew/connect-binary-operators branch from fbc5c6e to a2e0beb Compare December 4, 2024 02:09
@andrewgazelka andrewgazelka force-pushed the andrew/connect-group-by branch from f7f7e1e to beb40fa Compare December 4, 2024 02:09
@andrewgazelka andrewgazelka force-pushed the andrew/connect-binary-operators branch 2 times, most recently from b9a7f70 to 3ee5757 Compare December 4, 2024 02:33
@andrewgazelka andrewgazelka force-pushed the andrew/connect-group-by branch 2 times, most recently from 42c53df to a11e646 Compare December 4, 2024 02:45
Base automatically changed from andrew/connect-binary-operators to main December 4, 2024 03:01
@github-actions github-actions bot added the enhancement New feature or request label Dec 4, 2024
@andrewgazelka andrewgazelka force-pushed the andrew/connect-group-by branch from a11e646 to ffcd80e Compare December 4, 2024 08:53
Copy link

codspeed-hq bot commented Dec 4, 2024

CodSpeed Performance Report

Merging #3351 will degrade performances by 41.02%

Comparing andrew/connect-group-by (3d691bb) with main (86523a0)

Summary

❌ 1 regressions
✅ 16 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main andrew/connect-group-by Change
test_iter_rows_first_row[100 Small Files] 113.9 ms 193 ms -41.02%

Copy link

codecov bot commented Dec 4, 2024

Codecov Report

Attention: Patch coverage is 90.62500% with 3 lines in your changes missing coverage. Please review.

Project coverage is 77.36%. Comparing base (86523a0) to head (3d691bb).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...onnect/src/translation/expr/unresolved_function.rs 81.81% 2 Missing ⚠️
...nnect/src/translation/logical_plan/with_columns.rs 94.73% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3351      +/-   ##
==========================================
+ Coverage   77.28%   77.36%   +0.07%     
==========================================
  Files         699      700       +1     
  Lines       85240    85274      +34     
==========================================
+ Hits        65879    65973      +94     
+ Misses      19361    19301      -60     
Files with missing lines Coverage Δ
src/daft-connect/src/translation/logical_plan.rs 87.87% <100.00%> (+7.23%) ⬆️
...nnect/src/translation/logical_plan/with_columns.rs 94.73% <94.73%> (ø)
...onnect/src/translation/expr/unresolved_function.rs 68.29% <81.81%> (+16.18%) ⬆️

... and 7 files with indirect coverage changes

@andrewgazelka andrewgazelka requested a review from jaychia December 4, 2024 09:24
@andrewgazelka andrewgazelka marked this pull request as ready for review December 4, 2024 09:24
@andrewgazelka andrewgazelka force-pushed the andrew/connect-group-by branch from ffcd80e to 4612738 Compare December 4, 2024 09:25
Copy link
Contributor

@jaychia jaychia left a comment

Choose a reason for hiding this comment

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

LGTM, probably change the PR messaging though to reflect that we're adding sum and with_columns. Looks like groupby itself already works.

tests/connect/test_group_by.py Outdated Show resolved Hide resolved
@andrewgazelka andrewgazelka force-pushed the andrew/connect-group-by branch from 4612738 to 0ffb86d Compare December 4, 2024 23:57
@andrewgazelka andrewgazelka changed the title [FEAT] connect: add groupBy [FEAT] connect: add modulus operator and withColumns support Dec 4, 2024
@andrewgazelka andrewgazelka force-pushed the andrew/connect-group-by branch from 0ffb86d to cfe413b Compare December 4, 2024 23:59
@andrewgazelka andrewgazelka enabled auto-merge (squash) December 4, 2024 23:59
@andrewgazelka andrewgazelka force-pushed the andrew/connect-group-by branch from cfe413b to 3d691bb Compare December 5, 2024 00:42
@andrewgazelka andrewgazelka merged commit 8de0101 into main Dec 5, 2024
44 of 45 checks passed
@andrewgazelka andrewgazelka deleted the andrew/connect-group-by branch December 5, 2024 01:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants