Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 10.0.2.1, 10.1.1.0, 10.2.1.6, 10.3.1.4, 10.4.1.3, 10.5.1.1, 10.6.1.0, 10.7.1.1, 10.8.1.2, 10.9.1.0
    • Fix Version/s: 10.8.3.3, 10.9.2.2, 10.10.1.1
    • Component/s: SQL
    • Environment:
      Windows 7 Netbeans JDBC GUI
    • Urgency:
      Normal
    • Issue & fix info:
      Repro attached
    • Bug behavior facts:
      Wrong query result

      Description

      Hello
      I have a simple database with 100 records.
      I am running a SQL query from Netbeans GUI though JDBC

      This query was generated by Hibernate ORM.

      In certain circumstances the result rowset is not sorting.

      When I use condition morefld2_.mf_id in (5) the result is unsortable.
      When I use condition morefld2_.mf_id in (5,0) the result is sorting properly.

      1. 5933.log
        193 kB
        Dag H. Wanvik
      2. d5933-pof.diff
        0.8 kB
        Dag H. Wanvik
      3. d5933-remap.diff
        0.9 kB
        Knut Anders Hatlen
      4. d5933-remap+test.diff
        3 kB
        Dag H. Wanvik
      5. Helpdesk.zip
        168 kB
        Vlasov Igor
      6. repro.sql
        0.4 kB
        Knut Anders Hatlen
      7. right_sorting.png
        56 kB
        Vlasov Igor
      8. wrong_sorting.png
        56 kB
        Vlasov Igor

        Issue Links

          Activity

          Vlasov Igor created issue -
          Hide
          Vlasov Igor added a comment -

          The query is

          select
          projecttas0_.t_id as t1_4_0_,
          project1_.p_id as p1_3_1_,
          morefld2_.mf_id as mf1_2_2_,
          user3_.usr_id as usr1_9_3_,
          morefld4_.mf_id as mf1_2_4_,
          projecttas0_.t_content as t2_4_0_,
          projecttas0_.t_deadline as t3_4_0_,
          projecttas0_.t_hours as t4_4_0_,
          projecttas0_.mf_level as mf9_4_0_,
          projecttas0_.sys_usr as sys10_4_0_,
          projecttas0_.sys_cre as sys5_4_0_,
          projecttas0_.sys_end as sys6_4_0_,
          projecttas0_.sys_upd as sys7_4_0_,
          projecttas0_.owner_id as owner11_4_0_,
          projecttas0_.p_id as p12_4_0_,
          projecttas0_.t_start as t8_4_0_,
          projecttas0_.mf_status as mf13_4_0_,
          projecttas0_.worker_id as worker14_4_0_,
          project1_.mf_level as mf7_3_1_,
          project1_.p_name as p2_3_1_,
          project1_.sys_usr as sys8_3_1_,
          project1_.sys_cre as sys3_3_1_,
          project1_.sys_end as sys4_3_1_,
          project1_.sys_upd as sys5_3_1_,
          project1_.p_start as p6_3_1_,
          morefld2_.dm_id as dm8_2_2_,
          morefld2_.mf_key as mf2_2_2_,
          morefld2_.mf_name as mf3_2_2_,
          morefld2_.sys_usr as sys9_2_2_,
          morefld2_.sys_cre as sys4_2_2_,
          morefld2_.sys_end as sys5_2_2_,
          morefld2_.sys_upd as sys6_2_2_,
          morefld2_.mf_order as mf7_2_2_,
          user3_.usr_blocked as usr2_9_3_,
          user3_.usr_email as usr3_9_3_,
          user3_.usr_fio as usr4_9_3_,
          user3_.if_user as if5_9_3_,
          user3_.last_login as last6_9_3_,
          user3_.usr_login as usr7_9_3_,
          user3_.sys_usr as sys14_9_3_,
          user3_.sys_cre as sys8_9_3_,
          user3_.sys_end as sys9_9_3_,
          user3_.sys_upd as sys10_9_3_,
          user3_.usr_password as usr11_9_3_,
          user3_.usr_rme_code as usr12_9_3_,
          user3_.usr_emailsend as usr13_9_3_,
          user3_.usr_type as usr15_9_3_,
          morefld4_.dm_id as dm8_2_4_,
          morefld4_.mf_key as mf2_2_4_,
          morefld4_.mf_name as mf3_2_4_,
          morefld4_.sys_usr as sys9_2_4_,
          morefld4_.sys_cre as sys4_2_4_,
          morefld4_.sys_end as sys5_2_4_,
          morefld4_.sys_upd as sys6_2_4_,
          morefld4_.mf_order as mf7_2_4_
          from
          project_task projecttas0_
          inner join
          project project1_
          on projecttas0_.p_id=project1_.p_id
          inner join
          morefld morefld2_
          on projecttas0_.mf_status=morefld2_.mf_id
          left outer join
          user_ user3_
          on projecttas0_.worker_id=user3_.usr_id
          inner join
          morefld morefld4_
          on projecttas0_.mf_level=morefld4_.mf_id
          where
          1=1
          and (
          projecttas0_.sys_end is null
          )
          and (
          morefld2_.mf_id in (
          5
          )
          )
          order by
          projecttas0_.t_id desc fetch first 30 rows only

          Show
          Vlasov Igor added a comment - The query is select projecttas0_.t_id as t1_4_0_, project1_.p_id as p1_3_1_, morefld2_.mf_id as mf1_2_2_, user3_.usr_id as usr1_9_3_, morefld4_.mf_id as mf1_2_4_, projecttas0_.t_content as t2_4_0_, projecttas0_.t_deadline as t3_4_0_, projecttas0_.t_hours as t4_4_0_, projecttas0_.mf_level as mf9_4_0_, projecttas0_.sys_usr as sys10_4_0_, projecttas0_.sys_cre as sys5_4_0_, projecttas0_.sys_end as sys6_4_0_, projecttas0_.sys_upd as sys7_4_0_, projecttas0_.owner_id as owner11_4_0_, projecttas0_.p_id as p12_4_0_, projecttas0_.t_start as t8_4_0_, projecttas0_.mf_status as mf13_4_0_, projecttas0_.worker_id as worker14_4_0_, project1_.mf_level as mf7_3_1_, project1_.p_name as p2_3_1_, project1_.sys_usr as sys8_3_1_, project1_.sys_cre as sys3_3_1_, project1_.sys_end as sys4_3_1_, project1_.sys_upd as sys5_3_1_, project1_.p_start as p6_3_1_, morefld2_.dm_id as dm8_2_2_, morefld2_.mf_key as mf2_2_2_, morefld2_.mf_name as mf3_2_2_, morefld2_.sys_usr as sys9_2_2_, morefld2_.sys_cre as sys4_2_2_, morefld2_.sys_end as sys5_2_2_, morefld2_.sys_upd as sys6_2_2_, morefld2_.mf_order as mf7_2_2_, user3_.usr_blocked as usr2_9_3_, user3_.usr_email as usr3_9_3_, user3_.usr_fio as usr4_9_3_, user3_.if_user as if5_9_3_, user3_.last_login as last6_9_3_, user3_.usr_login as usr7_9_3_, user3_.sys_usr as sys14_9_3_, user3_.sys_cre as sys8_9_3_, user3_.sys_end as sys9_9_3_, user3_.sys_upd as sys10_9_3_, user3_.usr_password as usr11_9_3_, user3_.usr_rme_code as usr12_9_3_, user3_.usr_emailsend as usr13_9_3_, user3_.usr_type as usr15_9_3_, morefld4_.dm_id as dm8_2_4_, morefld4_.mf_key as mf2_2_4_, morefld4_.mf_name as mf3_2_4_, morefld4_.sys_usr as sys9_2_4_, morefld4_.sys_cre as sys4_2_4_, morefld4_.sys_end as sys5_2_4_, morefld4_.sys_upd as sys6_2_4_, morefld4_.mf_order as mf7_2_4_ from project_task projecttas0_ inner join project project1_ on projecttas0_.p_id=project1_.p_id inner join morefld morefld2_ on projecttas0_.mf_status=morefld2_.mf_id left outer join user_ user3_ on projecttas0_.worker_id=user3_.usr_id inner join morefld morefld4_ on projecttas0_.mf_level=morefld4_.mf_id where 1=1 and ( projecttas0_.sys_end is null ) and ( morefld2_.mf_id in ( 5 ) ) order by projecttas0_.t_id desc fetch first 30 rows only
          Vlasov Igor made changes -
          Field Original Value New Value
          Description Hello
          I have a simple database with 100 records.
          I am running a SQL query from Netbeans GUI though JDBC

          This query was generated by Hibernate ORM.

          In certain circumstances the result rowset is not sorting.

           
          Hello
          I have a simple database with 100 records.
          I am running a SQL query from Netbeans GUI though JDBC

          This query was generated by Hibernate ORM.

          In certain circumstances the result rowset is not sorting.

          When I use

           
          Vlasov Igor made changes -
          Description Hello
          I have a simple database with 100 records.
          I am running a SQL query from Netbeans GUI though JDBC

          This query was generated by Hibernate ORM.

          In certain circumstances the result rowset is not sorting.

          When I use

           
          Hello
          I have a simple database with 100 records.
          I am running a SQL query from Netbeans GUI though JDBC

          This query was generated by Hibernate ORM.

          In certain circumstances the result rowset is not sorting.

          When I use condition morefld2_.mf_id in (5) the result is unsortable.
          When I use condition morefld2_.mf_id in (5,0) the result is sorting properly.
              
           
          Vlasov Igor made changes -
          Attachment wrong_sorting.png [ 12545898 ]
          Attachment right_sorting.png [ 12545899 ]
          Hide
          Knut Anders Hatlen added a comment -

          If you could post the schema for the tables involved in the query, including indexes and constraints defined on those tables, it might help others reproduce the problem. For example you could post the output from running the dblook tool on the database. Thanks.

          Show
          Knut Anders Hatlen added a comment - If you could post the schema for the tables involved in the query, including indexes and constraints defined on those tables, it might help others reproduce the problem. For example you could post the output from running the dblook tool on the database. Thanks.
          Hide
          Kathey Marsden added a comment -

          It would also be good to do a consistency check on the database to make sure there is not a problem with an index. http://wiki.apache.org/db-derby/DatabaseConsistencyCheck and even attach the zipped database if he data is not confidential and the database is small.

          Show
          Kathey Marsden added a comment - It would also be good to do a consistency check on the database to make sure there is not a problem with an index. http://wiki.apache.org/db-derby/DatabaseConsistencyCheck and even attach the zipped database if he data is not confidential and the database is small.
          Hide
          Vlasov Igor added a comment -

          sample database

          Show
          Vlasov Igor added a comment - sample database
          Vlasov Igor made changes -
          Attachment Helpdesk.zip [ 12546006 ]
          Hide
          Knut Anders Hatlen added a comment -

          Thanks, Igor. I can reproduce the incorrect ordering on trunk. (I had to disable Derby's authentication checks to try the database, since it is password protected.)

          The consistency checker didn't find any problems with the database.

          The incorrect ordering is also seen in this simpler query:

          select
          projecttas0_.t_id
          from
          project_task projecttas0_
          inner join
          morefld morefld2_
          on projecttas0_.mf_status=morefld2_.mf_id
          left outer join
          user_ user3_
          on projecttas0_.worker_id=user3_.usr_id
          inner join
          morefld morefld4_
          on projecttas0_.mf_level=morefld4_.mf_id
          where
          morefld2_.mf_id = 5
          order by
          projecttas0_.t_id desc;

          Show
          Knut Anders Hatlen added a comment - Thanks, Igor. I can reproduce the incorrect ordering on trunk. (I had to disable Derby's authentication checks to try the database, since it is password protected.) The consistency checker didn't find any problems with the database. The incorrect ordering is also seen in this simpler query: select projecttas0_.t_id from project_task projecttas0_ inner join morefld morefld2_ on projecttas0_.mf_status=morefld2_.mf_id left outer join user_ user3_ on projecttas0_.worker_id=user3_.usr_id inner join morefld morefld4_ on projecttas0_.mf_level=morefld4_.mf_id where morefld2_.mf_id = 5 order by projecttas0_.t_id desc;
          Hide
          Knut Anders Hatlen added a comment -

          Attaching repro.sql, which is a standalone, stripped-down repro for the bug. Here is what I see when I run the script on trunk:

          ij version 10.10
          ij> connect 'jdbc:derby:memory:d5933;create=true';
          ij> create table a (a1 int, a2 int, a3 int, a4 int);
          0 rows inserted/updated/deleted
          ij> create table b (b1 int);
          0 rows inserted/updated/deleted
          ij> create table c (c1 int);
          0 rows inserted/updated/deleted
          ij> create table d (d1 int);
          0 rows inserted/updated/deleted
          ij> insert into a values (1,2,1,2), (2,3,1,3), (1,4,1,4);
          3 rows inserted/updated/deleted
          ij> insert into b values 1;
          1 row inserted/updated/deleted
          ij> insert into d values 2,3,4;
          3 rows inserted/updated/deleted
          ij> select a1
          from
          a inner join b on a3 = b1
          left outer join c on a4 = c1
          inner join d on a2 = d1
          where b1 = 1
          order by a1;
          A1
          -----------
          1
          2
          1

          3 rows selected

          The results come out unordered, even though there is an ORDER BY clause. The bug seems to go all the way back to 10.0.

          Show
          Knut Anders Hatlen added a comment - Attaching repro.sql, which is a standalone, stripped-down repro for the bug. Here is what I see when I run the script on trunk: ij version 10.10 ij> connect 'jdbc:derby:memory:d5933;create=true'; ij> create table a (a1 int, a2 int, a3 int, a4 int); 0 rows inserted/updated/deleted ij> create table b (b1 int); 0 rows inserted/updated/deleted ij> create table c (c1 int); 0 rows inserted/updated/deleted ij> create table d (d1 int); 0 rows inserted/updated/deleted ij> insert into a values (1,2,1,2), (2,3,1,3), (1,4,1,4); 3 rows inserted/updated/deleted ij> insert into b values 1; 1 row inserted/updated/deleted ij> insert into d values 2,3,4; 3 rows inserted/updated/deleted ij> select a1 from a inner join b on a3 = b1 left outer join c on a4 = c1 inner join d on a2 = d1 where b1 = 1 order by a1; A1 ----------- 1 2 1 3 rows selected The results come out unordered, even though there is an ORDER BY clause. The bug seems to go all the way back to 10.0.
          Knut Anders Hatlen made changes -
          Attachment repro.sql [ 12546164 ]
          Knut Anders Hatlen made changes -
          Labels derby_triage10_10
          Affects Version/s 10.8.1.2 [ 12316362 ]
          Affects Version/s 10.7.1.1 [ 12315564 ]
          Affects Version/s 10.6.1.0 [ 12313727 ]
          Affects Version/s 10.5.1.1 [ 12313771 ]
          Affects Version/s 10.4.1.3 [ 12313111 ]
          Affects Version/s 10.3.1.4 [ 12312590 ]
          Affects Version/s 10.2.1.6 [ 11187 ]
          Affects Version/s 10.1.1.0 [ 10993 ]
          Affects Version/s 10.0.2.1 [ 10991 ]
          Urgency Urgent [ 10051 ] Normal [ 10052 ]
          Issue & fix info Repro attached [ 10424 ]
          Hide
          Bryan Pendleton added a comment -

          I think it must be related to the outer join. If you remove the outer join,
          the optimizer tosses a SortResultSet as the top-most result set in the
          query plan, giving the right results:

          select a1
          from
          a inner join b on a3 = b1
          inner join d on a2 = d1
          where b1 = 1
          order by a1;

          A1
          -----------
          1
          1
          2

          3 rows selected

          And if you populate table C with some matching values, and
          then just change "left outer" to "inner" in the query, it again
          gives the correct results:

          insert into c values (3), (4), (2);
          select a1
          from
          a inner join b on a3 = b1
          inner join c on a4 = c1
          inner join d on a2 = d1
          where b1 = 1
          order by a1;

          A1
          -----------
          1
          1
          2

          3 rows selected

          But even with the data present in C, the OUTER JOIN query doesn't perform the sort.

          Actually, if you run repro.sql with -Dderby.language.logQueryPlan=true, you can see
          that if the OUTER JOIN is missing, the optimizer chooses a tree full of table scans,
          nested loop joins, and an outer-most SortResultSet at the end, while with the OUTER
          JOIN in place, the query plan is entirely comprised of HashJoin nodes.

          That is, the two query plans are wildly different, just by changing "left outer" to "inner"
          in the query.

          Not sure if any of this helps, just wanted to share it.

          Show
          Bryan Pendleton added a comment - I think it must be related to the outer join. If you remove the outer join, the optimizer tosses a SortResultSet as the top-most result set in the query plan, giving the right results: select a1 from a inner join b on a3 = b1 inner join d on a2 = d1 where b1 = 1 order by a1; A1 ----------- 1 1 2 3 rows selected And if you populate table C with some matching values, and then just change "left outer" to "inner" in the query, it again gives the correct results: insert into c values (3), (4), (2); select a1 from a inner join b on a3 = b1 inner join c on a4 = c1 inner join d on a2 = d1 where b1 = 1 order by a1; A1 ----------- 1 1 2 3 rows selected But even with the data present in C, the OUTER JOIN query doesn't perform the sort. Actually, if you run repro.sql with -Dderby.language.logQueryPlan=true, you can see that if the OUTER JOIN is missing, the optimizer chooses a tree full of table scans, nested loop joins, and an outer-most SortResultSet at the end, while with the OUTER JOIN in place, the query plan is entirely comprised of HashJoin nodes. That is, the two query plans are wildly different, just by changing "left outer" to "inner" in the query. Not sure if any of this helps, just wanted to share it.
          Hide
          Knut Anders Hatlen added a comment -

          Another observation: The optimizer knows that the predicate B1=1 means that the results are always ordered on B1 (see FromTable.tellRowOrderingAboutConstantColumns()). Although this fact should have no significance for this query, whose results should be ordered on A1 and not on B1, disabling the optimization makes the rows come out in the correct order. This is consistent with Igor's observation that changing the predicate from IN (5) to IN (5,0) made the results ordered. So maybe the sort avoidance code is confusing the two columns for some reason.

          Show
          Knut Anders Hatlen added a comment - Another observation: The optimizer knows that the predicate B1=1 means that the results are always ordered on B1 (see FromTable.tellRowOrderingAboutConstantColumns()). Although this fact should have no significance for this query, whose results should be ordered on A1 and not on B1, disabling the optimization makes the rows come out in the correct order. This is consistent with Igor's observation that changing the predicate from IN (5) to IN (5,0) made the results ordered. So maybe the sort avoidance code is confusing the two columns for some reason.
          Hide
          Dag H. Wanvik added a comment -

          > So maybe the sort avoidance code is confusing the two columns for some reason.

          If so that wouldn't be the first bug in this area, sounds like a good candidate culprit.

          Show
          Dag H. Wanvik added a comment - > So maybe the sort avoidance code is confusing the two columns for some reason. If so that wouldn't be the first bug in this area, sounds like a good candidate culprit.
          Hide
          Dag H. Wanvik added a comment -

          Enclosing dump of the compiler tree after parsing, binding and optimization, "5933.log". It shows that both B1 and A1 columns are labelled
          (4,1), i.e. tableNumber 4, columnNumber 1. In the debugger I see that the sort avoidance code checks and finds that (4,1) is indeed part of comparison with a constant. So, for some reason, the column reference for A1 here gets messed up.

          Show
          Dag H. Wanvik added a comment - Enclosing dump of the compiler tree after parsing, binding and optimization, "5933.log". It shows that both B1 and A1 columns are labelled (4,1), i.e. tableNumber 4, columnNumber 1. In the debugger I see that the sort avoidance code checks and finds that (4,1) is indeed part of comparison with a constant. So, for some reason, the column reference for A1 here gets messed up.
          Dag H. Wanvik made changes -
          Attachment 5933.log [ 12546346 ]
          Hide
          Dag H. Wanvik added a comment -

          Another data point: if I disable join flattening, the result is correct. I suspect the column reference remapping logic, e.g. used during

          PredicateList#remapColumnReferencesToExpressions
          JoinNode#flatten
          FromList#flattenTables
          SelectNode#preprocess

          is flawed.

          Show
          Dag H. Wanvik added a comment - Another data point: if I disable join flattening, the result is correct. I suspect the column reference remapping logic, e.g. used during PredicateList#remapColumnReferencesToExpressions JoinNode#flatten FromList#flattenTables SelectNode#preprocess is flawed.
          Hide
          Dag H. Wanvik added a comment -

          I checked the resulting tree after the preprocess phase of optimization, and the column reference to A1 is indeed already labeled (4,1). The enclosed log file shows this to be the case after end of the optimization phase. The problematic labelling happens during preprocess time, at which time the flattening occurs.

          Show
          Dag H. Wanvik added a comment - I checked the resulting tree after the preprocess phase of optimization, and the column reference to A1 is indeed already labeled (4,1). The enclosed log file shows this to be the case after end of the optimization phase. The problematic labelling happens during preprocess time, at which time the flattening occurs.
          Hide
          Dag H. Wanvik added a comment -

          Looking again, I believe that A1 is correctly labelled (4,1). The column reference to "B1", however, should be (4,5), not (4,1), since B1 is the fifth column in the result set of the left outer join. So now we just need to find out why "B1" ends up with the wrong label after flattening.

          Show
          Dag H. Wanvik added a comment - Looking again, I believe that A1 is correctly labelled (4,1). The column reference to "B1", however, should be (4,5), not (4,1), since B1 is the fifth column in the result set of the left outer join. So now we just need to find out why "B1" ends up with the wrong label after flattening.
          Hide
          Dag H. Wanvik added a comment - - edited

          Uploading an experimental patch which makes the query work.

          I found that the wrong label to the column reference "B1" happened when that expression was pushed into the left outer join node at this stack position:

          .sql.compile.ResultColumn.getColumnPosition(ResultColumn.java:447)
          .sql.compile.ColumnReference.remapColumnReferences(ColumnReference.java:721)
          .sql.compile.RemapCRsVisitor.visit(RemapCRsVisitor.java:75)
          .sql.compile.QueryTreeNode.accept(QueryTreeNode.java:718)
          .sql.compile.BinaryOperatorNode.acceptChildren(BinaryOperatorNode.java:812)
          .sql.compile.QueryTreeNode.accept(QueryTreeNode.java:721)
          .sql.compile.BinaryOperatorNode.acceptChildren(BinaryOperatorNode.java:812)
          .sql.compile.QueryTreeNode.accept(QueryTreeNode.java:721)
          .sql.compile.PredicateList.getPushablePredicates(PredicateList.java:1780)
          .sql.compile.ProjectRestrictNode.pushExpressions(ProjectRestrictNode.java:1095)
          .sql.compile.FromList.pushPredicates(FromList.java:846)
          .sql.compile.SelectNode.preprocess(SelectNode.java:1238)
          ...

          The code in ResultColumn#getColumnPosition looks like this:

          public int getColumnPosition()

          { if (columnDescriptor!=null) return columnDescriptor.getPosition(); else return virtualColumnId; }

          In this case, there is a columnDescriptor present in the RC (maybe because it is also used in the join condition?), but that refers the the base table "B", in which "B1" has column position 1. However, here we are trying to push into the LOJ, in which the real position is 5, the value of virtualColumnId. So, the patch just uses virtualColumnId iff we are sitting on top of a join.

          I tried the patch briefly and it worked for the join and predicate pushdown tests, but I am still very wary of this hack, but I post it now anyway . Running regressions to see what happens.

          Show
          Dag H. Wanvik added a comment - - edited Uploading an experimental patch which makes the query work. I found that the wrong label to the column reference "B1" happened when that expression was pushed into the left outer join node at this stack position: .sql.compile.ResultColumn.getColumnPosition(ResultColumn.java:447) .sql.compile.ColumnReference.remapColumnReferences(ColumnReference.java:721) .sql.compile.RemapCRsVisitor.visit(RemapCRsVisitor.java:75) .sql.compile.QueryTreeNode.accept(QueryTreeNode.java:718) .sql.compile.BinaryOperatorNode.acceptChildren(BinaryOperatorNode.java:812) .sql.compile.QueryTreeNode.accept(QueryTreeNode.java:721) .sql.compile.BinaryOperatorNode.acceptChildren(BinaryOperatorNode.java:812) .sql.compile.QueryTreeNode.accept(QueryTreeNode.java:721) .sql.compile.PredicateList.getPushablePredicates(PredicateList.java:1780) .sql.compile.ProjectRestrictNode.pushExpressions(ProjectRestrictNode.java:1095) .sql.compile.FromList.pushPredicates(FromList.java:846) .sql.compile.SelectNode.preprocess(SelectNode.java:1238) ... The code in ResultColumn#getColumnPosition looks like this: public int getColumnPosition() { if (columnDescriptor!=null) return columnDescriptor.getPosition(); else return virtualColumnId; } In this case, there is a columnDescriptor present in the RC (maybe because it is also used in the join condition?), but that refers the the base table "B", in which "B1" has column position 1. However, here we are trying to push into the LOJ, in which the real position is 5, the value of virtualColumnId. So, the patch just uses virtualColumnId iff we are sitting on top of a join. I tried the patch briefly and it worked for the join and predicate pushdown tests, but I am still very wary of this hack, but I post it now anyway . Running regressions to see what happens.
          Dag H. Wanvik made changes -
          Attachment d5933-pof.diff [ 12546395 ]
          Hide
          Knut Anders Hatlen added a comment -

          Good detective work, Dag!

          I noticed that ColumnReference.remapColumnReferencesToExpressions() had a fix for DERBY-3023 which made it use getVirtualColumnId() instead of getColumnPosition() if the source of the result column was a VirtualColumnNode. remapColumnReferences() does not have this fix.

          Adding a similar fix to remapColumnReferences() (see the attached d5933-remap.diff) also makes the results come out in the right order. It does essentially the same thing as the poc patch, only that it narrows down the change to only affect remapping.

          All the regression tests ran cleanly with the remap patch, FWIW.

          Show
          Knut Anders Hatlen added a comment - Good detective work, Dag! I noticed that ColumnReference.remapColumnReferencesToExpressions() had a fix for DERBY-3023 which made it use getVirtualColumnId() instead of getColumnPosition() if the source of the result column was a VirtualColumnNode. remapColumnReferences() does not have this fix. Adding a similar fix to remapColumnReferences() (see the attached d5933-remap.diff) also makes the results come out in the right order. It does essentially the same thing as the poc patch, only that it narrows down the change to only affect remapping. All the regression tests ran cleanly with the remap patch, FWIW.
          Knut Anders Hatlen made changes -
          Attachment d5933-remap.diff [ 12546506 ]
          Hide
          Bryan Pendleton added a comment -

          Thanks for the pointer to DERBY-3023. It was very helpful to re-read Army's
          comments in that issue, and in the older DERBY-2526. Thank goodness for
          6 years of accumulated JIRA discussions to refer back to!

          I do wonder if, in some future patch, the logic contained in d5933-remap.diff could be
          somehow moved down into the ResultColumn class itself, rather than being
          in its caller.

          But this is incredibly delicate code and I'm not advocating to expand the
          scope of this issue. +1 to the approach outlined in d5933-remap.diff.

          Show
          Bryan Pendleton added a comment - Thanks for the pointer to DERBY-3023 . It was very helpful to re-read Army's comments in that issue, and in the older DERBY-2526 . Thank goodness for 6 years of accumulated JIRA discussions to refer back to! I do wonder if, in some future patch, the logic contained in d5933-remap.diff could be somehow moved down into the ResultColumn class itself, rather than being in its caller. But this is incredibly delicate code and I'm not advocating to expand the scope of this issue. +1 to the approach outlined in d5933-remap.diff.
          Hide
          Dag H. Wanvik added a comment -

          I agree it would be nice to move this logic down, but narrowing the impact as Knut's version does is probably safer.
          Attaching a new version which includes a test case: JoinTest#5933.

          Note the related issues concerning the brittleness of the column renumbering: DERBY-4679 and DERBY-4695.

          Show
          Dag H. Wanvik added a comment - I agree it would be nice to move this logic down, but narrowing the impact as Knut's version does is probably safer. Attaching a new version which includes a test case: JoinTest#5933. Note the related issues concerning the brittleness of the column renumbering: DERBY-4679 and DERBY-4695 .
          Dag H. Wanvik made changes -
          Attachment d5933-remap+test.diff [ 12546532 ]
          Dag H. Wanvik made changes -
          Link This issue is related to DERBY-4695 [ DERBY-4695 ]
          Dag H. Wanvik made changes -
          Link This issue is related to DERBY-3023 [ DERBY-3023 ]
          Dag H. Wanvik made changes -
          Link This issue is related to DERBY-4679 [ DERBY-4679 ]
          Dag H. Wanvik made changes -
          Link This issue is related to DERBY-2526 [ DERBY-2526 ]
          Dag H. Wanvik made changes -
          Issue & fix info Repro attached [ 10424 ] Patch Available,Repro attached [ 10102, 10424 ]
          Hide
          Knut Anders Hatlen added a comment -

          The remap+test patch looks good to me. Thanks for explaining the cause of the bug so clearly in the test comment. +1 to commit.

          As to pushing the logic down, I agree that that would be cleaner. However, it looks like there are parts of the code that expect the current behaviour of ResultColumn.getColumnPosition(). At least, I ran into trouble with some other statements when I attempted to push down the logic without the extra check for JoinNode that Dag put in the poc patch (I suspect that the JoinNode check was what made Dag call that patch a hack in the first place). So in that case I think we should create a new method rather than changing the existing one. And try to explain clearly when to use which method to retrieve the column number/position.

          My main problem with this code is that I don't fully understand the invariants. My first thought was that (columnDescriptor == null) == (expression instanceof VirtualColumnNode) was one, but apparently not. And that makes it more difficult to understand when to use virtualColumnId and when to use columnDescriptor.

          Show
          Knut Anders Hatlen added a comment - The remap+test patch looks good to me. Thanks for explaining the cause of the bug so clearly in the test comment. +1 to commit. As to pushing the logic down, I agree that that would be cleaner. However, it looks like there are parts of the code that expect the current behaviour of ResultColumn.getColumnPosition(). At least, I ran into trouble with some other statements when I attempted to push down the logic without the extra check for JoinNode that Dag put in the poc patch (I suspect that the JoinNode check was what made Dag call that patch a hack in the first place). So in that case I think we should create a new method rather than changing the existing one. And try to explain clearly when to use which method to retrieve the column number/position. My main problem with this code is that I don't fully understand the invariants. My first thought was that (columnDescriptor == null) == (expression instanceof VirtualColumnNode) was one, but apparently not. And that makes it more difficult to understand when to use virtualColumnId and when to use columnDescriptor.
          Hide
          Dag H. Wanvik added a comment -

          Thanks for looking at the test and the further comments, Knut. You are right, the extra test for JoinNode made the fix so ugly I called it a hack
          I have the same problem with the code as you do as far as understanding the conditions under which the VCN has a columnDescriptor.
          I'll see if i can run some tracing over the set of regressions tests to see if I can understand it a little better.

          Committed the patch d5933-remap+test as svn 1390205.

          Show
          Dag H. Wanvik added a comment - Thanks for looking at the test and the further comments, Knut. You are right, the extra test for JoinNode made the fix so ugly I called it a hack I have the same problem with the code as you do as far as understanding the conditions under which the VCN has a columnDescriptor. I'll see if i can run some tracing over the set of regressions tests to see if I can understand it a little better. Committed the patch d5933-remap+test as svn 1390205.
          Dag H. Wanvik made changes -
          Issue & fix info Patch Available,Repro attached [ 10102, 10424 ] Repro attached [ 10424 ]
          Hide
          Bryan Pendleton added a comment -

          Yes, the comment in the test code is very readable and helpful, and the
          references to the associated JIRAs are quite useful. Thanks for taking
          the time to include that information, Dag!

          Show
          Bryan Pendleton added a comment - Yes, the comment in the test code is very readable and helpful, and the references to the associated JIRAs are quite useful. Thanks for taking the time to include that information, Dag!
          Hide
          Kathey Marsden added a comment -

          Is there more work to do on this issue or is fixed? It would be great to get the fix into 10.8.3.

          Show
          Kathey Marsden added a comment - Is there more work to do on this issue or is fixed? It would be great to get the fix into 10.8.3.
          Dag H. Wanvik made changes -
          Assignee Dag H. Wanvik [ dagw ]
          Hide
          Dag H. Wanvik added a comment -

          I ran some trace on the lang suite, and it's pretty common that VCN nodes have a non-null columnDescriptor, so I don't understand the invariant either, Knut. I'll resolve this issue for now, though, since the fix went in some months ago and we haven't seen issues with it AFAIK.

          Show
          Dag H. Wanvik added a comment - I ran some trace on the lang suite, and it's pretty common that VCN nodes have a non-null columnDescriptor, so I don't understand the invariant either, Knut. I'll resolve this issue for now, though, since the fix went in some months ago and we haven't seen issues with it AFAIK.
          Dag H. Wanvik made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Fix Version/s 10.10.0.0 [ 12321550 ]
          Resolution Fixed [ 1 ]
          Hide
          Dag H. Wanvik added a comment -

          Igor, feel free to close the issue if you believe it's been sufficiently addressed. If you can build a Derby version from subversion trunk and test your case with it, that would be optimal. Thanks for uncovering this bug!

          Show
          Dag H. Wanvik added a comment - Igor, feel free to close the issue if you believe it's been sufficiently addressed. If you can build a Derby version from subversion trunk and test your case with it, that would be optimal. Thanks for uncovering this bug!
          Gavin made changes -
          Workflow jira [ 12726255 ] Default workflow, editable Closed status [ 12801967 ]
          Hide
          Kathey Marsden added a comment -

          Assign to myself for backport

          Show
          Kathey Marsden added a comment - Assign to myself for backport
          Kathey Marsden made changes -
          Assignee Dag H. Wanvik [ dagw ] Kathey Marsden [ kmarsden ]
          Hide
          ASF subversion and git services added a comment -

          Commit 1510445 from Kathey Marsden in branch 'code/branches/10.9'
          [ https://svn.apache.org/r1510445 ]

          DERBY-5933 SQL sorting error
          merge from trunk revision 1510360
          Contributed by Dag H.Wanvik

          Show
          ASF subversion and git services added a comment - Commit 1510445 from Kathey Marsden in branch 'code/branches/10.9' [ https://svn.apache.org/r1510445 ] DERBY-5933 SQL sorting error merge from trunk revision 1510360 Contributed by Dag H.Wanvik
          Hide
          ASF subversion and git services added a comment -

          Commit 1510456 from Kathey Marsden in branch 'code/branches/10.8'
          [ https://svn.apache.org/r1510456 ]

          DERBY-5933 SQL sorting error
          merge from trunk revision 1510360
          Contributed by Dag H.Wanvik

          Show
          ASF subversion and git services added a comment - Commit 1510456 from Kathey Marsden in branch 'code/branches/10.8' [ https://svn.apache.org/r1510456 ] DERBY-5933 SQL sorting error merge from trunk revision 1510360 Contributed by Dag H.Wanvik
          Kathey Marsden made changes -
          Assignee Kathey Marsden [ kmarsden ] Dag H. Wanvik [ dagw ]
          Fix Version/s 10.8.3.1 [ 12323475 ]
          Fix Version/s 10.9.2.2 [ 12323562 ]
          Kathey Marsden made changes -
          Link This issue is related to DERBY-6289 [ DERBY-6289 ]

            People

            • Assignee:
              Dag H. Wanvik
              Reporter:
              Vlasov Igor
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development