Uploaded image for project: 'Calcite'
  1. Calcite
  2. CALCITE-2007

RexSimplify incorrectly simplifies AND bounds

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.14.0
    • Fix Version/s: 1.15.0
    • Component/s: core
    • Labels:
      None

      Description

      For example, RexSimplify incorrectly simplifies 1 < x AND x < 3 to x < 3. Patch in linked PR.

        Activity

        Hide
        julianhyde Julian Hyde added a comment -

        Resolved in release 1.15.0 (2017-12-11).

        Show
        julianhyde Julian Hyde added a comment - Resolved in release 1.15.0 (2017-12-11).
        Hide
        gian Gian Merlino added a comment -

        Got it, thanks for the tips.

        Show
        gian Gian Merlino added a comment - Got it, thanks for the tips.
        Hide
        julianhyde Julian Hyde added a comment -

        Thanks, Gian Merlino.

        For commit messages, we tend to omit the trailing ".". If the commit fixes a bug, use the bug description, "RexSimplify incorrectly simplifies AND bounds" rather than what was done, "Fix RexSimplify behavior when literals come first." And add "Close apache/calcite#547" to the commit comments to trigger the ASF bot to close the PR.

        Show
        julianhyde Julian Hyde added a comment - Thanks, Gian Merlino . For commit messages, we tend to omit the trailing ".". If the commit fixes a bug, use the bug description, "RexSimplify incorrectly simplifies AND bounds" rather than what was done, "Fix RexSimplify behavior when literals come first." And add "Close apache/calcite#547" to the commit comments to trigger the ASF bot to close the PR.
        Show
        gian Gian Merlino added a comment - Fixed in https://git1-us-west.apache.org/repos/asf?p=calcite.git;a=commit;h=06c18ca74df2d4bf20a5dccef11d424ab28c97d7 .
        Hide
        gian Gian Merlino added a comment -

        Thanks for the review Julian. I'll make those changes and then commit the patch.

        Show
        gian Gian Merlino added a comment - Thanks for the review Julian. I'll make those changes and then commit the patch.
        Hide
        julianhyde Julian Hyde added a comment -

        The process is loose. As a committer, you can commit directly, or you can ask for review. Your choice, and we won't blame you if you screw up.

        FWIW, here's my review:

        • I'd use switch (e.getKind()) ... rather than if (e.isA(...)) ....
        • There are already some tests for RexSimplify: see RexProgramTest.testSimplifyXxx. Either add your tests there, or move all the tests into your new class RexSimplifyTest. If the latter, add RexSimplifyTest.class to CalciteSuite.
        Show
        julianhyde Julian Hyde added a comment - The process is loose. As a committer, you can commit directly, or you can ask for review. Your choice, and we won't blame you if you screw up. FWIW, here's my review: I'd use switch (e.getKind()) ... rather than if (e.isA(...)) ... . There are already some tests for RexSimplify : see RexProgramTest.testSimplifyXxx . Either add your tests there, or move all the tests into your new class RexSimplifyTest . If the latter, add RexSimplifyTest.class to CalciteSuite .
        Hide
        gian Gian Merlino added a comment - - edited

        Julian Hyde last time I raised a PR you asked me if I could commit it directly. I haven't done that before, so I'm wondering, what is the process? Is it important to get someone else to review the code first or is it OK to commit unilaterally and then just update the JIRA?

        Sorry if this is answered in a doc that I didn't see. Thanks!

        Show
        gian Gian Merlino added a comment - - edited Julian Hyde last time I raised a PR you asked me if I could commit it directly. I haven't done that before, so I'm wondering, what is the process? Is it important to get someone else to review the code first or is it OK to commit unilaterally and then just update the JIRA? Sorry if this is answered in a doc that I didn't see. Thanks!

          People

          • Assignee:
            gian Gian Merlino
            Reporter:
            gian Gian Merlino
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development