[CALCITE-4771] add TRY_CAST function (enabled in MSSQL library) #3136
Conversation
| @@ -431,48 +431,59 @@ protected SqlOperatorFixture fixture() { | |||
| true); | |||
| } | |||
|
|
|||
| /** Tests that CAST and SAFE_CAST are basically equivalent but SAFE_CAST is | |||
| * only available in BigQuery library. */ | |||
| @Test void testCastVsSafeCast() { | |||
There was a problem hiding this comment.
adding a dialect udf shouldn't have touched so many existing tests, can you explain why?
There was a problem hiding this comment.
Because I want to reuse most of the test code for CAST and SAFE_CAST, just like what we did in
#3093
There was a problem hiding this comment.
in my understanding parameterized tests are better suited for this.
I'd suggest some refactoring into these tests. let's see what others think
There was a problem hiding this comment.
In this case the behavior of the functions (SAFE_CAST, TRY_CAST) are identical, it is just named differently across dialects, so I am a fan of the code reuse.
There was a problem hiding this comment.
@zinking All the tests in this class are in form of parameterized test except testCastVsSafeCastVsTryCast, such as testCastToString, do you mean I also change this test into a parameterized one?
There was a problem hiding this comment.
right, that would be better in my unserstanding
There was a problem hiding this comment.
@zinking I have modified this, please have another look when you have time
|
Kudos, SonarCloud Quality Gate passed! |
|
I am +1 now, @olivrlee @libenchao can some of you let the CI go and merge if no other concerns |
|
@zinking Thanks for your review, I've approved the CI, will go through the PR shortly, and will merge it if everything looks good to me. |
add TRY_CAST function (enabled in MSSQL library)