Derby
  1. Derby
  2. DERBY-4651

Hidden assumptions in FromVTI.makeRestriction()

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 10.6.1.0
    • Fix Version/s: 10.6.2.1, 10.7.1.1
    • Component/s: SQL
    • Labels:
      None

      Description

      FromVTI.makeRestriction() has this code to strip away Boolean constants from AND and OR expressions:

      // strip off trailing vacuous constant if present
      if ( andOperator.getRightOperand() instanceof BooleanConstantNode )

      { return makeRestriction( andOperator.getLeftOperand(), columnNameMap ); }

      and

      // strip off trailing vacuous constant if present
      if ( orOperator.getRightOperand() instanceof BooleanConstantNode )

      { return makeRestriction( orOperator.getLeftOperand(), columnNameMap ); }

      The code for AND will only work as expected if the right operand is TRUE. The code for OR only works correctly if the right operand is FALSE.

      I'm not sure if this can ever result in user-visible bugs, since Boolean constants are usually removed before we get to this point in the code. The predicate has probably been transformed to conjunctive normal form, in which Boolean constants in the right operand of an AndNode or an OrNode in fact is always TRUE or FALSE, respectively.

      I think this code either should be changed to work regardless of the value of the constant in the right operand, or the assumption that the predicate is on conjunctive normal form should be documented in the comments (and perhaps also checked in an assert statement).

      1. no_cast.diff
        2 kB
        Knut Anders Hatlen
      2. derby-4651-02-aa-fixBug.diff
        2 kB
        Rick Hillegas
      3. derby-4651-01-aa-verificationTest.diff
        9 kB
        Rick Hillegas

        Issue Links

          Activity

          Knut Anders Hatlen created issue -
          Hide
          Rick Hillegas added a comment -

          Thanks for looking closely at the code, Knut. The improvement you suggest sounds like a good idea, if only because the code is puzzling. I believe that the vacuous trailing constants are added by the normalization logic and are always TRUE in the case of ANDs and FALSE in the case of ORs. In any event, I don't think any rows will be incorrectly qualified. That is because the qualification is redundantly enforced by Derby after the table function returns each row.

          Show
          Rick Hillegas added a comment - Thanks for looking closely at the code, Knut. The improvement you suggest sounds like a good idea, if only because the code is puzzling. I believe that the vacuous trailing constants are added by the normalization logic and are always TRUE in the case of ANDs and FALSE in the case of ORs. In any event, I don't think any rows will be incorrectly qualified. That is because the qualification is redundantly enforced by Derby after the table function returns each row.
          Hide
          Knut Anders Hatlen added a comment -

          In the OR case, I think the end result would actually end up being wrong. If the predicate is X < 5 OR TRUE, and only X < 5 is pushed into the table function, the rows where X >= 5 won't be returned from the table function and Derby has no way to recreate these outside the table function.

          But I agree, the predicates should have been normalized before we get there, so updating the comments would probably suffice.

          Show
          Knut Anders Hatlen added a comment - In the OR case, I think the end result would actually end up being wrong. If the predicate is X < 5 OR TRUE, and only X < 5 is pushed into the table function, the rows where X >= 5 won't be returned from the table function and Derby has no way to recreate these outside the table function. But I agree, the predicates should have been normalized before we get there, so updating the comments would probably suffice.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-4651-01-aa-verificationTest.diff. This patch adds a test case to track the fact that the VTI's qualification logic isn't short-circuited by queries whose WHERE clauses end in constant expressions. Committed at subversion revision 943605 and ported to the 10.6 branch at revision 943616.

          I have not been able to figure out how to trigger the problem case raised by Knut. Right now Derby rejects the true and false literals. I have tried to replace them with simple expressions which are really constant, in the hope that the compiler would pre-compute the constant expressions and substitute them in the query tree. However, all I have succeeded in doing is forcing the compiler to not pass any restriction into the VTI.

          This problem case raised by Knut may very well arise once we re-enable the true and false literals. We should write some defensive code around this situation before we re-enable the literals.

          Touches the following files:

          -------

          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/IntegerArrayVTI.java

          Beefs up the VTI with machinery to actually enforce the restriction that was pushed into the VTI and to report the number of rows which came out of the VTI.

          -------

          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/RestrictedVTITest.java

          New test case to verify my simple attempts to create the problem situation reported by Knut.

          Show
          Rick Hillegas added a comment - Attaching derby-4651-01-aa-verificationTest.diff. This patch adds a test case to track the fact that the VTI's qualification logic isn't short-circuited by queries whose WHERE clauses end in constant expressions. Committed at subversion revision 943605 and ported to the 10.6 branch at revision 943616. I have not been able to figure out how to trigger the problem case raised by Knut. Right now Derby rejects the true and false literals. I have tried to replace them with simple expressions which are really constant, in the hope that the compiler would pre-compute the constant expressions and substitute them in the query tree. However, all I have succeeded in doing is forcing the compiler to not pass any restriction into the VTI. This problem case raised by Knut may very well arise once we re-enable the true and false literals. We should write some defensive code around this situation before we re-enable the literals. Touches the following files: ------- M java/testing/org/apache/derbyTesting/functionTests/tests/lang/IntegerArrayVTI.java Beefs up the VTI with machinery to actually enforce the restriction that was pushed into the VTI and to report the number of rows which came out of the VTI. ------- M java/testing/org/apache/derbyTesting/functionTests/tests/lang/RestrictedVTITest.java New test case to verify my simple attempts to create the problem situation reported by Knut.
          Rick Hillegas made changes -
          Field Original Value New Value
          Attachment derby-4651-01-aa-verificationTest.diff [ 12444328 ]
          Rick Hillegas made changes -
          Assignee Rick Hillegas [ rhillegas ]
          Hide
          Rick Hillegas added a comment - - edited

          Attaching derby-4651-02-aa-fixBug.diff. This patch adds some defensive logic so that boolean constants are short-circuited only when they have no effect on the result.

          Committed at subversion revision 943633. Ported to the 10.6 branch at subversion revision 943634.

          As a consequence of this patch, Knut's problem scenario will raise an exception rather than return wrong results. As I said, I have not been able to figure out how to create Knut's scenario but it may arise when we re-enable boolean literals. At that time, we may want to handle this situation more gracefully. For instance, we may want to add two new kinds of Restriction subclasses to model the boolean literals.

          Touches the following file:

          M java/engine/org/apache/derby/impl/sql/compile/FromVTI.java

          Show
          Rick Hillegas added a comment - - edited Attaching derby-4651-02-aa-fixBug.diff. This patch adds some defensive logic so that boolean constants are short-circuited only when they have no effect on the result. Committed at subversion revision 943633. Ported to the 10.6 branch at subversion revision 943634. As a consequence of this patch, Knut's problem scenario will raise an exception rather than return wrong results. As I said, I have not been able to figure out how to create Knut's scenario but it may arise when we re-enable boolean literals. At that time, we may want to handle this situation more gracefully. For instance, we may want to add two new kinds of Restriction subclasses to model the boolean literals. Touches the following file: M java/engine/org/apache/derby/impl/sql/compile/FromVTI.java
          Rick Hillegas made changes -
          Attachment derby-4651-02-aa-fixBug.diff [ 12444332 ]
          Rick Hillegas made changes -
          Link This issue relates to DERBY-4583 [ DERBY-4583 ]
          Hide
          Knut Anders Hatlen added a comment -

          Thanks for fixing this, Rick. I'm happy with this resolution.

          > As a consequence of this patch, Knut's problem scenario will raise
          > an exception rather than return wrong results.

          It seems to me that what will happen if that scenario arises after the
          latest changes, is that makeRestriction() will return null. This
          prevents the restriction from being pushed (and ensures that the
          correct results will be returned), but won't cause an exception. Did I
          miss something?

          Show
          Knut Anders Hatlen added a comment - Thanks for fixing this, Rick. I'm happy with this resolution. > As a consequence of this patch, Knut's problem scenario will raise > an exception rather than return wrong results. It seems to me that what will happen if that scenario arises after the latest changes, is that makeRestriction() will return null. This prevents the restriction from being pushed (and ensures that the correct results will be returned), but won't cause an exception. Did I miss something?
          Hide
          Knut Anders Hatlen added a comment -

          I'm making one small simplification before I close the issue. isBooleanTrue() and isBooleanFalse() are defined in ValueNode, and return false if the object is not an instance of BooleanConstantNode, so there's no need to check the type of the right operand and cast it to BooleanConstantNode before calling one of those methods. Removing redundant check and cast in patch no_cast.diff.

          Committed revision 944174.

          Show
          Knut Anders Hatlen added a comment - I'm making one small simplification before I close the issue. isBooleanTrue() and isBooleanFalse() are defined in ValueNode, and return false if the object is not an instance of BooleanConstantNode, so there's no need to check the type of the right operand and cast it to BooleanConstantNode before calling one of those methods. Removing redundant check and cast in patch no_cast.diff. Committed revision 944174.
          Knut Anders Hatlen made changes -
          Attachment no_cast.diff [ 12444480 ]
          Knut Anders Hatlen made changes -
          Status Open [ 1 ] Closed [ 6 ]
          Fix Version/s 10.6.1.1 [ 12314973 ]
          Fix Version/s 10.7.0.0 [ 12314971 ]
          Resolution Fixed [ 1 ]
          Hide
          Rick Hillegas added a comment -

          Thanks for polishing the patch, Knut.

          You are also right about null being returned rather than an exception being raised. At one point iAmConfused() raised an exception. But I see that at some point I changed that method to return null instead. So you see I was confused about iAmConfused().

          Thanks!

          Show
          Rick Hillegas added a comment - Thanks for polishing the patch, Knut. You are also right about null being returned rather than an exception being raised. At one point iAmConfused() raised an exception. But I see that at some point I changed that method to return null instead. So you see I was confused about iAmConfused(). Thanks!
          Kathey Marsden made changes -
          Fix Version/s 10.6.2.0 [ 12315342 ]
          Fix Version/s 10.6.1.1 [ 12314973 ]
          Knut Anders Hatlen made changes -
          Fix Version/s 10.6.2.1 [ 12315343 ]
          Fix Version/s 10.6.2.0 [ 12315342 ]
          Rick Hillegas made changes -
          Fix Version/s 10.7.1.1 [ 12315564 ]
          Fix Version/s 10.7.1.0 [ 12314971 ]
          Gavin made changes -
          Workflow jira [ 12510597 ] Default workflow, editable Closed status [ 12800453 ]

            People

            • Assignee:
              Rick Hillegas
              Reporter:
              Knut Anders Hatlen
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development