-
Notifications
You must be signed in to change notification settings - Fork 68
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
refactor: split up arrow_expression
module
#750
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NOTE: Github did not detect that the code was moved. So I manually diffed each new file against the original to detect any changes made during code movement, e.g.:
git diff -b maruschin/split_up_arrow_expression~1:kernel/src/engine/arrow_expression.rs maruschin/split_up_arrow_expression:kernel/src/engine/arrow_expression/tests.rs
Everything looks good except a few private functions that should be changed to pub(crate)
instead of pub
.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #750 +/- ##
==========================================
- Coverage 84.39% 84.38% -0.01%
==========================================
Files 77 80 +3
Lines 19226 19222 -4
Branches 19226 19222 -4
==========================================
- Hits 16225 16220 -5
- Misses 2198 2200 +2
+ Partials 803 802 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome thanks! Do change the visibility things scovich flagged, and maybe write a bit more of a description (as that's what will end up in the git history).
Otherwise looks great, thanks.
f3280ec
to
56a882b
Compare
Move tests and utility functions to separate files for better organization and maintainability.
56a882b
to
a31a539
Compare
Corrected all comments. |
arrow_expression
module
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm! Thanks for your contribution
What changes are proposed in this pull request?
Split arrow_expression.rs into a dedicated submodule.
Move tests and utility functions to separate files for better organization and maintainability.
This solves the issue 745.
How was this change tested?
existing UT