[GitHub] [arrow-datafusion] berkaysynnada commented on pull request #6433: Refactor temporal arithmetic

["berkaysynnada (via GitHub)" <gi...@apache.org>]

berkaysynnada commented on PR #6433:
URL: https://github.com/apache/arrow-datafusion/pull/6433#issuecomment-1567146425

   > Thank you @berkaysynnada -- in general this code is looking quite good. I think it is structured nicely and is improving
   > 
   > My one big concern is that the new code seems to be mostly be untested -- specifically there are a lot of new functions and code that looks similar to me such as `ts_ms_interval_mdn_op` and `ts_us_interval_mdn_op`
   > 
   > Do you know if this code covered by existing tests?
   > 
   > I realize it might be repetitive to write tests for this code, but I think it is is really important to avoid regressions in the future. Perhaps there is some way to reduce the code replication (and therefore also the testing burden) 🤔
   
   Thank you for the review ☺️ I have applied your suggestions. I have added some tests that compares the results of a(ScalarValue) vs b(another ScalarValue) and a vs c(Array having one element and the same value of b). I thought this would suffice, as the ScalarValue tests are wide-ranging and testing edge cases. I have also tried to reduce arithmetic functions as much as possible, but I would appreciate it if you let me know if anything else catches your eye.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org