Details

    • Urgency:
      Normal

      Description

      code attempts to compare two BaseColumnNodes but doesn't compare the tableName correctly.

      1. runoutputJune2.out
        96 kB
        Jayaram Subramanian
      2. runoutputJune14.out
        11 kB
        Jayaram Subramanian
      3. june15.out
        41 kB
        Jayaram Subramanian
      4. IsEquivalent_Donotcommit_june14.txt
        1.0 kB
        Jayaram Subramanian
      5. IsEquivalent_DoNotCommit_June11.txt
        0.9 kB
        Jayaram Subramanian
      6. derby.log
        78 kB
        Jayaram Subramanian
      7. derby.log
        278 kB
        Jayaram Subramanian
      8. bad_equivalence_check.diff
        0.6 kB
        Dave Brosius

        Activity

        Dave Brosius created issue -
        Dave Brosius made changes -
        Field Original Value New Value
        Attachment bad_equivalence_check.diff [ 12470545 ]
        Dave Brosius made changes -
        Attachment bad_equivalence_check.diff [ 12470551 ]
        Dave Brosius made changes -
        Attachment bad_equivalence_check.diff [ 12470545 ]
        Hide
        Knut Anders Hatlen added a comment -

        The current code looks wrong and the suggested fix looks right.

        None of our regression tests seem to exercise this code path, though. It would be good if someone could come up with a statement that caused this code to be executed so that we can verify that it behaves as expected after the proposed changes.

        Show
        Knut Anders Hatlen added a comment - The current code looks wrong and the suggested fix looks right. None of our regression tests seem to exercise this code path, though. It would be good if someone could come up with a statement that caused this code to be executed so that we can verify that it behaves as expected after the proposed changes.
        Rick Hillegas made changes -
        Fix Version/s 10.8.1.1 [ 12316356 ]
        Fix Version/s 10.8.1.0 [ 12315561 ]
        Rick Hillegas made changes -
        Fix Version/s 10.8.1.2 [ 12316362 ]
        Fix Version/s 10.8.1.1 [ 12316356 ]
        Hide
        Kathey Marsden added a comment -

        There was some discussion on the list about tracking down the statement that goes through BaseColumnNode.isEquivalient() , by adding a thread dump in the method. I wanted to suggest an alternate method that might make things easier to track down. I may be missing the mark as I have not been folllowing the thread on the list carefully.

        I think if you set the system property derby.language.logStatementText=true with -Dderby.language.logStatementText=true and then in the method call org.apache.derby.iapi.services.monitor.Monitor.getMonitor().logThrowable(new Exception("Trace in isEquivalent")
        you should get the stack trace right after the statement that goes there in derby.log

        Show
        Kathey Marsden added a comment - There was some discussion on the list about tracking down the statement that goes through BaseColumnNode.isEquivalient() , by adding a thread dump in the method. I wanted to suggest an alternate method that might make things easier to track down. I may be missing the mark as I have not been folllowing the thread on the list carefully. I think if you set the system property derby.language.logStatementText=true with -Dderby.language.logStatementText=true and then in the method call org.apache.derby.iapi.services.monitor.Monitor.getMonitor().logThrowable(new Exception("Trace in isEquivalent") you should get the stack trace right after the statement that goes there in derby.log
        Hide
        Jayaram Subramanian added a comment -

        The getmonitor() method returns a modulefactory interface which doesnt have a logThrowable method in it..

        Show
        Jayaram Subramanian added a comment - The getmonitor() method returns a modulefactory interface which doesnt have a logThrowable method in it..
        Rick Hillegas made changes -
        Fix Version/s 10.8.1.3 [ 12316378 ]
        Fix Version/s 10.8.1.2 [ 12316362 ]
        Hide
        Jayaram Subramanian added a comment -

        Modified the BasecolumnNode class's IsEquivalent method to add the monitor method... But still when executing test.lang.__Suite, the log doesn't show any trace of the newly added trace , i am attaching the patch and the __Suite output

        Show
        Jayaram Subramanian added a comment - Modified the BasecolumnNode class's IsEquivalent method to add the monitor method... But still when executing test.lang.__Suite, the log doesn't show any trace of the newly added trace , i am attaching the patch and the __Suite output
        Jayaram Subramanian made changes -
        Attachment IsEquivalent_DoNotCommit_June11.txt [ 12482144 ]
        Attachment runoutputJune2.out [ 12482145 ]
        Hide
        Bryan Pendleton added a comment -

        Perhaps the code is simply never run, or perhaps the traces are getting swallowed
        up. The tests sometimes redirect the output, so it can be hard to figure out where the
        traces might go.

        Change the method to perform System.exit(1);

        Then see if any of the tests in the test suite fail.

        Show
        Bryan Pendleton added a comment - Perhaps the code is simply never run, or perhaps the traces are getting swallowed up. The tests sometimes redirect the output, so it can be hard to figure out where the traces might go. Change the method to perform System.exit(1); Then see if any of the tests in the test suite fail.
        Jayaram Subramanian made changes -
        Attachment runoutputJune14.out [ 12482635 ]
        Attachment IsEquivalent_Donotcommit_june14.txt [ 12482636 ]
        Hide
        Jayaram Subramanian added a comment -

        After adding the system.exit(1) to the BaseColumnNode.IsEquivalent method and the running java -Dderby.tests.trace=true junit.textui.TestRunner org.apache.derbyTesting.functionTests.tests.lang._Suite > runoutputJune14.out 2>&1
        is causing the log output to stop at the "aggregate" test.
        Attaching the output log and the diff for reference.

        Show
        Jayaram Subramanian added a comment - After adding the system.exit(1) to the BaseColumnNode.IsEquivalent method and the running java -Dderby.tests.trace=true junit.textui.TestRunner org.apache.derbyTesting.functionTests.tests.lang._Suite > runoutputJune14.out 2>&1 is causing the log output to stop at the "aggregate" test. Attaching the output log and the diff for reference.
        Hide
        Bryan Pendleton added a comment -

        If it's definitely the 'aggregate' test which is touching the code, then you should be able
        to confirm this by doing something like:

        java org.apache.derbyTesting.functionTests.harness.RunTest lang/aggregate.sql

        with and without your "System.exit(1)", and see the test pass without your change, and
        exit early with your change.

        Show
        Bryan Pendleton added a comment - If it's definitely the 'aggregate' test which is touching the code, then you should be able to confirm this by doing something like: java org.apache.derbyTesting.functionTests.harness.RunTest lang/aggregate.sql with and without your "System.exit(1)", and see the test pass without your change, and exit early with your change.
        Hide
        Knut Anders Hatlen added a comment -

        Alternatively, since the test will probably fail with some spurious diffs when run in the old test harness now even when it's unmodified, aggregate.sql can be run as a standalone JUnit test by invoking this command:

        java org.apache.derbyTesting.functionTests.tests.lang.LangScripts aggregate

        Thanks for continuing the digging, Jayaram!

        Show
        Knut Anders Hatlen added a comment - Alternatively, since the test will probably fail with some spurious diffs when run in the old test harness now even when it's unmodified, aggregate.sql can be run as a standalone JUnit test by invoking this command: java org.apache.derbyTesting.functionTests.tests.lang.LangScripts aggregate Thanks for continuing the digging, Jayaram!
        Rick Hillegas made changes -
        Fix Version/s 10.8.1.4 [ 12316500 ]
        Fix Version/s 10.8.1.3 [ 12316378 ]
        Jayaram Subramanian made changes -
        Attachment june15.out [ 12482747 ]
        Hide
        Jayaram Subramanian added a comment -

        Modified the the basecolumnNode class isequivalent method to do a Thread.dumpStack(); and then ran the aggregate test alone by doing
        $ java org.apache.derbyTesting.functionTests.tests.lang.LangScripts aggregate
        >june15.out 2>&1

        Verified that executing aggregate resulted in the dump getting produced . But the dump didnt indicate the sql statement which resulted in calling the isquivalent method.. Attaching the june15.out file for reference

        Show
        Jayaram Subramanian added a comment - Modified the the basecolumnNode class isequivalent method to do a Thread.dumpStack(); and then ran the aggregate test alone by doing $ java org.apache.derbyTesting.functionTests.tests.lang.LangScripts aggregate >june15.out 2>&1 Verified that executing aggregate resulted in the dump getting produced . But the dump didnt indicate the sql statement which resulted in calling the isquivalent method.. Attaching the june15.out file for reference
        Hide
        Bryan Pendleton added a comment -

        Now that you know that the aggregate test exercises this code, can you try Kathey's suggestion from
        https://issues.apache.org/jira/browse/DERBY-5010?focusedCommentId=13021634&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13021634

        to see if you can further isolate the particular statement that is being executed?

        Show
        Bryan Pendleton added a comment - Now that you know that the aggregate test exercises this code, can you try Kathey's suggestion from https://issues.apache.org/jira/browse/DERBY-5010?focusedCommentId=13021634&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13021634 to see if you can further isolate the particular statement that is being executed?
        Hide
        Jayaram Subramanian added a comment -

        As mentioned added the Monitor.getMonitor().logThrowable(new Exception("Trace in isEquivalent") and then executed java org.apache.derbyTesting.functionTests.tests.lang.LangScripts aggregate >> June19.out ..

        in the derby log file got
        Sun Jun 19 14:08:06 CDT 2011 Thread[main,5,main] (XID = 624), (SESSIONID = 3), (DATABASE = wombat), (DRDAID = null), Failed Statement is: null

        the failed statement is coming as null.. Attaching the log file for reference... The log file shows the "isequivalent" method being invoked but the statement could not be traced out..

        Show
        Jayaram Subramanian added a comment - As mentioned added the Monitor.getMonitor().logThrowable(new Exception("Trace in isEquivalent") and then executed java org.apache.derbyTesting.functionTests.tests.lang.LangScripts aggregate >> June19.out .. in the derby log file got Sun Jun 19 14:08:06 CDT 2011 Thread [main,5,main] (XID = 624), (SESSIONID = 3), (DATABASE = wombat), (DRDAID = null), Failed Statement is: null the failed statement is coming as null.. Attaching the log file for reference... The log file shows the "isequivalent" method being invoked but the statement could not be traced out..
        Jayaram Subramanian made changes -
        Attachment derby.log [ 12483100 ]
        Hide
        Kathey Marsden added a comment -

        Looking at the derby.log, it seems to me that the derby.language.logStatementText=true property is not getting picked up.
        If you look at the top of the log, there is no statement listed before the first instance of the stack trace printing,

        Database Class Loader started - derby.database.classpath=''
        java.lang.Exception: Trace in isEquivalent
        at org.apache.derby.impl.sql.compile.BaseColumnNode.isEquivalent(BaseColumnNode.java:183)
        at org.apache.derby.impl.sql.compile.SubstituteExpressionVisitor.visit(SubstituteExpressionVisitor.java:62)
        at org.apache.derby.impl.sql.compile.QueryTreeNode.accept(QueryTreeNode.java:718)
        at org.apache.derby.impl.sql.compile.ResultColumn.acceptChildren(ResultColumn.java:1550)

        In your previous comment, you mentioned you ran it as:
        java org.apache.derbyTesting.functionTests.tests.lang.LangScripts aggregate

        try
        java -Dderby.language.logStatementText=true org.apache.derbyTesting.functionTests.tests.lang.LangScripts aggregate

        If that still doesn't show the statement before the exception prints, try running the java/testing/org/apache/derbyTesting/functionTests/tests/lang/aggregate.sql ust with ij, outside of the harness, making sure you start ij with the property

        java -Dderby.language.logStatementText=true org.apache.derby.tools.ij

        Show
        Kathey Marsden added a comment - Looking at the derby.log, it seems to me that the derby.language.logStatementText=true property is not getting picked up. If you look at the top of the log, there is no statement listed before the first instance of the stack trace printing, Database Class Loader started - derby.database.classpath='' java.lang.Exception: Trace in isEquivalent at org.apache.derby.impl.sql.compile.BaseColumnNode.isEquivalent(BaseColumnNode.java:183) at org.apache.derby.impl.sql.compile.SubstituteExpressionVisitor.visit(SubstituteExpressionVisitor.java:62) at org.apache.derby.impl.sql.compile.QueryTreeNode.accept(QueryTreeNode.java:718) at org.apache.derby.impl.sql.compile.ResultColumn.acceptChildren(ResultColumn.java:1550) In your previous comment, you mentioned you ran it as: java org.apache.derbyTesting.functionTests.tests.lang.LangScripts aggregate try java -Dderby.language.logStatementText=true org.apache.derbyTesting.functionTests.tests.lang.LangScripts aggregate If that still doesn't show the statement before the exception prints, try running the java/testing/org/apache/derbyTesting/functionTests/tests/lang/aggregate.sql ust with ij, outside of the harness, making sure you start ij with the property java -Dderby.language.logStatementText=true org.apache.derby.tools.ij
        Knut Anders Hatlen made changes -
        Fix Version/s 10.8.1.5 [ 12316676 ]
        Fix Version/s 10.8.1.4 [ 12316500 ]
        Hide
        Jayaram Subramanian added a comment -

        As suggested ran the aggregate test with logstatementtext=true option and was able to get the sql statement causing the isequivalent method to be called

        select c1 from t1
        group by c1
        having (max(c2) in (select c1 from t2)) OR
        (max(c1) in (select c2-999 from t2)) OR
        (count > 0)

        Show
        Jayaram Subramanian added a comment - As suggested ran the aggregate test with logstatementtext=true option and was able to get the sql statement causing the isequivalent method to be called select c1 from t1 group by c1 having (max(c2) in (select c1 from t2)) OR (max(c1) in (select c2-999 from t2)) OR (count > 0)
        Hide
        Kathey Marsden added a comment -

        Thank you Jayaram for tracking this down!
        Can you post the full stack trace from isEquivalent when this statement is executed?

        Now that we know how to get into isEquivalent(), the next step will be to try to find a variation of this statement that fails without Dave's patch and passes with it. First, to get yourself setup to debug the situation, I would suggest you break out an SQL script with just the first part of aggregate.sql to setup these two tables call it create.sql or some such. Then you can use that to create a database with ij and then you can just connect with ij to the database and run this one sql statement and variations to understand what is happening in isEquivalent, either by printing the values for tableName and other.tableName or using the debugger.

        Show
        Kathey Marsden added a comment - Thank you Jayaram for tracking this down! Can you post the full stack trace from isEquivalent when this statement is executed? Now that we know how to get into isEquivalent(), the next step will be to try to find a variation of this statement that fails without Dave's patch and passes with it. First, to get yourself setup to debug the situation, I would suggest you break out an SQL script with just the first part of aggregate.sql to setup these two tables call it create.sql or some such. Then you can use that to create a database with ij and then you can just connect with ij to the database and run this one sql statement and variations to understand what is happening in isEquivalent, either by printing the values for tableName and other.tableName or using the debugger.
        Hide
        Jayaram Subramanian added a comment -

        adding the full stack trace of isequivalent when the statement got executed

        Show
        Jayaram Subramanian added a comment - adding the full stack trace of isequivalent when the statement got executed
        Jayaram Subramanian made changes -
        Attachment derby.log [ 12483804 ]
        Hide
        Jayaram Subramanian added a comment -

        When executing the sql statement select c1 from t1 group by c1 having (max(c2) in (select c1 from t2)) OR (max(c1) in (select c2-999 from t2)) OR (count > 0); in ij observed the following

        1) the control goes to isequivalent method but if (isSameNodeType(o)) is returning false making the flow not to go in to the modified section of code
        2) the reason why if (isSameNodeType(o)) is returning false is
        2.1) in valuenode class nodetype has a value 62
        2.2) in querytreenode class nodetype has a value 94

        I am not able to infer what these values mean or how they got assigned... but if we are able to get to make them equal then control would go to the modified section of code...

        Show
        Jayaram Subramanian added a comment - When executing the sql statement select c1 from t1 group by c1 having (max(c2) in (select c1 from t2)) OR (max(c1) in (select c2-999 from t2)) OR (count > 0); in ij observed the following 1) the control goes to isequivalent method but if (isSameNodeType(o)) is returning false making the flow not to go in to the modified section of code 2) the reason why if (isSameNodeType(o)) is returning false is 2.1) in valuenode class nodetype has a value 62 2.2) in querytreenode class nodetype has a value 94 I am not able to infer what these values mean or how they got assigned... but if we are able to get to make them equal then control would go to the modified section of code...
        Hide
        Knut Anders Hatlen added a comment -

        Hi Jayaram,

        I think these constants come from C_NodeTypes:

        > static final int COLUMN_REFERENCE = 62;

        and

        > static final int BASE_COLUMN_NODE = 94;

        So it looks like we're calling isEquivalent() to check whether a ColumnReference instance is equivalent to a BaseColumnNode in this query.

        I think the ColumnReference here is the reference to C1 in the GROUP BY clause, and to exercise the code using a variant of this query, we'd have to get the GROUP BY clause to contain a BaseColumnNode somehow. I'm afraid I don't have any good ideas on how to do that (I'm not even sure if it's possible).

        Show
        Knut Anders Hatlen added a comment - Hi Jayaram, I think these constants come from C_NodeTypes: > static final int COLUMN_REFERENCE = 62; and > static final int BASE_COLUMN_NODE = 94; So it looks like we're calling isEquivalent() to check whether a ColumnReference instance is equivalent to a BaseColumnNode in this query. I think the ColumnReference here is the reference to C1 in the GROUP BY clause, and to exercise the code using a variant of this query, we'd have to get the GROUP BY clause to contain a BaseColumnNode somehow. I'm afraid I don't have any good ideas on how to do that (I'm not even sure if it's possible).
        Hide
        Jayaram Subramanian added a comment -

        Hi Knut,
        I tried all the following variations but nothing seem to break the basecolumn to columnreference comparison.. So shall i jump on to the next task
        select c1 from t1 group by c1 having (max(c2) in (select c1 from t2)) OR (max(c1) in (select c2-999 from t2)) OR (count > 0);
        select c1 from t1 group by c1 having (max(c1) in (select c1 from t2)) OR (max(c1) in (select c2-999 from t2)) OR (count > 0);
        select c1 from t1 group by c1 having (max(c1) in (select c1 from t2)) OR (max(c1) in (select c1-999 from t2)) OR (count > 0);
        select c1,count(c1) from t1 group by c1 having (max(c1) in (select c1 from t2)) OR (max(c1) in (select c1-999 from t2)) OR (count > 0);

        Show
        Jayaram Subramanian added a comment - Hi Knut, I tried all the following variations but nothing seem to break the basecolumn to columnreference comparison.. So shall i jump on to the next task select c1 from t1 group by c1 having (max(c2) in (select c1 from t2)) OR (max(c1) in (select c2-999 from t2)) OR (count > 0); select c1 from t1 group by c1 having (max(c1) in (select c1 from t2)) OR (max(c1) in (select c2-999 from t2)) OR (count > 0); select c1 from t1 group by c1 having (max(c1) in (select c1 from t2)) OR (max(c1) in (select c1-999 from t2)) OR (count > 0); select c1,count(c1) from t1 group by c1 having (max(c1) in (select c1 from t2)) OR (max(c1) in (select c1-999 from t2)) OR (count > 0);
        Hide
        Knut Anders Hatlen added a comment -

        Thanks for the continued investigation, Jayaram.

        The more I look at this code, the more convinced I get that the broken code cannot currently be exercised, and your experiments seem to support that.

        In any case, the patch looks correct, so I've committed it to trunk (revision 1143002). Thanks, Dave, for the fix.

        Show
        Knut Anders Hatlen added a comment - Thanks for the continued investigation, Jayaram. The more I look at this code, the more convinced I get that the broken code cannot currently be exercised, and your experiments seem to support that. In any case, the patch looks correct, so I've committed it to trunk (revision 1143002). Thanks, Dave, for the fix.
        Knut Anders Hatlen made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Issue & fix info [Patch Available]
        Assignee Dave Brosius [ dbrosius@apache.org ]
        Fix Version/s 10.9.0.0 [ 12316344 ]
        Fix Version/s 10.8.1.6 [ 12316676 ]
        Resolution Fixed [ 1 ]
        Kathey Marsden made changes -
        Labels derby_backport_reject_10_8
        Hide
        Kathey Marsden added a comment -

        It seems the code may not be reachable so no need to backport.

        Show
        Kathey Marsden added a comment - It seems the code may not be reachable so no need to backport.
        Hide
        Knut Anders Hatlen added a comment -

        [bulk update] Close all resolved issues that haven't been updated for more than one year.

        Show
        Knut Anders Hatlen added a comment - [bulk update] Close all resolved issues that haven't been updated for more than one year.
        Knut Anders Hatlen made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Gavin made changes -
        Workflow jira [ 12544853 ] Default workflow, editable Closed status [ 12801078 ]

          People

          • Assignee:
            Dave Brosius
            Reporter:
            Dave Brosius
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 1h
              1h
              Remaining:
              Remaining Estimate - 1h
              1h
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development