[GitHub] [arrow-datafusion] alamb commented on pull request #6234: Port remainder of `window.rs` to sqllogictest

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

alamb commented on PR #6234:
URL: https://github.com/apache/arrow-datafusion/pull/6234#issuecomment-1542080696

   > Other than these this PR is LGTM!. 
   
   Thank you for the review @mustafasrepo  -- I will make the changes
   
   > However, I wonder whether we should keep an example test case each file. Because, tests in .rs files are quite useful during development. 
   
   Can you explain how SQL tests in .rs files are more useful during development than ones in .slt files? I suspect you are using a different workflow from me (which is totally good and understandable, but I therefore lack context).
   
    When I am working on some new feature, I found it quite easy to make some `new_feature.slt` file with just the SQL I want to run and then test with `cargo test --test sqllogictests -- new_feature`. This is often better for me than .rs tests because I can change the test and re-run it immediately without having to wait for it to recompile.
   
   > Novices can use these example test cases as starting point when they work on new functionality. I am not really sure, whether it will be helpful. What are you thoughts about it?
   
   My thoughts on this topic are:
   1. I agree that for people starting out with DataFusion development .rs tests are easier (because they are familiar to all Rust developers). sqllogictests are harder because they require learning another tool (though I think the barrier is pretty low)
   2. However,  for SQL based tests, sqllogictest based tests are *much* easier to maintain (specifically updating the tests with `cargo test --test sqllogictests -- --complete`). In my opinion this long term benefit is worth the short term cost
   
   The reason I am spending effort porting (and asking others to help) port the existing tests to sqllogictest is precisely to discourage new SQL tests written in rust (so the code is easier to maintain long term)
   
   🤔  if this makes sense to you, perhaps I can try to incorporate some of this rational into the documentation to make it clearer. 
   
   
   
   


-- 
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