GH-43183: [C++] Add date{32,64} to date{32,64} cast
#43192
Conversation
|
|
Fokko
changed the title
date to date castdate{32,64} to date{32,64} cast
github-actions
bot
added
awaiting committer review
|
@Fokko Thank you for your effort! I think LGTM. For more detailed review, @kou (reviewed similar casting logic implementations I did before) or @felipecrv might be able to take a look. |
|
Thanks for jumping in here @llama90 appreciate it 👍 |
python/pyarrow/tests/test_ipc.py
Outdated
| @@ -1199,6 +1199,25 @@ def make_batches(): | |||
| reader = pa.RecordBatchReader.from_batches(None, batches) | |||
| pass | |||
|
|
|||
| # https://github.com/apache/arrow/issues/43183 | |||
There was a problem hiding this comment.
If we really want to test this on the Python side, it would be more logical to add a test to test_compute.py as this is not IPC-related at all.
There was a problem hiding this comment.
That makes sense, thanks 👍
github-actions
bot
added
awaiting merge
python/pyarrow/tests/test_compute.py
Outdated
| schema = pa.schema([ | ||
| ('date32', pa.date32()), | ||
| ('date64', pa.date64()), | ||
| ]) | ||
|
|
||
| batch = pa.RecordBatch.from_arrays(data, schema=schema) | ||
|
|
||
| table = pa.RecordBatchReader.from_batches( | ||
| schema, | ||
| [batch] | ||
| ).cast(schema).read_all() |
There was a problem hiding this comment.
Why do we need to create a table here?
Can we use pc.cast() or Array.cast like other tests do?
There was a problem hiding this comment.
Yes, certainly. I've been using this code since it was exactly the code that caused issues on my end. I've updated the test 👍
|
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 5e451d8. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them. |
Rationale for this change
This one seems to be missing, see #43183
What changes are included in this PR?
Are these changes tested?
I'm not sure what the best place is to test this, please advise
Are there any user-facing changes?
date{32,64}todate{32,64}#43183