Uh oh!
There was an error while loading. Please reload this page.
impl binary ops between binary arrays and scalars - #3124
Conversation
Codecov Report
@@ Coverage Diff @@## master #3124 +/- ##
==========================================
- Coverage 85.74% 85.72% -0.02%
==========================================
Files 294 294 Lines 53770 53852 +82 ==========================================
+ Hits 46104 46165 +61 - Misses 7666 7687 +21
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
alamb
left a comment
There was a problem hiding this comment.
Thank you for the contribution @ozgrakkurt !
Looks like this PR has a clippy errors
Also, can you please add some tests for this? Specifically, it would be great to get an end to end test
Perhaps:
- make a RecordBatch with a BinaryArray and LargeBinaryArray (like this example: https://github.com/apache/arrow-datafusion/blob/3eb55e9a0510d872f6f7765b1a5f17db46486e45/datafusion/row/src/lib.rs#L351)
- Write a query that does
SELECT binary_col='foo', binary_col = other_binary_col, large_binary_col = 'bar', large_binary_col = other_large_binary_col, following the model https://github.com/apache/arrow-datafusion/blob/834924f274d9e04444030b8eb3e07e1035fcd3cf/datafusion/core/tests/sql/functions.rs#L114-L143
Also, it appears that the fmt CI check has failed: https://github.com/apache/arrow-datafusion/runs/7839919855?check_suite_focus=true
to fix you probably need to run cargo fmt and check it in!
Thank you so much!
alamb
commented
Aug 17, 2022
It looks like all this PR needs is some tests. @ozgrakkurt is that something you can help with? |
alamb
commented
Aug 17, 2022
Marking as draft while we work on the tests |
ozgrakkurt
commented
Aug 30, 2022
Hey @alamb Sorry for late follow up, I was busy with my full time job, Currently can't put binary literals into sql. Does creating a logical plan to test this sound ok? |
alamb
commented
Aug 31, 2022
I totally understand -- no worries. I was out last week anyways :)
I think that would be ok. An alternate would be to use a function that creates a select sha256('foo') = sha256('bar') |
ozgrakkurt
commented
Aug 31, 2022
I suggest I add support for binary datatypes to those functions in this PR as well (which should be easy) and create the test using them |
ozgrakkurt
commented
Aug 31, 2022
closes #3308 |
ozgrakkurt
commented
Aug 31, 2022
@alamb should be ready to review |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
ozgrakkurt
commented
Aug 31, 2022
@alamb committing fix for digest function signature and formatting errors |
ozgrakkurt
commented
Aug 31, 2022
@alamb sorry, can you review again |
ozgrakkurt
commented
Sep 1, 2022
tests are failing because of #3327 |
ozgrakkurt
commented
Sep 1, 2022
@alamb can you merge this when you have time? |
ursabot
commented
Sep 2, 2022
Benchmark runs are scheduled for baseline = 8b2fd22 and contender = 24db13b. 24db13b is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
* impl binary ops between binary arrays and scalars * impl binary ops between binary arrays and scalars * run fmt * add binary array input support in digest functions * remove wrong input typecheck for digest functions * fix digest function signatures * add test for binary equality * fix signature of trim function * run fmt * fix digest function signature, run fmt
closes#3117