Derby
  1. Derby
  2. DERBY-1644

NPE when inserting values to a table that has a column declared as generated by default as identity

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 10.2.1.6
    • Fix Version/s: 10.3.1.4
    • Component/s: SQL
    • Labels:
      None
    • Environment:
      Sun JDK 1.4.2

      Description

      The following scenario triggers a NullPointerException in statement compilation:

      ij> create table t1 (c1 int, c2 int generated by default as identity);
      0 rows inserted/updated/deleted
      ij> insert into t1 (c2) values default, 10;
      ERROR XJ001: Java exception: ': java.lang.NullPointerException'.

      Stacktrace from derby.log:

      Database Class Loader started - derby.database.classpath=''
      2006-08-04 06:31:17.235 GMT Thread[main,5,main] (XID = 235), (SESSIONID = 0), (DATABASE = wombat), (DRDAID = null), Cleanup action starting
      2006-08-04 06:31:17.235 GMT Thread[main,5,main] (XID = 235), (SESSIONID = 0), (DATABASE = wombat), (DRDAID = null), Failed Statement is: insert into t1 (c2) values default, 10
      java.lang.NullPointerException
      at org.apache.derby.impl.sql.compile.ResultColumnList.generateCore(ResultColumnList.java:1033)
      at org.apache.derby.impl.sql.compile.ResultColumnList.generate(ResultColumnList.java:893)
      at org.apache.derby.impl.sql.compile.RowResultSetNode.generate(RowResultSetNode.java:690)
      at org.apache.derby.impl.sql.compile.UnionNode.generate(UnionNode.java:589)
      at org.apache.derby.impl.sql.compile.ProjectRestrictNode.generateMinion(ProjectRestrictNode.java:1424)
      at org.apache.derby.impl.sql.compile.ProjectRestrictNode.generate(ProjectRestrictNode.java:1286)
      at org.apache.derby.impl.sql.compile.NormalizeResultSetNode.generate(NormalizeResultSetNode.java:122)
      at org.apache.derby.impl.sql.compile.InsertNode.generate(InsertNode.java:764)
      at org.apache.derby.impl.sql.compile.StatementNode.generate(StatementNode.java:232)
      at org.apache.derby.impl.sql.GenericStatement.prepMinion(GenericStatement.java:477)
      at org.apache.derby.impl.sql.GenericStatement.prepare(GenericStatement.java:118)
      at org.apache.derby.impl.sql.conn.GenericLanguageConnectionContext.prepareInternalStatement(GenericLanguageConnectionContext.java:713)
      at org.apache.derby.impl.jdbc.EmbedStatement.execute(EmbedStatement.java:567)
      at org.apache.derby.impl.jdbc.EmbedStatement.execute(EmbedStatement.java:516)
      at org.apache.derby.impl.tools.ij.ij.executeImmediate(ij.java:313)
      at org.apache.derby.impl.tools.ij.utilMain.doCatch(utilMain.java:478)
      at org.apache.derby.impl.tools.ij.utilMain.runScriptGuts(utilMain.java:347)
      at org.apache.derby.impl.tools.ij.utilMain.go(utilMain.java:248)
      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:69)

      sysinfo:

      ------------------ Java Information ------------------
      Java Version: 1.4.2_12
      Java Vendor: Sun Microsystems Inc.
      Java home: C:\jdk142\jre
      Java classpath: classes;.
      OS name: Windows XP
      OS architecture: x86
      OS version: 5.1
      Java user name: yip
      Java user home: C:\Documents and Settings\Administrator
      Java user dir: C:\derby\trunk
      java.specification.name: Java Platform API Specification
      java.specification.version: 1.4
      --------- Derby Information --------
      JRE - JDBC: J2SE 1.4.2 - JDBC 3.0
      [C:\derby\trunk\classes] 10.2.0.5 alpha - (1)
      ------------------------------------------------------
      ----------------- Locale Information -----------------
      Current Locale : [English/United States [en_US]]
      Found support for locale: [de_DE]
      version: 10.2.0.5 alpha - (1)
      Found support for locale: [es]
      version: 10.2.0.5 alpha - (1)
      Found support for locale: [fr]
      version: 10.2.0.5 alpha - (1)
      Found support for locale: [it]
      version: 10.2.0.5 alpha - (1)
      Found support for locale: [ja_JP]
      version: 10.2.0.5 alpha - (1)
      Found support for locale: [ko_KR]
      version: 10.2.0.5 alpha - (1)
      Found support for locale: [pt_BR]
      version: 10.2.0.5 alpha - (1)
      Found support for locale: [zh_CN]
      version: 10.2.0.5 alpha - (1)
      Found support for locale: [zh_TW]
      version: 10.2.0.5 alpha - (1)
      ------------------------------------------------------

      1. d1644_combined_v3.diff
        21 kB
        Bryan Pendleton
      2. RSN_EnhanceRCL_Simplify_v2.diff
        6 kB
        Bryan Pendleton
      3. RSNCommentFixup_v1.diff
        2 kB
        Bryan Pendleton
      4. d1644_recursivelyCheck_v1.diff
        13 kB
        Bryan Pendleton

        Issue Links

          Activity

          Hide
          Bryan Pendleton added a comment -

          I think that the issue here involves mixing "default" with constant values.

          Note that the quite-similar-but-not-identical statements below all work fine:

          insert into t1 (c2) values (10);
          insert into t1 (c2) values default;
          insert into t1 (c2) values (default);
          insert into t1 (c2) values 10, 20, 30;

          However, this statement gets the same NPE as the one in the description:

          insert into t1 (c2) values 40, 50, default;

          So it appears to be that the bug involves:

          • inserting multiple rows with a single INSERT statement using multiple values clauses, and
          • some of the rows have numeric constant values, while others fire the identity field generator
          Show
          Bryan Pendleton added a comment - I think that the issue here involves mixing "default" with constant values. Note that the quite-similar-but-not-identical statements below all work fine: insert into t1 (c2) values (10); insert into t1 (c2) values default; insert into t1 (c2) values (default); insert into t1 (c2) values 10, 20, 30; However, this statement gets the same NPE as the one in the description: insert into t1 (c2) values 40, 50, default; So it appears to be that the bug involves: inserting multiple rows with a single INSERT statement using multiple values clauses, and some of the rows have numeric constant values, while others fire the identity field generator
          Hide
          A B added a comment -

          I was running the above queries for kicks to see if I could reproduce the problem-but they all succeeded. I then realized that my test table "t1" only had one column-the "generated by default" column c2. When I recreated the table so that it had two columns (c1 and c2 as posted in the repro) the NPE occurs.

          So the bug appears to require that there be at least one non-identity column in the table, as well. Maybe that's useful, maybe not...

          Show
          A B added a comment - I was running the above queries for kicks to see if I could reproduce the problem- but they all succeeded. I then realized that my test table "t1" only had one column -the "generated by default" column c2. When I recreated the table so that it had two columns (c1 and c2 as posted in the repro) the NPE occurs. So the bug appears to require that there be at least one non-identity column in the table, as well. Maybe that's useful, maybe not...
          Hide
          Bryan Pendleton added a comment -

          Thanks Army! I think that your observation is crucial, and here's why:

          The problem arises due to this code in InsertNode.bind():

          if (! inOrder || resultSet.resultColumns.size() < numTableColumns)

          { // one thing we do know is that all of the resultsets underneath // us have their resultColumn names filled in with the names of // the target table columns. That makes generating the mapping // "easier" -- we simply generate the names of the target table columns // that are included. For the missing columns, we generate default // value expressions. resultSet = resultSet.enhanceRCLForInsert(numTableColumns, colMap, dataDictionary, targetTableDescriptor, targetVTI); }

          if (resultSet instanceof UnionNode)

          { // If we are inserting a number of rows in VALUES clause, we need to // examine each row for 'autoincrement'. resultColumnList.checkAutoincrementUnion(resultSet); }

          else resultColumnList.checkAutoincrement(resultSet.getResultColumns());

          When the VALUES clause encounters multiple rows, it generates a UNION node tree to
          combine the rows to be inserted. The code above notices the top-level UNION node
          and calls the special checkAutoincrementUnion() method which knows how to
          recursively traverse the Union tree and call checkAutoIncrement() on the underlying
          RowResultSetNode instances at the leaf level of the tree.

          HOWEVER, when the number of columns in the rows in the VALUES clause is a subset of
          the number of columns in the table we're inserting into, the top node of the tree
          is not a UnionNode, but is rather a ProjectRestrictNode. This means that we skip past
          the UnionNode test and just call checkAutoincrement(), which processes the PRN but doesn't
          go down to the RowResultSetNode(s) at the leaf level.

          This leaves the ResultColumn instance at the leaf level with a NULL column descriptor,
          which causes the NPE during the code generation phase.

          And, there is a second, related problem. The enhanceRCLForInsert() call is also only made
          at the top level of the tree. However, this call is a necessary pre-condition for calling
          checkAutoincrement() because enhanceRCLForInsert() ensures that the proper ResultColumnList
          values are in place prior to the checkAutoincrement() reconciliation of the column lists.

          I believe that the way to solve this is to merge the above code from InsertNode.bind
          together with the current recursive processing in ResultColumnList.checkAutoincrementUnion()
          to produce a new recursive routine, which I have called enhanceAndCheckForAutoincrement(),
          which will recursively traverse the ResultSet tree, calling both enhanceRCLForInsert()
          and checkAutoincrement() on the various nodes in the tree.

          That way, even if the RowResultSetNode instances are buried within UnionNode trees, they
          will still be correctly set up for autoincrement processing during bind processing.

          I'll attach a complete patch to this Jira issue once I finish writing a bunch of tests
          and tidying up the code changes.

          Show
          Bryan Pendleton added a comment - Thanks Army! I think that your observation is crucial, and here's why: The problem arises due to this code in InsertNode.bind(): if (! inOrder || resultSet.resultColumns.size() < numTableColumns) { // one thing we do know is that all of the resultsets underneath // us have their resultColumn names filled in with the names of // the target table columns. That makes generating the mapping // "easier" -- we simply generate the names of the target table columns // that are included. For the missing columns, we generate default // value expressions. resultSet = resultSet.enhanceRCLForInsert(numTableColumns, colMap, dataDictionary, targetTableDescriptor, targetVTI); } if (resultSet instanceof UnionNode) { // If we are inserting a number of rows in VALUES clause, we need to // examine each row for 'autoincrement'. resultColumnList.checkAutoincrementUnion(resultSet); } else resultColumnList.checkAutoincrement(resultSet.getResultColumns()); When the VALUES clause encounters multiple rows, it generates a UNION node tree to combine the rows to be inserted. The code above notices the top-level UNION node and calls the special checkAutoincrementUnion() method which knows how to recursively traverse the Union tree and call checkAutoIncrement() on the underlying RowResultSetNode instances at the leaf level of the tree. HOWEVER, when the number of columns in the rows in the VALUES clause is a subset of the number of columns in the table we're inserting into, the top node of the tree is not a UnionNode, but is rather a ProjectRestrictNode. This means that we skip past the UnionNode test and just call checkAutoincrement(), which processes the PRN but doesn't go down to the RowResultSetNode(s) at the leaf level. This leaves the ResultColumn instance at the leaf level with a NULL column descriptor, which causes the NPE during the code generation phase. And, there is a second, related problem. The enhanceRCLForInsert() call is also only made at the top level of the tree. However, this call is a necessary pre-condition for calling checkAutoincrement() because enhanceRCLForInsert() ensures that the proper ResultColumnList values are in place prior to the checkAutoincrement() reconciliation of the column lists. I believe that the way to solve this is to merge the above code from InsertNode.bind together with the current recursive processing in ResultColumnList.checkAutoincrementUnion() to produce a new recursive routine, which I have called enhanceAndCheckForAutoincrement(), which will recursively traverse the ResultSet tree, calling both enhanceRCLForInsert() and checkAutoincrement() on the various nodes in the tree. That way, even if the RowResultSetNode instances are buried within UnionNode trees, they will still be correctly set up for autoincrement processing during bind processing. I'll attach a complete patch to this Jira issue once I finish writing a bunch of tests and tidying up the code changes.
          Hide
          Bryan Pendleton added a comment -

          Attached is d1644_recursivelyCheck_v1.diff, a proposed patch to fix this issue.

          This patch moves the method checkAutoincrementUnion() from
          ResultColumnList into InsertNode, renames the method to
          enhanceAndCheckForAutoincrement(), and updates the recursive
          processing so that it:
          a) can handle PRN nodes in the ResultSet tree
          b) knows to call enhanceRCLForInsert on ResultSetNodes
          c) can recurse on all sorts of ResultSet trees, not just those
          headed by a UnionNode.

          The patch also contains some new tests.

          The patch passes derbyall and suites.All, and I'd love to hear your feedback on it.

          Show
          Bryan Pendleton added a comment - Attached is d1644_recursivelyCheck_v1.diff, a proposed patch to fix this issue. This patch moves the method checkAutoincrementUnion() from ResultColumnList into InsertNode, renames the method to enhanceAndCheckForAutoincrement(), and updates the recursive processing so that it: a) can handle PRN nodes in the ResultSet tree b) knows to call enhanceRCLForInsert on ResultSetNodes c) can recurse on all sorts of ResultSet trees, not just those headed by a UnionNode. The patch also contains some new tests. The patch passes derbyall and suites.All, and I'd love to hear your feedback on it.
          Hide
          Bryan Pendleton added a comment -

          I think there is room for a follow-on patch in this area, to ResultSetNode.enhanceRCLForInsert.

          The Javadoc comments for this method suggest that this is the place where PRN nodes
          can get inserted into the ResultSet node tree, but in fact this method does not do this.
          I suspect that perhaps it once did, and it could be that when that processing was moved
          elsewhere, that is when DERBY-1644 arose.

          I think it would be useful to have a follow-on patch to ResultSetNode, which would have
          no functional changes, but would improve the clarity of the code, to do the following:

          1) Change the return type of enhanceRCLForInsert from ResultSetNode to void
          2) Remove the "return this" statement
          3) Remove the unused variable numResultSetColumns
          4) Fix the Javadoc for this method to remove the suggest that it may insert a PRN
          into the ResultSet tree.

          I shall attach such a follow-on patch for review.

          Show
          Bryan Pendleton added a comment - I think there is room for a follow-on patch in this area, to ResultSetNode.enhanceRCLForInsert. The Javadoc comments for this method suggest that this is the place where PRN nodes can get inserted into the ResultSet node tree, but in fact this method does not do this. I suspect that perhaps it once did, and it could be that when that processing was moved elsewhere, that is when DERBY-1644 arose. I think it would be useful to have a follow-on patch to ResultSetNode, which would have no functional changes, but would improve the clarity of the code, to do the following: 1) Change the return type of enhanceRCLForInsert from ResultSetNode to void 2) Remove the "return this" statement 3) Remove the unused variable numResultSetColumns 4) Fix the Javadoc for this method to remove the suggest that it may insert a PRN into the ResultSet tree. I shall attach such a follow-on patch for review.
          Hide
          Bryan Pendleton added a comment -

          RSNCommentFixup_v1.diff, which must be applied after the recursivelyCheck patch,
          is an optional second part to the patch which, I believe, improves the clarity of
          ResultSetNode.enhanceRCLForInsert().

          I would appreciate any feedback about whether this patch is worth including as part of this change.

          Show
          Bryan Pendleton added a comment - RSNCommentFixup_v1.diff, which must be applied after the recursivelyCheck patch, is an optional second part to the patch which, I believe, improves the clarity of ResultSetNode.enhanceRCLForInsert(). I would appreciate any feedback about whether this patch is worth including as part of this change.
          Hide
          Bryan Pendleton added a comment -

          Hmmm... I think that second patch is mis-considered. I see now that
          SetOperatorNode.enhanceRCLForInsert overrides ResultSetNode.enhanceRCLForInsert,
          and the SetOperatorNode version does return a new result set node.

          I think this means that I still don't understand this problem well enough.

          Feedback on the patch(es) is most welcome, but they are definitely not ready for commit.

          Show
          Bryan Pendleton added a comment - Hmmm... I think that second patch is mis-considered. I see now that SetOperatorNode.enhanceRCLForInsert overrides ResultSetNode.enhanceRCLForInsert, and the SetOperatorNode version does return a new result set node. I think this means that I still don't understand this problem well enough. Feedback on the patch(es) is most welcome, but they are definitely not ready for commit.
          Hide
          Bryan Pendleton added a comment -

          I think the method SetOperatorNode.enhanceRCLForInsert can
          be wholly deleted, leaving only the implementation in ResultSetNode.
          I shall pursue this possibility.

          Show
          Bryan Pendleton added a comment - I think the method SetOperatorNode.enhanceRCLForInsert can be wholly deleted, leaving only the implementation in ResultSetNode. I shall pursue this possibility.
          Hide
          Bryan Pendleton added a comment -

          After doing some more studying and testing, I believe that it is in fact
          safe to remove SetOperatorNode.enhanceRCLForInsert() entirely,
          and to simply ResultSetNode.enhanceRCLForInsert() as described
          in my previous comment. There seems to be no code path which can
          all SetOperatorNode.enhanceRCLForInsert().

          Attached is RSN_EnhanceRCL_Simplify_v2.diff, which is as I said an
          optional part of this fix, but I think it is nice to delete code if it is clearly
          unnecessary because that simplifies the overall implementation.

          Please let me know your feedback.

          Show
          Bryan Pendleton added a comment - After doing some more studying and testing, I believe that it is in fact safe to remove SetOperatorNode.enhanceRCLForInsert() entirely, and to simply ResultSetNode.enhanceRCLForInsert() as described in my previous comment. There seems to be no code path which can all SetOperatorNode.enhanceRCLForInsert(). Attached is RSN_EnhanceRCL_Simplify_v2.diff, which is as I said an optional part of this fix, but I think it is nice to delete code if it is clearly unnecessary because that simplifies the overall implementation. Please let me know your feedback.
          Hide
          A B added a comment -

          Hi Bryan,

          I applied the d1644_recursivelyCheck_v1.diff patch to my local codeline and it applied cleanly. However, when I tried to run lang/autoincrement.sql the test failed due to an ArrayIndexOutOfBounds exception. I was able to reproduce the error on a clean database by executing the following (pulled from that test):

          ij> create table ai_test (x int generated always as identity (start with 2, increment by 2), y int);
          0 rows inserted/updated/deleted
          ij> insert into ai_test values (1),(2);
          ERROR XJ001: Java exception: '1 >= 1: java.lang.ArrayIndexOutOfBoundsException'.
          java.lang.ArrayIndexOutOfBoundsException: 1 >= 1
          at java.util.Vector.elementAt(Vector.java(Compiled Code))
          at org.apache.derby.impl.sql.compile.QueryTreeNodeVector.elementAt(QueryTreeNodeVector.java:52)
          at org.apache.derby.impl.sql.compile.ResultColumnList.checkStorableExpressions(ResultColumnList.java:890)
          at org.apache.derby.impl.sql.compile.InsertNode.bind(InsertNode.java:419)
          at org.apache.derby.impl.sql.GenericStatement.prepMinion(GenericStatement.java:345)
          at org.apache.derby.impl.sql.GenericStatement.prepare(GenericStatement.java:119)
          at org.apache.derby.impl.sql.conn.GenericLanguageConnectionContext.prepareInternalStatement(GenericLanguageConnectionContext.java:742)
          at org.apache.derby.impl.jdbc.EmbedStatement.execute(EmbedStatement.java:568)
          at org.apache.derby.impl.jdbc.EmbedStatement.execute(EmbedStatement.java:517)
          at org.apache.derby.impl.tools.ij.ij.executeImmediate(ij.java:321)
          at org.apache.derby.impl.tools.ij.utilMain.doCatch(utilMain.java:517)
          at org.apache.derby.impl.tools.ij.utilMain.runScriptGuts(utilMain.java:370)
          at org.apache.derby.impl.tools.ij.utilMain.go(utilMain.java:268)
          at org.apache.derby.impl.tools.ij.Main.go(Main.java:204)
          at org.apache.derby.impl.tools.ij.Main.mainCore(Main.java:170)
          at org.apache.derby.impl.tools.ij.Main14.main(Main14.java:56)
          at org.apache.derby.tools.ij.main(ij.java:71)

          To see if it made any difference I also applied RSN_EnhanceRCL_Simplify_v2.diff to my codeline. When I did that things started working correctly again. This surprised me since I was under the impression that this second patch was optional--i.e. nice but not required. Am I overlooking something here, or is it in fact true that both patches must be applied in order for your fix to be complete? I didn't look into it at all but if you could give a quick explanation of why the second patch is required, that'd be great.

          In any event, having applied both patches together I was able to run lang/autoincrement.sql without problems. I also verified that without the changes the new tests fail with an NPE, as expected.

          My one minor suggestion is to perhaps add two more test cases to your current scenario:

          insert into D1644 (c2) values default, default, default, default;
          insert into D1644 (c2, c1) values (default, 128), (default, 129), (default, 131);

          The above two INSERTs work as expected when both of your patches have been applied, so this is not a requirement. But it might be nice to have the extra test cases, just for safety...

          Show
          A B added a comment - Hi Bryan, I applied the d1644_recursivelyCheck_v1.diff patch to my local codeline and it applied cleanly. However, when I tried to run lang/autoincrement.sql the test failed due to an ArrayIndexOutOfBounds exception. I was able to reproduce the error on a clean database by executing the following (pulled from that test): ij> create table ai_test (x int generated always as identity (start with 2, increment by 2), y int); 0 rows inserted/updated/deleted ij> insert into ai_test values (1),(2); ERROR XJ001: Java exception: '1 >= 1: java.lang.ArrayIndexOutOfBoundsException'. java.lang.ArrayIndexOutOfBoundsException: 1 >= 1 at java.util.Vector.elementAt(Vector.java(Compiled Code)) at org.apache.derby.impl.sql.compile.QueryTreeNodeVector.elementAt(QueryTreeNodeVector.java:52) at org.apache.derby.impl.sql.compile.ResultColumnList.checkStorableExpressions(ResultColumnList.java:890) at org.apache.derby.impl.sql.compile.InsertNode.bind(InsertNode.java:419) at org.apache.derby.impl.sql.GenericStatement.prepMinion(GenericStatement.java:345) at org.apache.derby.impl.sql.GenericStatement.prepare(GenericStatement.java:119) at org.apache.derby.impl.sql.conn.GenericLanguageConnectionContext.prepareInternalStatement(GenericLanguageConnectionContext.java:742) at org.apache.derby.impl.jdbc.EmbedStatement.execute(EmbedStatement.java:568) at org.apache.derby.impl.jdbc.EmbedStatement.execute(EmbedStatement.java:517) at org.apache.derby.impl.tools.ij.ij.executeImmediate(ij.java:321) at org.apache.derby.impl.tools.ij.utilMain.doCatch(utilMain.java:517) at org.apache.derby.impl.tools.ij.utilMain.runScriptGuts(utilMain.java:370) at org.apache.derby.impl.tools.ij.utilMain.go(utilMain.java:268) at org.apache.derby.impl.tools.ij.Main.go(Main.java:204) at org.apache.derby.impl.tools.ij.Main.mainCore(Main.java:170) at org.apache.derby.impl.tools.ij.Main14.main(Main14.java:56) at org.apache.derby.tools.ij.main(ij.java:71) To see if it made any difference I also applied RSN_EnhanceRCL_Simplify_v2.diff to my codeline. When I did that things started working correctly again. This surprised me since I was under the impression that this second patch was optional--i.e. nice but not required. Am I overlooking something here, or is it in fact true that both patches must be applied in order for your fix to be complete? I didn't look into it at all but if you could give a quick explanation of why the second patch is required, that'd be great. In any event, having applied both patches together I was able to run lang/autoincrement.sql without problems. I also verified that without the changes the new tests fail with an NPE, as expected. My one minor suggestion is to perhaps add two more test cases to your current scenario: insert into D1644 (c2) values default, default, default, default; insert into D1644 (c2, c1) values (default, 128), (default, 129), (default, 131); The above two INSERTs work as expected when both of your patches have been applied, so this is not a requirement. But it might be nice to have the extra test cases, just for safety...
          Hide
          Bryan Pendleton added a comment -

          Thanks for the review Army! I had both patches together in my sandbox and thought
          I had separated them cleanly, but it seems that they got hooked together.

          I shall go study this some more.

          Show
          Bryan Pendleton added a comment - Thanks for the review Army! I had both patches together in my sandbox and thought I had separated them cleanly, but it seems that they got hooked together. I shall go study this some more.
          Hide
          Bryan Pendleton added a comment -

          I'm not quite sure what I was thinking when I thought that the two code
          changes could be separated, but they cannot. They are linked and really
          it needs to be a single patch proposal.

          Attached is d1644_combined_v3.diff, a patch proposal which combines
          the two previous code change patches. The new patch also adds the
          new tests suggested by Army.

          There are several ideas involved in this patch:

          • When an INSERT statement will insert multiple rows from the VALUES
            clause, the compiler will compile the various values into a tree of
            UnionNodes with RowResultSetNodes at the leaves of the three
          • The columns specified in the INSERT statement may be a subset
            of the rows in the table. The "extra" columns need to be constructed by
            the INSERT statement, either by generating NULL values for those
            columns which are nullable, or by compiling a default values for those
            columns which have DEFAULT values, or by generating a value for an IDENTITY
            column which is GENERATED. The work of constructing these extra
            column values is done by genNewRCForInsert.
          • For columns which are GENERATED ALWAYS, we must make sure that
            the INSERT statement doesn't allow the user to insert their own value for
            the generated column.
          • The columns which are specified in the INSERT column spec may not match
            the order in which the columns arise in the table. Therefore, the column
            values may need to be re-ordered by the INSERT statement so that they
            occur in the proper order.
          • In the case when the ResultSet which provides the values for the INSERT
            statement is not just a single node, but is rather a tree of UnionNodes, the
            above processing needs to happen throughout the tree, not just at the root node.

          The combined patch proposal accomplishes all of this.

          There is one aspect to this patch that I can't fully explain yet. If somebody knows
          the reason behind it, that would be very nice; I am hoping to continue studying
          this in the near future. This question involves the ProjectRestrictNode at the
          root of the ResultSetNode tree. With the current code, when the VALUES clause
          results in multiple rows, and is compiled into a UnionNode tree with
          RowResultSetNodes at the leaves, InsertNode.bind calls SetOperatorNode.
          enhanceRCLForInsert(), which constructs a brand-new ProjectRestrictNode to
          be the new root of the UnionNode tree. With my patch proposal, the PRN is
          no longer built; the ResultSetNode tree is left with a UnionNode at the root.
          From what I can tell, this works fine: the PRN is no longer needed once we are
          sure to generate and re-order the ResultSet columns at all nodes in the
          ResultSetNode tree. The new rows can be inserted directly from the UnionNodes.

          I think this patch is correct, but it is possible that there is some other aspect to
          having the PRN at the root of the tree which I haven't discovered yet, which will
          cause a problem if we don't generate that PRN. If anybody knows what such a
          problem might be, please let me know.

          Show
          Bryan Pendleton added a comment - I'm not quite sure what I was thinking when I thought that the two code changes could be separated, but they cannot. They are linked and really it needs to be a single patch proposal. Attached is d1644_combined_v3.diff, a patch proposal which combines the two previous code change patches. The new patch also adds the new tests suggested by Army. There are several ideas involved in this patch: When an INSERT statement will insert multiple rows from the VALUES clause, the compiler will compile the various values into a tree of UnionNodes with RowResultSetNodes at the leaves of the three The columns specified in the INSERT statement may be a subset of the rows in the table. The "extra" columns need to be constructed by the INSERT statement, either by generating NULL values for those columns which are nullable, or by compiling a default values for those columns which have DEFAULT values, or by generating a value for an IDENTITY column which is GENERATED. The work of constructing these extra column values is done by genNewRCForInsert. For columns which are GENERATED ALWAYS, we must make sure that the INSERT statement doesn't allow the user to insert their own value for the generated column. The columns which are specified in the INSERT column spec may not match the order in which the columns arise in the table. Therefore, the column values may need to be re-ordered by the INSERT statement so that they occur in the proper order. In the case when the ResultSet which provides the values for the INSERT statement is not just a single node, but is rather a tree of UnionNodes, the above processing needs to happen throughout the tree, not just at the root node. The combined patch proposal accomplishes all of this. There is one aspect to this patch that I can't fully explain yet. If somebody knows the reason behind it, that would be very nice; I am hoping to continue studying this in the near future. This question involves the ProjectRestrictNode at the root of the ResultSetNode tree. With the current code, when the VALUES clause results in multiple rows, and is compiled into a UnionNode tree with RowResultSetNodes at the leaves, InsertNode.bind calls SetOperatorNode. enhanceRCLForInsert(), which constructs a brand-new ProjectRestrictNode to be the new root of the UnionNode tree. With my patch proposal, the PRN is no longer built; the ResultSetNode tree is left with a UnionNode at the root. From what I can tell, this works fine: the PRN is no longer needed once we are sure to generate and re-order the ResultSet columns at all nodes in the ResultSetNode tree. The new rows can be inserted directly from the UnionNodes. I think this patch is correct, but it is possible that there is some other aspect to having the PRN at the root of the tree which I haven't discovered yet, which will cause a problem if we don't generate that PRN. If anybody knows what such a problem might be, please let me know.
          Hide
          A B added a comment -

          > Attached is d1644_combined_v3.diff, a patch proposal which combines the two previous code change patches.

          I will try to review the latest patch sometime today...

          Show
          A B added a comment - > Attached is d1644_combined_v3.diff, a patch proposal which combines the two previous code change patches. I will try to review the latest patch sometime today...
          Hide
          A B added a comment -

          Thank you for the updated patch, Bryan. I was able to apply it cleanly to trunk and I ran the lang/autoincrement.sql test without any problems. Code comments look good and the changes agree with the explanation of the problem/solution that you provided for this issue.

          As for your question about the PRN over the UnioNode, I agree that the PRN seems unnecessary based on my understanding of the changes. As you mentioned in a previous comment, prior to your changes the enhanceRCLForInsert() call was only made at the top of the tree. In the case of a UnionNode the code did the "enhancing" by generating a PRN over the UnionNode, where the PRN itself had the "enhanced" result columns. The rest of the tree was then left untouched. But since, as you have said, your changes make it so that the entire tree beneath the UnionNode (including the UnionNode itself) is "enhanced", the additional PRN is no longer required.

          So basically, I agree with your analysis and I think your solution looks good. Assuming, of course, that derbyall ran cleanly?

          I should say, though, that after re-reading the code comments for enhanceRCLForInsert() method several times, these lines still have me scratching my head:

          • Those RSNs whose generate() method does not handle projects will
          • insert a PRN, with a new RCL which matches the target RCL, above
          • the current RSN.

          I see that you removed these lines in your patch so that the comment matches the current state of the code, which is good. But I'm having problems figuring out what the lines were saying to begin with. In particular, what does it mean to say "those RSNs whose generate() method does not handle projects"?

          In any event, your changes make sense and the basic tests I've run all pass, so I too tend to think the patch is correct. Assuming derbyall runs cleanly, I vote +1 to commit.

          Show
          A B added a comment - Thank you for the updated patch, Bryan. I was able to apply it cleanly to trunk and I ran the lang/autoincrement.sql test without any problems. Code comments look good and the changes agree with the explanation of the problem/solution that you provided for this issue. As for your question about the PRN over the UnioNode, I agree that the PRN seems unnecessary based on my understanding of the changes. As you mentioned in a previous comment, prior to your changes the enhanceRCLForInsert() call was only made at the top of the tree. In the case of a UnionNode the code did the "enhancing" by generating a PRN over the UnionNode, where the PRN itself had the "enhanced" result columns. The rest of the tree was then left untouched. But since, as you have said, your changes make it so that the entire tree beneath the UnionNode (including the UnionNode itself) is "enhanced", the additional PRN is no longer required. So basically, I agree with your analysis and I think your solution looks good. Assuming, of course, that derbyall ran cleanly? I should say, though, that after re-reading the code comments for enhanceRCLForInsert() method several times, these lines still have me scratching my head: Those RSNs whose generate() method does not handle projects will insert a PRN, with a new RCL which matches the target RCL, above the current RSN. I see that you removed these lines in your patch so that the comment matches the current state of the code, which is good. But I'm having problems figuring out what the lines were saying to begin with. In particular, what does it mean to say "those RSNs whose generate() method does not handle projects"? In any event, your changes make sense and the basic tests I've run all pass, so I too tend to think the patch is correct. Assuming derbyall runs cleanly, I vote +1 to commit.
          Hide
          Yip Ng added a comment -

          Per Bryan's request, I will be reviewing this patch also. Hopefully I can give some feedback tomorrow.

          Show
          Yip Ng added a comment - Per Bryan's request, I will be reviewing this patch also. Hopefully I can give some feedback tomorrow.
          Hide
          Yip Ng added a comment -

          I wasn't able to apply the latest patch, d1644_combined_v3.diff cleanly. It has a problem patching ResultColumnList.java, so I have to resolve this manually. The patch fixes the stated problem of this jira and without it, it fails. I have reviewed the code changes and they all look reasonable to me. (Great comments and thanks for cleaning up the code to make it clearer for reviewers/contributers). I also ran derbyall + junit suite and they all pass. +1 to commit.

          Show
          Yip Ng added a comment - I wasn't able to apply the latest patch, d1644_combined_v3.diff cleanly. It has a problem patching ResultColumnList.java, so I have to resolve this manually. The patch fixes the stated problem of this jira and without it, it fails. I have reviewed the code changes and they all look reasonable to me. (Great comments and thanks for cleaning up the code to make it clearer for reviewers/contributers). I also ran derbyall + junit suite and they all pass. +1 to commit.
          Hide
          Bryan Pendleton added a comment -

          Thank you very much Army and Yip for the reviews. I'll bring the patch up to date and look into
          proceeding with some final testing and commit.

          Show
          Bryan Pendleton added a comment - Thank you very much Army and Yip for the reviews. I'll bring the patch up to date and look into proceeding with some final testing and commit.
          Hide
          Bryan Pendleton added a comment -

          Committed d1644_combined_v3.diff to the trunk as revision 487414.

          I haven't marked this issue resolved yet because I suspect we may
          want to investigate merging these changes back to the 10.2 branch. Yip,
          what do you think? Should we merge these changes back to 10.2?

          Show
          Bryan Pendleton added a comment - Committed d1644_combined_v3.diff to the trunk as revision 487414. I haven't marked this issue resolved yet because I suspect we may want to investigate merging these changes back to the 10.2 branch. Yip, what do you think? Should we merge these changes back to 10.2?
          Hide
          Bryan Pendleton added a comment -

          After thinking about it some more I decided to mark this issue as resolved,
          since the change is in the trunk. If we later decide that we want to port this
          change to one or more of the older branches, we can re-open the issue then.

          Show
          Bryan Pendleton added a comment - After thinking about it some more I decided to mark this issue as resolved, since the change is in the trunk. If we later decide that we want to port this change to one or more of the older branches, we can re-open the issue then.
          Hide
          Yip Ng added a comment -

          <snip>

          Hi Bryan, I think if there is a need, we can always merge back the fix
          to 10.2. So let's mark this issue as resolved for now.

          Regards,
          Yip Ng

          Show
          Yip Ng added a comment - <snip> Hi Bryan, I think if there is a need, we can always merge back the fix to 10.2. So let's mark this issue as resolved for now. Regards, Yip Ng
          Hide
          Bryan Pendleton added a comment -

          Revision 487656 is a follow-on commit with a 1-line fix to a javadoc typo
          parameter targetTD should have been targetTableDescriptor.

          Show
          Bryan Pendleton added a comment - Revision 487656 is a follow-on commit with a 1-line fix to a javadoc typo parameter targetTD should have been targetTableDescriptor.
          Hide
          Dyre Tjeldvoll added a comment -

          This issue is resolved and has not been updated in the last 12 months (since 24/Jan/07).

          Show
          Dyre Tjeldvoll added a comment - This issue is resolved and has not been updated in the last 12 months (since 24/Jan/07).

            People

            • Assignee:
              Bryan Pendleton
              Reporter:
              Yip Ng
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development