-
Notifications
You must be signed in to change notification settings - Fork 246
feat: implement_comet_native_lpad_expr #2102
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
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.
Thanks @coderfender This function might be a good candidate for https://github.com/apache/datafusion/tree/main/datafusion/spark/src/function
|
@mbutrovich , Once the PR #2099 is merged, I plan to change code in this branch to leverage the same class since most of the code for rpad could be leveraged to support lpad operation as well |
a586a57 to
d76d530
Compare
|
@comphead , @mbutrovich I leveraged #2099 to implement |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2102 +/- ##
============================================
+ Coverage 56.12% 58.56% +2.43%
- Complexity 976 1445 +469
============================================
Files 119 146 +27
Lines 11743 13541 +1798
Branches 2251 2356 +105
============================================
+ Hits 6591 7930 +1339
- Misses 4012 4379 +367
- Partials 1140 1232 +92 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
native/spark-expr/src/static_invoke/char_varchar_utils/read_side_padding.rs
Outdated
Show resolved
Hide resolved
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.
Thanks @coderfender please add also tests from
https://spark.apache.org/docs/latest/api/sql/#lpad
d76d530 to
bc062bb
Compare
|
@comphead Changes are ready for review (please rerun the failed test caused due to a transient error) |
native/spark-expr/src/static_invoke/char_varchar_utils/read_side_padding.rs
Show resolved
Hide resolved
native/spark-expr/src/static_invoke/char_varchar_utils/read_side_padding.rs
Show resolved
Hide resolved
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.
THanks @coderfender I think we very close
|
Thank you @comphead . Let me push fixes right away and re run CI |
7d602f4 to
91ec7ee
Compare
91ec7ee to
40cb4fe
Compare
|
@comphead thank you for kicking off the CI . Please review the changes whenever you get a chance |
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.
Thanks @coderfender I think this PR is good to go
|
Thank you very much for the swift approval @comphead |
|
I will raise a doc change PR shortly |
Which issue does this PR close?
Closes #2101
Closes #.
Rationale for this change
Moving towards goal to support more native operations and not fallback to spark .
What changes are included in this PR?
How are these changes tested?
Unit tests in comet expression suite
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->