Derby
  1. Derby
  2. DERBY-4284

All Columns become Nullable when Using left join

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 10.5.1.1
    • Fix Version/s: 10.5.3.1, 10.6.1.0
    • Component/s: JDBC, SQL
    • Labels:
      None
    • Environment:
      Microsoft Windows XP SP3, Sun JDK 6 Update 14
    • Urgency:
      Normal

      Description

      Consider following:-

      create table person (
      id varchar(20) not null,
      name varchar(100) not null
      );

      create table car (
      id varchar(20) not null,
      person_id varchar(20) not null,
      model varchar(100) not null,
      plat_no varchar(100) not null
      );

      When select :-
      select
      p.name,
      c.model,
      c.plat_no
      from person p
      left join car c on (p.id = c.person_id);

      From the ResultSet, get the ResultSetMetaData and inspect each column's isNullable() value, which is always = 1 (always nullable). Expected : column 'p.name' isNullable = 0 (not nullable), but I get 'p.name' isNullable = 1 (nullable)

      1. Main.java
        2 kB
        Dag H. Wanvik
      2. derby-4284-1a.diff
        8 kB
        Knut Anders Hatlen
      3. derby-4284-1b.diff
        12 kB
        Knut Anders Hatlen
      4. derby-4284-1b.stat
        0.5 kB
        Knut Anders Hatlen
      5. derby-4284-1c.diff
        14 kB
        Knut Anders Hatlen
      6. derby-4284-1c.stat
        0.7 kB
        Knut Anders Hatlen
      7. derby-4284-1d.diff
        14 kB
        Knut Anders Hatlen

        Issue Links

          Activity

          Hide
          Dag H. Wanvik added a comment -

          Uploading a repro; Main.java.

          Show
          Dag H. Wanvik added a comment - Uploading a repro; Main.java.
          Hide
          Dag H. Wanvik added a comment -

          Triaged for 10.5.2, checking "repro attached" and setting "normal" urgency.

          Show
          Dag H. Wanvik added a comment - Triaged for 10.5.2, checking "repro attached" and setting "normal" urgency.
          Hide
          Chua Chee Seng added a comment -

          hi Dag H. Wanyik,

          Thanks for attending the issue, our application framework is depending on this thus it becomes show stopper to run our applicaiton on Apache Derby, it will be great if it is going to be fixed in version 10.5.2.

          Thanks!

          Best Regards,
          Chee Seng

          Show
          Chua Chee Seng added a comment - hi Dag H. Wanyik, Thanks for attending the issue, our application framework is depending on this thus it becomes show stopper to run our applicaiton on Apache Derby, it will be great if it is going to be fixed in version 10.5.2. Thanks! Best Regards, Chee Seng
          Hide
          Knut Anders Hatlen added a comment -

          JoinNode.buildRCL() correctly makes all the columns from the right side of the join nullable in a left outer join, and leaves the nullability of the columns on the left side untouched. However, this code in SelectNode.bindResultColumns() later marks all the columns as nullable:

          /* Fix nullability in case of any outer joins in the fromList */
          if (fromList.hasOuterJoins())
          resultColumns.setNullability(true);

          I'm running tests now to see if just removing the code causes any problems. Removing it does give the expected nullability for the query in the description (name non-nullable, model nullable, plat_no nullable).

          Show
          Knut Anders Hatlen added a comment - JoinNode.buildRCL() correctly makes all the columns from the right side of the join nullable in a left outer join, and leaves the nullability of the columns on the left side untouched. However, this code in SelectNode.bindResultColumns() later marks all the columns as nullable: /* Fix nullability in case of any outer joins in the fromList */ if (fromList.hasOuterJoins()) resultColumns.setNullability(true); I'm running tests now to see if just removing the code causes any problems. Removing it does give the expected nullability for the query in the description (name non-nullable, model nullable, plat_no nullable).
          Hide
          Knut Anders Hatlen added a comment -

          Many regression tests failed with the change (2 failures in suites.All, 21 failures in derbyall). I haven't studied the failures in detail, but it looks like most of them, if not all, are caused by differences in ij's formatting when columns are changed from nullable to non-nullable. For instance, a CHAR(2) NOT column is given space for two characters in the output, whereas a CHAR(2) column is given space for four characters (to allow NULL to be printed). I believe this difference is to be expected.

          The code below illustrates how ij formats nullable and non-nullable columns differently:

          ij> create table t(col1 char(2) not null, col2 char(2));
          0 rows inserted/updated/deleted
          ij> select * from t;
          C&|COL2
          -------

          0 rows selected

          Show
          Knut Anders Hatlen added a comment - Many regression tests failed with the change (2 failures in suites.All, 21 failures in derbyall). I haven't studied the failures in detail, but it looks like most of them, if not all, are caused by differences in ij's formatting when columns are changed from nullable to non-nullable. For instance, a CHAR(2) NOT column is given space for two characters in the output, whereas a CHAR(2) column is given space for four characters (to allow NULL to be printed). I believe this difference is to be expected. The code below illustrates how ij formats nullable and non-nullable columns differently: ij> create table t(col1 char(2) not null, col2 char(2)); 0 rows inserted/updated/deleted ij> select * from t; C&|COL2 ------- 0 rows selected
          Hide
          Knut Anders Hatlen added a comment -

          I'm attaching a patch with the code changes I believe are necessary to fix this bug. It also contains a regression test which verifies that the nullability is as expected in a left outer join and in a right outer join.

          I'll post an updated patch which contains the necessary updates to the test canons once I've gone through all the test failures and verified that they are expected. However, I'm posting this minimal patch now to make it easier for reviewers to see the actual code changes.

          The patch is not ready for commit due to the regression test failures seen with it.

          Show
          Knut Anders Hatlen added a comment - I'm attaching a patch with the code changes I believe are necessary to fix this bug. It also contains a regression test which verifies that the nullability is as expected in a left outer join and in a right outer join. I'll post an updated patch which contains the necessary updates to the test canons once I've gone through all the test failures and verified that they are expected. However, I'm posting this minimal patch now to make it easier for reviewers to see the actual code changes. The patch is not ready for commit due to the regression test failures seen with it.
          Hide
          Knut Anders Hatlen added a comment -

          At least one of the test failures shows an actual problem. store/xaOffline1.sql defines the following view as a right outer join between two diagnostic tables:

          create view lock_table as
          select
          cast(username as char(8)) as username,
          cast(t.type as char(8)) as trantype,
          cast(l.type as char(8)) as type,
          cast(lockcount as char(3)) as cnt,
          mode,
          cast(tablename as char(12)) as tabname,
          cast(lockname as char(10)) as lockname,
          state,
          status
          from
          syscs_diag.lock_table l right outer join syscs_diag.transaction_table t
          on l.xid = t.xid where l.tableType <> 'S' and t.type='UserTransaction';

          Without the patch, all the columns returned by the view are nullable. That's not correct. However, with the patch, the columns CNT, TABNAME and LOCKNAME are incorrectly reported as non-nullable. Since those columns come from the left side of a right outer join, they should be nullable.

          Show
          Knut Anders Hatlen added a comment - At least one of the test failures shows an actual problem. store/xaOffline1.sql defines the following view as a right outer join between two diagnostic tables: create view lock_table as select cast(username as char(8)) as username, cast(t.type as char(8)) as trantype, cast(l.type as char(8)) as type, cast(lockcount as char(3)) as cnt, mode, cast(tablename as char(12)) as tabname, cast(lockname as char(10)) as lockname, state, status from syscs_diag.lock_table l right outer join syscs_diag.transaction_table t on l.xid = t.xid where l.tableType <> 'S' and t.type='UserTransaction'; Without the patch, all the columns returned by the view are nullable. That's not correct. However, with the patch, the columns CNT, TABNAME and LOCKNAME are incorrectly reported as non-nullable. Since those columns come from the left side of a right outer join, they should be nullable.
          Hide
          Knut Anders Hatlen added a comment -

          The problematic query in store/xaOffline1.sql was also used in many other tests, so most of the test failures were caused by that problem.

          I'm uploading a new patch, still not ready for commit, which updates the canons for the tests that had only the expected changes in the output (nist/dml148.sql, nist/dml162.sql, lang/outerjoin.sql).

          Show
          Knut Anders Hatlen added a comment - The problematic query in store/xaOffline1.sql was also used in many other tests, so most of the test failures were caused by that problem. I'm uploading a new patch, still not ready for commit, which updates the canons for the tests that had only the expected changes in the output (nist/dml148.sql, nist/dml162.sql, lang/outerjoin.sql).
          Hide
          Knut Anders Hatlen added a comment -

          The problem seems to be related to the cast. Here's some code that exposes the problem without using the diagnostic tables:

          ij> create table t1 (x varchar(10) not null);
          0 rows inserted/updated/deleted
          ij> create table t2 (y varchar(10) not null);
          0 rows inserted/updated/deleted
          ij> create view v1 as select x,y from t1 left outer join t2 on 1=1;
          0 rows inserted/updated/deleted
          ij> describe v1;
          COLUMN_NAME |TYPE_NAME|DEC&|NUM&|COLUM&|COLUMN_DEF|CHAR_OCTE&|IS_NULL&
          ------------------------------------------------------------------------------
          X |VARCHAR |NULL|NULL|10 |NULL |20 |NO
          Y |VARCHAR |NULL|NULL|10 |NULL |20 |YES

          2 rows selected
          ij> create view v2 as select x,cast(y as char(2))y from t1 left outer join t2 on 1=1;
          0 rows inserted/updated/deleted
          ij> describe v2;
          COLUMN_NAME |TYPE_NAME|DEC&|NUM&|COLUM&|COLUMN_DEF|CHAR_OCTE&|IS_NULL&
          ------------------------------------------------------------------------------
          X |VARCHAR |NULL|NULL|10 |NULL |20 |NO
          Y |CHAR |NULL|NULL|2 |NULL |4 |NO

          2 rows selected

          Y is correctly reported as nullable without the cast, but it is reported as non-nullable if it is cast to CHAR(2).

          Without the patch, both X and Y are nullable, which is also wrong. Only Y should be nullable in these two queries.

          Show
          Knut Anders Hatlen added a comment - The problem seems to be related to the cast. Here's some code that exposes the problem without using the diagnostic tables: ij> create table t1 (x varchar(10) not null); 0 rows inserted/updated/deleted ij> create table t2 (y varchar(10) not null); 0 rows inserted/updated/deleted ij> create view v1 as select x,y from t1 left outer join t2 on 1=1; 0 rows inserted/updated/deleted ij> describe v1; COLUMN_NAME |TYPE_NAME|DEC&|NUM&|COLUM&|COLUMN_DEF|CHAR_OCTE&|IS_NULL& ------------------------------------------------------------------------------ X |VARCHAR |NULL|NULL|10 |NULL |20 |NO Y |VARCHAR |NULL|NULL|10 |NULL |20 |YES 2 rows selected ij> create view v2 as select x,cast(y as char(2))y from t1 left outer join t2 on 1=1; 0 rows inserted/updated/deleted ij> describe v2; COLUMN_NAME |TYPE_NAME|DEC&|NUM&|COLUM&|COLUMN_DEF|CHAR_OCTE&|IS_NULL& ------------------------------------------------------------------------------ X |VARCHAR |NULL|NULL|10 |NULL |20 |NO Y |CHAR |NULL|NULL|2 |NULL |4 |NO 2 rows selected Y is correctly reported as nullable without the cast, but it is reported as non-nullable if it is cast to CHAR(2). Without the patch, both X and Y are nullable, which is also wrong. Only Y should be nullable in these two queries.
          Hide
          Knut Anders Hatlen added a comment -

          Here's a new patch which addresses the remaining issues. It also adds more test cases. All the regression tests ran cleanly with the patch.

          Description of the changes:

          SelectNode.java:

          • bindResultColumns(): Don't make all the columns nullable just because the query contains an outer join.

          FromList.java:

          • Removed helper method hasOuterJoins() which is no longer used after the changes in SelectNode.

          JoinNode.java:

          • getMatchingColumn(): If the join is a "half" outer join, and the matching column is found on the logical right side of the join (logical right side == left side in a right outer join), make that column nullable before returning it. Although JoinNode already has code in buildRCL() that makes all columns on the logical right side nullable, getMatchingColumn() may be called before the RCL has been built, which causes the problem with casts mentioned in the previous comment.

          lang/JoinTest.java:
          lang/_Suite.java:

          • Test cases for this bug.

          dml148.out:
          dml162.out:
          outerjoin.out:

          • Updated master with formatting changes caused by nullable column becoming non-nullable.

          wisconsin.out:

          • Updated printed query plan for an outer join because a scan stopped using "ordered null sematics" on a column now that it sees that it's not nullable.
          Show
          Knut Anders Hatlen added a comment - Here's a new patch which addresses the remaining issues. It also adds more test cases. All the regression tests ran cleanly with the patch. Description of the changes: SelectNode.java: bindResultColumns(): Don't make all the columns nullable just because the query contains an outer join. FromList.java: Removed helper method hasOuterJoins() which is no longer used after the changes in SelectNode. JoinNode.java: getMatchingColumn(): If the join is a "half" outer join, and the matching column is found on the logical right side of the join (logical right side == left side in a right outer join), make that column nullable before returning it. Although JoinNode already has code in buildRCL() that makes all columns on the logical right side nullable, getMatchingColumn() may be called before the RCL has been built, which causes the problem with casts mentioned in the previous comment. lang/JoinTest.java: lang/_Suite.java: Test cases for this bug. dml148.out: dml162.out: outerjoin.out: Updated master with formatting changes caused by nullable column becoming non-nullable. wisconsin.out: Updated printed query plan for an outer join because a scan stopped using "ordered null sematics" on a column now that it sees that it's not nullable.
          Hide
          Bryan Pendleton added a comment -

          That wisconsin.out change is intriguing. I vaguely remember a situation several years back when we updated the query plan in the ref file due to an ordered null semantics diff, and we couldn't figure out what had caused it, or why. Unfortunately I can't remember more of the details, but I'm hopeful that with this latest change we now have a tighter grip on the nullability of result columns. Thanks for investigating these issues in such detail!

          Show
          Bryan Pendleton added a comment - That wisconsin.out change is intriguing. I vaguely remember a situation several years back when we updated the query plan in the ref file due to an ordered null semantics diff, and we couldn't figure out what had caused it, or why. Unfortunately I can't remember more of the details, but I'm hopeful that with this latest change we now have a tighter grip on the nullability of result columns. Thanks for investigating these issues in such detail!
          Hide
          Knut Anders Hatlen added a comment -

          Thanks for that hint, Bryan. It looks like my changes to wisconsin.out are the exact reverse of the following commit:

          ------------------------------------------------------------------------
          r562113 | djd | 2007-08-02 16:10:47 +0200 (to, 02 aug 2007) | 2 lines

          DERBY-2916 Fix wisconsin canon file due to changes that allowed a valid optimization to the plan.

          ------------------------------------------------------------------------

          If the root cause of DERBY-2916 is found and fixed, it will hopefully be possible to revert the changes I have suggested for JoinNode.getMatchingColumn(), as it feels somewhat dirty to have to set the nullability two different places (both buildRCL() and getMatchingColumn()). For the record, I did try to move setting the nullability from buildRCL(), which is called from bindResultColumn(), to bindExpressions() which is called before bindResultColumn() and also before the cast node is bound. That seemed to fix the issues for most queries, but for multi-level nested outer joins the left/right child result sets hadn't always initialized their RCLs at that time, so that broke down.

          I didn't find any single place to put the nullability code so that it was executed sufficiently early so that all nodes on top of it would see the correct nullability, and at the same time sufficiently late so that the child result sets were properly bound. Though I must admit I don't fully understand JoinNode's bind logic with bindExpressions() followed by bindResultColumns() again followed by deferredBindExpressions().

          It sounds very likely that the problem with the cast node not seeing the correct nullability is caused by the DERBY-2775 changes. Before DERBY-2775, when the nullability of the result column in the join node was changed, the change in nullability would automatically cascade up to the cast node since it had a reference to the exact same data type descriptor.

          Show
          Knut Anders Hatlen added a comment - Thanks for that hint, Bryan. It looks like my changes to wisconsin.out are the exact reverse of the following commit: ------------------------------------------------------------------------ r562113 | djd | 2007-08-02 16:10:47 +0200 (to, 02 aug 2007) | 2 lines DERBY-2916 Fix wisconsin canon file due to changes that allowed a valid optimization to the plan. ------------------------------------------------------------------------ If the root cause of DERBY-2916 is found and fixed, it will hopefully be possible to revert the changes I have suggested for JoinNode.getMatchingColumn(), as it feels somewhat dirty to have to set the nullability two different places (both buildRCL() and getMatchingColumn()). For the record, I did try to move setting the nullability from buildRCL(), which is called from bindResultColumn(), to bindExpressions() which is called before bindResultColumn() and also before the cast node is bound. That seemed to fix the issues for most queries, but for multi-level nested outer joins the left/right child result sets hadn't always initialized their RCLs at that time, so that broke down. I didn't find any single place to put the nullability code so that it was executed sufficiently early so that all nodes on top of it would see the correct nullability, and at the same time sufficiently late so that the child result sets were properly bound. Though I must admit I don't fully understand JoinNode's bind logic with bindExpressions() followed by bindResultColumns() again followed by deferredBindExpressions(). It sounds very likely that the problem with the cast node not seeing the correct nullability is caused by the DERBY-2775 changes. Before DERBY-2775 , when the nullability of the result column in the join node was changed, the change in nullability would automatically cascade up to the cast node since it had a reference to the exact same data type descriptor.
          Hide
          Knut Anders Hatlen added a comment -

          The 1d patch updates the comment in JoinNode.getMatchingColumn() and explains that it's a workaround for the problem logged as DERBY-2916.

          I now feel confident that the patch is ok, and plan to commit it shortly. The changes in SelectNode and FromList look rather obvious, I think. The change in JoinNode didn't feel quite right, but now that we have found that it's another manifestation of DERBY-2916, I think it is fine to leave the workaround there and add a note to DERBY-2916 saying that it should be removed once the underlying cause has been found and fixed.

          Also, the proposed patch moves the workaround for DERBY-2916 from the test canons to the product code, which I think is a slight improvement, even though the best solution would be to fix the root cause of DERBY-2916.

          Show
          Knut Anders Hatlen added a comment - The 1d patch updates the comment in JoinNode.getMatchingColumn() and explains that it's a workaround for the problem logged as DERBY-2916 . I now feel confident that the patch is ok, and plan to commit it shortly. The changes in SelectNode and FromList look rather obvious, I think. The change in JoinNode didn't feel quite right, but now that we have found that it's another manifestation of DERBY-2916 , I think it is fine to leave the workaround there and add a note to DERBY-2916 saying that it should be removed once the underlying cause has been found and fixed. Also, the proposed patch moves the workaround for DERBY-2916 from the test canons to the product code, which I think is a slight improvement, even though the best solution would be to fix the root cause of DERBY-2916 .
          Hide
          Knut Anders Hatlen added a comment -

          Committed to trunk with revision 810860. I'll leave the issue open until the fix has been back-ported to 10.5.

          Show
          Knut Anders Hatlen added a comment - Committed to trunk with revision 810860. I'll leave the issue open until the fix has been back-ported to 10.5.
          Hide
          Knut Anders Hatlen added a comment -

          Merged to the 10.5 branch and committed revision 812536.

          Show
          Knut Anders Hatlen added a comment - Merged to the 10.5 branch and committed revision 812536.

            People

            • Assignee:
              Knut Anders Hatlen
              Reporter:
              Chua Chee Seng
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development