[jira] [Commented] (CALCITE-4917) Add test for 'IS NOT NULL(a) AND a=b' simplification

["Julian Hyde (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/CALCITE-4917?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17452566#comment-17452566 ] 

Julian Hyde commented on CALCITE-4917:
--------------------------------------

Thanks for this change. Tests are really important.

But I'll quibble with the format. The test case names are not very useful, and putting the tests in their own methods is not very useful. (On balance, that is - I know there is some benefit to allowing each test to fail individual. But it's outweighed by readability, conciseness and organization.)

I'd use the following format (based on an example in {{{}testSimplifyNot{}}}):
{code:java}
// "NOT(IS NOT NULL(x)) => "IS NULL(x)"
checkSimplify(not(isNotNull(vBool())), "IS NULL(?0.bool0)"); {code}
It's easy to read what the test is testing. Which is important when you are scanning through a file with 1,000 tests to find whether the test you need already exists. And it groups it with similar tests (in the same method).

Lastly, in the subject of the Jira case, I would write 'a IS NOT NULL AND a = b'. SQL syntax is always preferred in release notes.

I saw that [~vlsi]  has already +1ed but consider making the change I suggested.

> Add test for 'IS NOT NULL(a) AND a=b' simplification
> ----------------------------------------------------
>
>                 Key: CALCITE-4917
>                 URL: https://issues.apache.org/jira/browse/CALCITE-4917
>             Project: Calcite
>          Issue Type: Test
>          Components: core
>    Affects Versions: 1.28.0
>            Reporter: Alessandro Solimando
>            Assignee: Alessandro Solimando
>            Priority: Minor
>              Labels: pull-request-available
>          Time Spent: 0.5h
>  Remaining Estimate: 0h
>
> Simplification (from _RexSimplify_ class) is mostly covered in {_}RexProgramTest{_}, but tests for expressions of the form "IS NOT NULL(a) AND a=b" (with nullable and not nullable types) seem to be missing.
>  
> Since I had to write tests to make sure the simplification was as expected, I assume others might end up doing the same, and that the tests will both act as documentation and it will also protect against regressions.



--
This message was sent by Atlassian Jira
(v8.20.1#820001)