[CALCITE-6248] Illegal dates are accepted by casts #3708
Conversation
|
This is the PR which is being reverted in this one: #3685 |
|
|
It looks like this CI failure is to be expected, since it fails with Avatica main. That should pass once Avatica main contains the corresponding fix. |
| f.checkScalar("cast('1945-30-24' as DATE)", "1947-06-26", "DATE NOT NULL"); | ||
| // Remove the if condition and the else block once CALCITE-6248 is fixed in Avatica | ||
| if (TestUtil.AVATICA_VERSION.startsWith("1.0.0-dev-main")) { | ||
| f.checkFails("cast('52534253' as DATE)", BAD_DATETIME_MESSAGE, true); |
There was a problem hiding this comment.
There is a unit test failed
There was a problem hiding this comment.
The unit test failed because the changes in avatica have not been merged.
| @@ -1320,7 +1320,7 @@ void testCastStringToDateTime(CastType castType, SqlOperatorFixture f) { | |||
| f.checkCastToString("DATE '1945-2-24'", null, "1945-02-24", castType); | |||
|
|
|||
| f.checkScalar("cast('1945-02-24' as DATE)", "1945-02-24", "DATE NOT NULL"); | |||
| f.checkScalar("cast(' 1945-02-04 ' as DATE)", "1945-02-04", "DATE NOT NULL"); | |||
There was a problem hiding this comment.
Is it possible to add new unit tests instead of changing existing test cases?
There was a problem hiding this comment.
I am afraid we have to change the tests because they are wrong.
|
@mihaibudiu |
|
I sent a message on the dev list about this. I still have to figure out how to do this. We need simultaneous changes to avatica and calcite to make this pass. |
Signed-off-by: Mihai Budiu <[email protected]>
|
This is ready for review, to enable migration to Avatica 1.25. |
|
I am thinking to merge this PR, since there isn't much to review, so we can proceed with Avatica. |
This is the second attempt to provide a partial fix for [CALCITE-6248].
The fix is partial, because the bug is in Avatica, but Avatica CI fails if this fix is not applied.
I have already merged a PR for this issue, but here I revert the changes in that prior PR.
I apologize for this change of mind, it has become apparent that the previous choice for the fix was incorrect.
The fix is about CAST(string to DATE). The assumption is that this cast should be more lenient than DATE literals in parsing the string, but should still require the date to be correct.
The plan is to merge this PR, then attempt to merge the corresponding Avatica PR: apache/calcite-avatica#238. If that PR passes, it can be merged. Then some leftover code in Calcite can be cleaned-up when a new Calcite release picks up a new Avatica release.
Unfortunately I don't know of an easy way to test this PR before it's merged.