Derby
  1. Derby
  2. DERBY-2218

Null Pointer Exception when an untyped NULL subquery ("values null") appears outside of the FROM list in a SELECT query.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 10.1.3.2, 10.2.2.1, 10.3.1.4
    • Fix Version/s: 10.2.2.1, 10.3.1.4
    • Component/s: SQL
    • Labels:
      None

      Description

      If a SELECT query contains a subquery which includes an untyped NULL value at any place other than in the FROM list, Derby will throw an NPE at bind time.

      ij> create table t1 (i int);
      0 rows inserted/updated/deleted

      – If the untyped NULL is in the FROM list, a reasonable error is thrown.

      ij> select * from (values null) x;
      ERROR 42X07: Null is only allowed in a VALUES clause within an INSERT statement.

      ij> select * from (select * from t1, (values null) x )y;
      ERROR 42X07: Null is only allowed in a VALUES clause within an INSERT statement.

      – But if it appears anywhere else, the result is an NPE:

      – IN-list
      ij> select * from t1 where i in (1, 2, (values null));
      ERROR XJ001: Java exception: ': java.lang.NullPointerException'.

      – where clause
      select * from t1 where (values null);

      – order by clause
      select * from t1 order by (values null);

      – result column
      select (values null) from t1;

      – group by clause (only works in 10.2 and later)
      select * from t1 group by (values null);

      – having clause
      select * from t1 group by i having (values null);

      Stack trace (from 10.2.2) is:

      java.lang.NullPointerException
      at org.apache.derby.impl.sql.compile.SubqueryNode.setDataTypeServices(SubqueryNode.java:2289)
      at org.apache.derby.impl.sql.compile.SubqueryNode.bindExpression(SubqueryNode.java:529)
      at org.apache.derby.impl.sql.compile.ValueNodeList.bindExpression(ValueNodeList.java:130)
      at org.apache.derby.impl.sql.compile.BinaryListOperatorNode.bindExpression(BinaryListOperatorNode.java:161)
      at org.apache.derby.impl.sql.compile.SelectNode.bindExpressions(SelectNode.java:540)
      at org.apache.derby.impl.sql.compile.DMLStatementNode.bindExpressions(DMLStatementNode.java:249)
      at org.apache.derby.impl.sql.compile.DMLStatementNode.bind(DMLStatementNode.java:162)
      at org.apache.derby.impl.sql.compile.CursorNode.bind(CursorNode.java:253)
      at org.apache.derby.impl.sql.GenericStatement.prepMinion(GenericStatement.java:345)
      at org.apache.derby.impl.sql.GenericStatement.prepare(GenericStatement.java:119)

      1. derby2218-10.2-diff01.txt
        6 kB
        Yip Ng
      2. derby2218-10.2-stat01.txt
        0.5 kB
        Yip Ng
      3. derby2218-trunk-diff03.txt
        3 kB
        Yip Ng
      4. derby2218-trunk-stat03.txt
        0.3 kB
        Yip Ng
      5. derby2218-trunk-diff02.txt
        4 kB
        Yip Ng
      6. derby2218-trunk-stat02.txt
        0.3 kB
        Yip Ng
      7. derby2218-trunk-diff01.txt
        2 kB
        Yip Ng
      8. derby2218-trunk-stat01.txt
        0.3 kB
        Yip Ng

        Activity

        Hide
        A B added a comment -

        Issue has not been ported 10.1, but there has been no request to do so. If any such request comes in then the issue can be re-opened. But I'm marking it close for now.

        Show
        A B added a comment - Issue has not been ported 10.1, but there has been no request to do so. If any such request comes in then the issue can be re-opened. But I'm marking it close for now.
        Hide
        A B added a comment -

        Thanks, Yip. I committed derby2218-10.2-diff01.txt with svn revision 510562 and am updating the fixin version accordingly:

        URL: http://svn.apache.org/viewvc?view=rev&rev=510562

        > I might not be able to work on the backport and rerun the regression for 10.1 this week .

        That's fine. I'll just leave the issue in RESOLVED state for now. Thank you for the port to 10.2!

        Show
        A B added a comment - Thanks, Yip. I committed derby2218-10.2-diff01.txt with svn revision 510562 and am updating the fixin version accordingly: URL: http://svn.apache.org/viewvc?view=rev&rev=510562 > I might not be able to work on the backport and rerun the regression for 10.1 this week . That's fine. I'll just leave the issue in RESOLVED state for now. Thank you for the port to 10.2!
        Hide
        A B added a comment -

        Thanks, Yip. I committed derby2218-10.2-diff01.txt with svn revision 510562 and am updating the fixin version accordingly:

        > I might not be able to work on the backport and rerun the regression for 10.1 this week .

        That's fine. I'll just leave the issue in RESOLVED state for now.

        Show
        A B added a comment - Thanks, Yip. I committed derby2218-10.2-diff01.txt with svn revision 510562 and am updating the fixin version accordingly: > I might not be able to work on the backport and rerun the regression for 10.1 this week . That's fine. I'll just leave the issue in RESOLVED state for now.
        Hide
        Yip Ng added a comment -

        derbyall passes for 10.2

        I might not be able to work on the backport and rerun the regression for 10.1 this week since I am busy with something else. Maybe sometime next week.

        Show
        Yip Ng added a comment - derbyall passes for 10.2 I might not be able to work on the backport and rerun the regression for 10.1 this week since I am busy with something else. Maybe sometime next week.
        Hide
        Yip Ng added a comment -

        Running regression for 10.2, will post result when it is completed.

        Show
        Yip Ng added a comment - Running regression for 10.2, will post result when it is completed.
        Hide
        Yip Ng added a comment -

        Patch available for 10.2

        Show
        Yip Ng added a comment - Patch available for 10.2
        Hide
        Yip Ng added a comment -

        Attaching derby2218-10.2-diff01.txt patch for 10.2.

        Show
        Yip Ng added a comment - Attaching derby2218-10.2-diff01.txt patch for 10.2.
        Hide
        Yip Ng added a comment -

        Sure, Army, I will port it to 10.2 and 10.1.

        Show
        Yip Ng added a comment - Sure, Army, I will port it to 10.2 and 10.1.
        Hide
        A B added a comment -

        Resolving as fixed in 10.3. Yip, do you have any interest/desire to port this back to 10.2 and/or 10.1?

        Show
        A B added a comment - Resolving as fixed in 10.3. Yip, do you have any interest/desire to port this back to 10.2 and/or 10.1?
        Hide
        A B added a comment -

        Reopening to set Fixin version.

        Show
        A B added a comment - Reopening to set Fixin version.
        Hide
        A B added a comment -

        Thank you for the follow-up patch, Yip. I ran lang/valuesclause and lang/subquery as a sanity check and then committed *diff03.txt to trunk with svn # 498689:

        URL: http://svn.apache.org/viewvc?view=rev&rev=498689.

        Show
        A B added a comment - Thank you for the follow-up patch, Yip. I ran lang/valuesclause and lang/subquery as a sanity check and then committed *diff03.txt to trunk with svn # 498689: URL: http://svn.apache.org/viewvc?view=rev&rev=498689 .
        Hide
        Yip Ng added a comment -

        Attaching derby2218-trunk-diff03.txt for additional tests + the bindResultColumns call removal.
        derbyall and junit suite passes.

        Show
        Yip Ng added a comment - Attaching derby2218-trunk-diff03.txt for additional tests + the bindResultColumns call removal. derbyall and junit suite passes.
        Hide
        Yip Ng added a comment -

        Thanks Army for taking the time to review the patch. I can certainly post a follow up patch for the additional tests
        you added in this jira.

        Yes, the resultSet.bindResultColumns(fromList) is called to make sure that the result column have its type assigned
        by the expression. This is what I interpreted from the method name of resultSet.bindUntypedNullsToResultColumns(null),
        which looks to me that the RCL should be resolved before calling this method.

        However, it looks like if the RC's type cannot be retrieved from the binding RC, then it will get it from its expression. So we
        may be able to safely remove the extra call to bindResultColumns(). But I'll rerun derbyall and junit suite to make sure
        of that. Will post result here when the regression completes.

        Show
        Yip Ng added a comment - Thanks Army for taking the time to review the patch. I can certainly post a follow up patch for the additional tests you added in this jira. Yes, the resultSet.bindResultColumns(fromList) is called to make sure that the result column have its type assigned by the expression. This is what I interpreted from the method name of resultSet.bindUntypedNullsToResultColumns(null), which looks to me that the RCL should be resolved before calling this method. However, it looks like if the RC's type cannot be retrieved from the binding RC, then it will get it from its expression. So we may be able to safely remove the extra call to bindResultColumns(). But I'll rerun derbyall and junit suite to make sure of that. Will post result here when the regression completes.
        Hide
        A B added a comment -

        Actually, I changed my mind The patch as posted by Yip fixes the problem and doesn't break anything else (Yip reported that derbyall ran without error) so I committed it with svn #498004. Follow-up work to address my review comments can be committed separately.

        Show
        A B added a comment - Actually, I changed my mind The patch as posted by Yip fixes the problem and doesn't break anything else (Yip reported that derbyall ran without error) so I committed it with svn #498004. Follow-up work to address my review comments can be committed separately.
        Hide
        A B added a comment -

        Changed Summary and Description to match the behavior described in my preceding comment.

        Show
        A B added a comment - Changed Summary and Description to match the behavior described in my preceding comment.
        Hide
        A B added a comment -

        I looked at the patch and it seems straightforward--thank you for tracking this down, Yip!

        The test cases all look good-but I did notice that they are limited to IN and EXISTS predicates. Since the only file you changed was SuqbueryNode.java that made me wonder if the problem didn't apply elsewhere, as well. And sure enough, it looks like "values null" will cause a NPE if it appears in any part of the query except the FROM list-without your patch.

        For example, all of the following queries fail with NPEs without your patch but throw the correct error with it:

        – where clause
        select * from t1 where (values null);

        – order by clause
        select * from t1 order by (values null);

        – result column
        select (values null) from t1;

        – group by clause
        select * from t1 group by (values null);

        – having clause
        select * from t1 group by i having (values null);

        So your patch actually fixes more than just IN-list; I wonder if the description for this issue should be changed to reflect that? In any event, do you think it would be useful to add these queries to the test, as well? If so, that can of course come as a follow-up patch (by you or someone else with that fish to fry).

        My only other comment is that with your change we will end up calling:

        resultSet.bindResultColumns(fromList);

        twice for an EXISTS query. Given that derbyall and suites.All all passed I guess this isn't really a problem--but it does kind of look weird in the code and it sort of makes me worry (a little). How certain are we that double-binding is safe thing to do here (I admit, I didn't actually look into the relevant code)? Would it be worth it to add some minimal logic to avoid the double bind? Also, with the patch we call "bindExpressions()" for EXISTS queries when we didn't use to. Again, this is apparently pretty harmless, but I thought I'd mention it...

        If you think the double-binding is safe and that it's not worth adding logic to avoid it, then just let me know and I'll go ahead with the commit. I don't consider anything I've mentioned here to be a blocker for the patch.

        Thank you for picking this issue up and resolving it so quickly!

        Show
        A B added a comment - I looked at the patch and it seems straightforward--thank you for tracking this down, Yip! The test cases all look good- but I did notice that they are limited to IN and EXISTS predicates. Since the only file you changed was SuqbueryNode.java that made me wonder if the problem didn't apply elsewhere, as well. And sure enough, it looks like "values null" will cause a NPE if it appears in any part of the query except the FROM list -without your patch. For example, all of the following queries fail with NPEs without your patch but throw the correct error with it: – where clause select * from t1 where (values null); – order by clause select * from t1 order by (values null); – result column select (values null) from t1; – group by clause select * from t1 group by (values null); – having clause select * from t1 group by i having (values null); So your patch actually fixes more than just IN-list; I wonder if the description for this issue should be changed to reflect that? In any event, do you think it would be useful to add these queries to the test, as well? If so, that can of course come as a follow-up patch (by you or someone else with that fish to fry). My only other comment is that with your change we will end up calling: resultSet.bindResultColumns(fromList); twice for an EXISTS query. Given that derbyall and suites.All all passed I guess this isn't really a problem--but it does kind of look weird in the code and it sort of makes me worry (a little). How certain are we that double-binding is safe thing to do here (I admit, I didn't actually look into the relevant code)? Would it be worth it to add some minimal logic to avoid the double bind? Also, with the patch we call "bindExpressions()" for EXISTS queries when we didn't use to. Again, this is apparently pretty harmless, but I thought I'd mention it... If you think the double-binding is safe and that it's not worth adding logic to avoid it, then just let me know and I'll go ahead with the commit. I don't consider anything I've mentioned here to be a blocker for the patch. Thank you for picking this issue up and resolving it so quickly!
        Hide
        Yip Ng added a comment -

        Attaching patch derby2218-trunk-diff02.txt to resolve the EXISTS subquery issue also. It turns out that the EXISTS subquery will get transform from:

        ... EXISTS (SELECT * FROM t1 ...)

        to

        ... EXISTS (SELECT TRUE FROM t1...)

        So prior this transformation, the bind phase in Subquery node need to check for untyped NULL and throw the appropriate exception.

        derbyall + junit suite passes.

        Show
        Yip Ng added a comment - Attaching patch derby2218-trunk-diff02.txt to resolve the EXISTS subquery issue also. It turns out that the EXISTS subquery will get transform from: ... EXISTS (SELECT * FROM t1 ...) to ... EXISTS (SELECT TRUE FROM t1...) So prior this transformation, the bind phase in Subquery node need to check for untyped NULL and throw the appropriate exception. derbyall + junit suite passes.
        Hide
        Yip Ng added a comment -

        Unchecking patch available. The following should have returned an error instead of executed successfully:

        ij> create table t1 (i int);
        0 rows inserted/updated/deleted
        ij> insert into t1 values 1,2,3;
        3 rows inserted/updated/deleted
        ij> select * from t1 where exists (values null);
        I
        -----------
        1
        2
        3

        3 rows selected

        Show
        Yip Ng added a comment - Unchecking patch available. The following should have returned an error instead of executed successfully: ij> create table t1 (i int); 0 rows inserted/updated/deleted ij> insert into t1 values 1,2,3; 3 rows inserted/updated/deleted ij> select * from t1 where exists (values null); I ----------- 1 2 3 3 rows selected
        Hide
        Yip Ng added a comment -

        Attaching patch derby2218-trunk-diff01.txt. This is a simple fix where it catches untyped null in SubqueryNode at bind phase.

        Show
        Yip Ng added a comment - Attaching patch derby2218-trunk-diff01.txt. This is a simple fix where it catches untyped null in SubqueryNode at bind phase.
        Hide
        Yip Ng added a comment -

        Happens in 10.1.3 too

        2007-01-05 22:55:13.328 GMT Thread[main,5,main] (XID = 115), (SESSIONID = 0), (DATABASE = wombat), (DRDAID = null), Cleanup action starting
        2007-01-05 22:55:13.328 GMT Thread[main,5,main] (XID = 115), (SESSIONID = 0), (DATABASE = wombat), (DRDAID = null), Failed Statement is: select * from t1 where i in (1,2,(values null))
        java.lang.NullPointerException
        at org.apache.derby.impl.sql.compile.SubqueryNode.setDataTypeServices(SubqueryNode.java:2303)
        at org.apache.derby.impl.sql.compile.SubqueryNode.bindExpression(SubqueryNode.java:536)
        at org.apache.derby.impl.sql.compile.ValueNodeList.bindExpression(ValueNodeList.java:137)
        at org.apache.derby.impl.sql.compile.BinaryListOperatorNode.bindExpression(BinaryListOperatorNode.java:164)
        at org.apache.derby.impl.sql.compile.SelectNode.bindExpressions(SelectNode.java:554)
        at org.apache.derby.impl.sql.compile.DMLStatementNode.bindExpressions(DMLStatementNode.java:247)
        at org.apache.derby.impl.sql.compile.DMLStatementNode.bind(DMLStatementNode.java:161)
        at org.apache.derby.impl.sql.compile.ReadCursorNode.bind(ReadCursorNode.java:74)
        at org.apache.derby.impl.sql.compile.CursorNode.bind(CursorNode.java:250)
        at org.apache.derby.impl.sql.GenericStatement.prepMinion(GenericStatement.java:332)
        at org.apache.derby.impl.sql.GenericStatement.prepare(GenericStatement.java:107)
        at org.apache.derby.impl.sql.conn.GenericLanguageConnectionContext.prepareInternalStatement(GenericLanguageConnectionContext.java:704)
        at org.apache.derby.impl.jdbc.EmbedStatement.execute(EmbedStatement.java:513)
        at org.apache.derby.impl.jdbc.EmbedStatement.execute(EmbedStatement.java:487)
        at org.apache.derby.impl.tools.ij.ij.executeImmediate(ij.java:313)
        at org.apache.derby.impl.tools.ij.utilMain.doCatch(utilMain.java:433)
        at org.apache.derby.impl.tools.ij.utilMain.go(utilMain.java:312)
        at org.apache.derby.impl.tools.ij.Main.go(Main.java:203)
        at org.apache.derby.impl.tools.ij.Main.mainCore(Main.java:169)
        at org.apache.derby.impl.tools.ij.Main14.main(Main14.java:55)
        at org.apache.derby.tools.ij.main(ij.java:60)
        Cleanup action completed

        Show
        Yip Ng added a comment - Happens in 10.1.3 too 2007-01-05 22:55:13.328 GMT Thread [main,5,main] (XID = 115), (SESSIONID = 0), (DATABASE = wombat), (DRDAID = null), Cleanup action starting 2007-01-05 22:55:13.328 GMT Thread [main,5,main] (XID = 115), (SESSIONID = 0), (DATABASE = wombat), (DRDAID = null), Failed Statement is: select * from t1 where i in (1,2,(values null)) java.lang.NullPointerException at org.apache.derby.impl.sql.compile.SubqueryNode.setDataTypeServices(SubqueryNode.java:2303) at org.apache.derby.impl.sql.compile.SubqueryNode.bindExpression(SubqueryNode.java:536) at org.apache.derby.impl.sql.compile.ValueNodeList.bindExpression(ValueNodeList.java:137) at org.apache.derby.impl.sql.compile.BinaryListOperatorNode.bindExpression(BinaryListOperatorNode.java:164) at org.apache.derby.impl.sql.compile.SelectNode.bindExpressions(SelectNode.java:554) at org.apache.derby.impl.sql.compile.DMLStatementNode.bindExpressions(DMLStatementNode.java:247) at org.apache.derby.impl.sql.compile.DMLStatementNode.bind(DMLStatementNode.java:161) at org.apache.derby.impl.sql.compile.ReadCursorNode.bind(ReadCursorNode.java:74) at org.apache.derby.impl.sql.compile.CursorNode.bind(CursorNode.java:250) at org.apache.derby.impl.sql.GenericStatement.prepMinion(GenericStatement.java:332) at org.apache.derby.impl.sql.GenericStatement.prepare(GenericStatement.java:107) at org.apache.derby.impl.sql.conn.GenericLanguageConnectionContext.prepareInternalStatement(GenericLanguageConnectionContext.java:704) at org.apache.derby.impl.jdbc.EmbedStatement.execute(EmbedStatement.java:513) at org.apache.derby.impl.jdbc.EmbedStatement.execute(EmbedStatement.java:487) at org.apache.derby.impl.tools.ij.ij.executeImmediate(ij.java:313) at org.apache.derby.impl.tools.ij.utilMain.doCatch(utilMain.java:433) at org.apache.derby.impl.tools.ij.utilMain.go(utilMain.java:312) at org.apache.derby.impl.tools.ij.Main.go(Main.java:203) at org.apache.derby.impl.tools.ij.Main.mainCore(Main.java:169) at org.apache.derby.impl.tools.ij.Main14.main(Main14.java:55) at org.apache.derby.tools.ij.main(ij.java:60) Cleanup action completed

          People

          • Assignee:
            Yip Ng
            Reporter:
            A B
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development