[CALCITE-4771] add TRY_CAST function (enabled in MSSQL library)#3136
[CALCITE-4771] add TRY_CAST function (enabled in MSSQL library)#3136zoudan wants to merge 2 commits intoapache:mainfrom
Conversation
|
|
||
| /** 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. |
4 participants
add TRY_CAST function (enabled in MSSQL library)