[CALCITE-2438] RexCall#isAlwaysTrue return incorrect result#778
[CALCITE-2438] RexCall#isAlwaysTrue return incorrect result#778jiayuanv127 wants to merge 10 commits intoapache:masterfrom
Conversation
merge master
…ror when sql has "where 'a' is null"
merge master
vlsi
left a comment
vlsi
left a comment
There was a problem hiding this comment.
See in-line comments, and please add more tests if you want to get this PR committed.
-
You have only positive tests. Please add negative tests as well (e.g. to ensure
IS_FALSEcan produceisAlwaysTrueat all) -
You have changed the logic for
IS_NOT_TRUE, and you have added zero cases for that.
Please either revert unintended changes or cover the changes with tests.
| final RelDataType booleanNullableType = | ||
| typeFactory.createTypeWithNullability( | ||
| typeFactory.createSqlType(SqlTypeName.BOOLEAN), true); | ||
| RexNode node = rexBuilder.makeInputRef(booleanNullableType, 0); |
There was a problem hiding this comment.
I am sure it makes no sense to clutter test code with boiler-plate creation of RelDataType and inputrefs
| typeFactory.createSqlType(SqlTypeName.BOOLEAN), true); | ||
| RexNode node = rexBuilder.makeInputRef(booleanNullableType, 0); | ||
|
|
||
| RexNode exp1 = rexBuilder.makeCall(SqlStdOperatorTable.IS_FALSE, isNotNull(isNull(node))); |
There was a problem hiding this comment.
IS_FALSE should be refactored to a method like isNull for consistency and test brevity
| RexNode node = rexBuilder.makeInputRef(booleanNullableType, 0); | ||
|
|
||
| RexNode exp1 = rexBuilder.makeCall(SqlStdOperatorTable.IS_FALSE, isNotNull(isNull(node))); | ||
| assertEquals(exp1.isAlwaysTrue(), false); |
There was a problem hiding this comment.
Please use message parameter for assertEquals or other means to make exception message clear.
Current one would be like "expected false, got true at line 2180.
It would be extremely hard to maintain if ever this test goes off the rails.
The same applies to all assertEquals in this test
| return rexBuilder.makeCall(SqlStdOperatorTable.IS_TRUE, node); | ||
| } | ||
|
|
||
| private RexNode isNotTrue(RexNode node) { |
There was a problem hiding this comment.
Could you please co-locate is*True/False methods? It looks like isNotFalse is declared elsewhere
There was a problem hiding this comment.
Thanks! I have added the isNotFalse method after isNotTrue.
| RexNode inputRef = rexBuilder.makeInputRef(type, 0); | ||
|
|
||
| RexNode isFalseExp = isFalse(isNotNull(isNull(inputRef))); | ||
| assertEquals("isAlwaysTrue should return false,but get true", |
There was a problem hiding this comment.
Please make the test fail (e.g. by replacing isAlwaysTrue with a simple return false), then see how the test reports the problem.
Would you be able to tell the nature of the issue from the error message/stacktrace then?
There was a problem hiding this comment.
Thanks for you suggestion,I have make a change to make the error message more clear.
…pzw) Make tests also call RexSimplify (Julian Hyde) Close apache#778
…pzw) Make tests also call RexSimplify (Julian Hyde) Close apache#778
… (Zoltan Haindrich) closes apache#778 closes apache#875
… (Zoltan Haindrich) closes apache#778 closes apache#875
… (Zoltan Haindrich) closes apache#778 closes apache#875
… (Zoltan Haindrich) closes apache#778 closes apache#875 Change-Id: I98c63b904f2c0164b37e6b0b2a147f8353102a69
2 participants
No description provided.