[CALCITE-5921] SqlOperatorFixture.checkFails and checkAggFails don't check runtime failure#3412
[CALCITE-5921] SqlOperatorFixture.checkFails and checkAggFails don't check runtime failure#3412herunkang2018 wants to merge 2 commits intoapache:mainfrom
Conversation
| // input to extract for a particular input type. | ||
| String INVALID_EXTRACT_UNIT_CONVERTLET_ERROR = | ||
| "Extract.*from.*type data is not supported"; | ||
| "Was not expecting value.*for enumeration.*in this context"; |
There was a problem hiding this comment.
This is to fix SqlOperatorTest.testExtractIntervalDayTime, the full error message is Was not expecting value 'DOY' for enumeration 'org.apache.calcite.avatica.util.TimeUnit' in this context.
I think the previous error message was wrong before, because I find that the exception check logic is introduced by CALCITE-1987 in 2017, and has remained unchanged, and the previous error message is introduced by CALCITE-4885 in 2021.
c3e8603 to
8b84a23
Compare
|
Kudos, SonarCloud Quality Gate passed! |
| SqlValidator validator = factory.createValidator(); | ||
| SqlNode n = parseAndValidate(validator, sql); | ||
| assertNotNull(n); | ||
| tester.checkFails(factory, sap, expectedError, true); |
There was a problem hiding this comment.
the last parameter to this call should be "runtime" and not "true"
There was a problem hiding this comment.
IMO the line 160 checks the runtime is true, so this line uses true is ok.
| SqlValidator validator = factory.createValidator(); | ||
| SqlNode n = parseAndValidate(validator, sql); | ||
| assertNotNull(n); | ||
| tester.checkAggFails(factory, expr, inputValues, expectedError, true); |
mihaibudiu
left a comment
There was a problem hiding this comment.
There is something more deeply broken in the implementation of checkFails.
You can write a test that will fail when called with checkScalarExact (by throwing an exception), but will also fail when called with checkFails (because it doesn't throw an exception)! It looks like the testing code for checkFails takes a different path through execution than the code for checkScalarExact. I am still trying to figure out where the two executions diverge, they seem to apply different optimizations to the expression.
So while indeed 5990 introduced a regression, it looks like there are several other problems that this change is surfacing. One of them seems to be that Calcite always implemented casts wrong, by not checking for overflow. Only some simplification code seems to trigger exceptions.
|
Please consider #3471 instead of this PR, it fixes a few extra things. |
|
Closing this one in favor of #3471 |
No description provided.