Derby
  1. Derby
  2. DERBY-3155

Support for SQL:2003 MERGE statement

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 10.11.0.0
    • Component/s: SQL
    • Urgency:
      Normal
    • Issue & fix info:
      Patch Available

      Description

      A relatively common piece of logic in a database application is to check for a row's existence and then either update or insert depending on its existence.

      SQL:2003 added a MERGE statement to perform this operation. It looks like this:

      MERGE INTO table_name USING table_name ON (condition)
      WHEN MATCHED THEN UPDATE SET column1 = value1 [, column2 = value2 ...]
      WHEN NOT MATCHED THEN INSERT column1 [, column2 ...] VALUES (value1 [, value2 ...])

      At the moment, the only workaround for this would be to write a stored procedure to do the same operation, or to implement the logic client-side.

      1. derby-3155-53-aa-transitionSimpleColumn.diff
        3 kB
        Rick Hillegas
      2. MergeStatement.html
        11 kB
        Rick Hillegas
      3. derby-3155-52-aa-upgrade.diff
        4 kB
        Rick Hillegas
      4. derby-3155-51-aa-cleanup2.diff
        7 kB
        Rick Hillegas
      5. derby-3155-50-aa-revampDeleteThenRows.diff
        15 kB
        Rick Hillegas
      6. derby-3155-49-aa-cleanup1.diff
        34 kB
        Rick Hillegas
      7. derby-3155-48-aa-indexScan.diff
        7 kB
        Rick Hillegas
      8. derby-3155-47-aa-collations.diff
        6 kB
        Rick Hillegas
      9. derby-3155-46-aa-deferredDeletes.diff
        8 kB
        Rick Hillegas
      10. derby-3155-45-aa-serialization.diff
        10 kB
        Rick Hillegas
      11. derby-3155-44-aa-lobsInTriggers.diff
        23 kB
        Rick Hillegas
      12. derby-3155-43-aa-eliminateDuplicateColumnRefs.diff
        2 kB
        Rick Hillegas
      13. derby-3155-42-aa-triggersAndGeneratedColumns.diff
        8 kB
        Rick Hillegas
      14. derby-3155-41-aa-nullGeneratedColumns.diff
        4 kB
        Rick Hillegas
      15. derby-3155-40-aa-bigLobs.diff
        16 kB
        Rick Hillegas
      16. derby-3155-39-aa-fixBuild.diff
        2 kB
        Rick Hillegas
      17. derby-3155-38-aa-datatypes.diff
        49 kB
        Rick Hillegas
      18. derby-3155-37-aa-printSubNodes.diff
        4 kB
        Rick Hillegas
      19. derby-3155-36-aa-lockModeComment.diff
        1.0 kB
        Rick Hillegas
      20. derby-3155-35-aa-allPrivsTest.diff
        14 kB
        Rick Hillegas
      21. derby-3155-34-ab-updatePrivs.diff
        22 kB
        Rick Hillegas
      22. derby-3155-34-aa-updatePrivs.diff
        20 kB
        Rick Hillegas
      23. derby-3155-33-ab-insertPrivs.diff
        21 kB
        Rick Hillegas
      24. derby-3155-32-aa-newTestFunction.diff
        0.7 kB
        Rick Hillegas
      25. derby-3155-31-aa-deletePrivs.diff
        34 kB
        Rick Hillegas
      26. derby-3155-30-ab-moreCorrelationNames.diff
        34 kB
        Rick Hillegas
      27. derby-3155-29-aa-missingSchema.diff
        4 kB
        Rick Hillegas
      28. MergeStatement.html
        10 kB
        Rick Hillegas
      29. derby-3155-28-aa-cardinalityViolations.diff
        13 kB
        Rick Hillegas
      30. derby-3155-27-aa-adjustMatchingRefinements.diff
        8 kB
        Rick Hillegas
      31. derby-3155-26-aa-copyRowLocationForIndexScans.diff
        8 kB
        Rick Hillegas
      32. derby-3155-25-aa-parametersAsInsertValues.diff
        6 kB
        Rick Hillegas
      33. derby-3155-24-aa-supportParameters.diff
        3 kB
        Rick Hillegas
      34. derby-3155-23-aa-forbidDerivedColumnLists.diff
        5 kB
        Rick Hillegas
      35. derby-3155-22-ad-testIdentifiersOnLeftSideOfSetClauses.diff
        3 kB
        Rick Hillegas
      36. derby-3155-21-ac-cleanupAndForbidSynonyms.diff
        17 kB
        Rick Hillegas
      37. derby-3155-20-aa-reworkColumnMatching.diff
        30 kB
        Rick Hillegas
      38. derby-3155-19-aa-forbidSubqueriesInMatchedClauses.diff
        8 kB
        Rick Hillegas
      39. derby-3155-18-aa-basicView.diff
        3 kB
        Rick Hillegas
      40. derby-3155-17-aa-serializingRowLocations.diff
        3 kB
        Rick Hillegas
      41. derby-3155-16-aa-treatCurrentRowLocationNodeLikeBaseColumnNode.diff
        4 kB
        Rick Hillegas
      42. derby-3155-15-aa-replumbMergeResultSetCleanup.diff
        4 kB
        Rick Hillegas
      43. derby-3155-14-aa-replaceCorrelationNamesOnLeftSideOfSETclauses.diff
        4 kB
        Rick Hillegas
      44. derby-3155-13-aa-allowSystemAndTempTables.diff
        5 kB
        Rick Hillegas
      45. derby-3155-12-aa-canOmitInsertColumnList.diff
        5 kB
        Rick Hillegas
      46. derby-3155-11-ab-beforeTriggersCantFireMerge.diff
        6 kB
        Rick Hillegas
      47. derby-3155-10-aa-correlationNames.diff
        19 kB
        Rick Hillegas
      48. derby-3155-09-aa-correlationNames.diff
        20 kB
        Rick Hillegas
      49. derby-3155-08-ah-updateAction.diff
        94 kB
        Rick Hillegas
      50. derby-3155-07-ad-insertAction.diff
        114 kB
        Rick Hillegas
      51. derby-3155-06-aa-triggerTransitionTableAsSource.diff
        10 kB
        Rick Hillegas
      52. derby-3155-05-aa-triggerTransitionTableAsTarget.diff
        2 kB
        Rick Hillegas
      53. derby-3155-04-af-deleteAction.diff
        140 kB
        Rick Hillegas
      54. derby-3155-03-ah-backingStoreHashtableWithRowLocation.diff
        48 kB
        Rick Hillegas
      55. derby-3155-03-ag-backingStoreHashtableWithRowLocation.diff
        48 kB
        Rick Hillegas
      56. derby-3155-04-ae-deleteAction.diff
        135 kB
        Rick Hillegas
      57. derby-3155-03-af-backingStoreHashtableWithRowLocation.diff
        47 kB
        Rick Hillegas
      58. derby-3155-03-ae-backingStoreHashtableWithRowLocation.diff
        37 kB
        Rick Hillegas
      59. derby-3155-02-ag-fixParserWarning.diff
        0.6 kB
        Rick Hillegas
      60. MergeStatement.html
        10 kB
        Rick Hillegas
      61. MergeStatement.html
        10 kB
        Rick Hillegas
      62. derby-3155-01-ac-grammar.diff
        59 kB
        Rick Hillegas
      63. MergeStatement.html
        10 kB
        Rick Hillegas

        Issue Links

          Activity

          Trejkaz created issue -
          Hide
          Denis Assanbaev (RD-Software GmbH) added a comment -

          I find this feature usefull for also for other cases:
          currently derby cannot provide an update using data from another table.

          UPDATE XXX t_u
          set (col1, col2, col3)
          = (select col_n1,col_n2,col_n3
          from XXX t_n
          where t_n.id = t_u.id).
          CURRENT OF- version doesn't help too.

          UPDATE XXX t_u
          set col1
          = (select col_n1
          from XXX t_n
          where t_n.id = t_u.id),
          col2
          = (select col_n2
          from XXX t_n
          where t_n.id = t_u.id).
          works, but it results in a lot of updates or performance boosts when
          using for a table with ~ 100 columns.

          Regards


          Mit freundlichen Grüßen / Kind Regards

          Denis

          Show
          Denis Assanbaev (RD-Software GmbH) added a comment - I find this feature usefull for also for other cases: currently derby cannot provide an update using data from another table. UPDATE XXX t_u set (col1, col2, col3) = (select col_n1,col_n2,col_n3 from XXX t_n where t_n.id = t_u.id). CURRENT OF- version doesn't help too. UPDATE XXX t_u set col1 = (select col_n1 from XXX t_n where t_n.id = t_u.id), col2 = (select col_n2 from XXX t_n where t_n.id = t_u.id). works, but it results in a lot of updates or performance boosts when using for a table with ~ 100 columns. Regards – Mit freundlichen Grüßen / Kind Regards Denis
          Dag H. Wanvik made changes -
          Field Original Value New Value
          Issue Type New Feature [ 2 ] Improvement [ 4 ]
          Hide
          Jarek Jarcec Cecho added a comment -

          I would also vote for including this feature.

          Jarcec

          Show
          Jarek Jarcec Cecho added a comment - I would also vote for including this feature. Jarcec
          Mamta A. Satoor made changes -
          Labels derby_triage10_10
          Urgency Normal [ 10052 ]
          Knut Anders Hatlen made changes -
          Link This issue is related to DERBY-6011 [ DERBY-6011 ]
          Gavin made changes -
          Workflow jira [ 12415982 ] Default workflow, editable Closed status [ 12801841 ]
          Hide
          Rick Hillegas added a comment -

          Attaching a first rev of a functional spec for this feature: MergeStatement.html.

          Show
          Rick Hillegas added a comment - Attaching a first rev of a functional spec for this feature: MergeStatement.html.
          Rick Hillegas made changes -
          Attachment MergeStatement.html [ 12597795 ]
          Hide
          Kim Haase added a comment -

          The spec draft looks great to me from a doc perspective – very clear.

          One typo maybe – in the second bullet item under "The following rules apply:", should "tableTable" be "targetTable"?

          Show
          Kim Haase added a comment - The spec draft looks great to me from a doc perspective – very clear. One typo maybe – in the second bullet item under "The following rules apply:", should "tableTable" be "targetTable"?
          Hide
          Rick Hillegas added a comment -

          Thanks for the quick feedback, Kim. I'll correct that typo in the next rev. Thanks.

          Show
          Rick Hillegas added a comment - Thanks for the quick feedback, Kim. I'll correct that typo in the next rev. Thanks.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-01-ac-grammar.diff. This is the first increment of work for implementing the MERGE statement. This patch covers the grammar and bind() logic. I will run tests.

          Although permissions descriptors are attached at bind() time, I did not tackle any of the permissions logic. The GRANT/REVOKE behavior of the MERGE statement will have to be verified and debugged when the execution logic is written. This may cascade changes back into the bind() logic.

          It is also possible that I have modelled the bind() structures in a way which will be frustrating for code-generation or execution. When I tackle code-generation and execution, I may need to adjust the bind() logic further.

          However, at this point I think that this increment is big enough for submission. This is a convenient checkpoint of work on the MERGE statement.

          Here is the basic model I have adopted:

          1) The WHEN MATCHED ... THEN UPDATE clause is modelled by an internally generated UpdateNode.

          2) The WHEN MATCHED ... THEN DELETE clause is modelled by an internally generated DeleteNode.

          3) The WHEN NOT MATCHED ... THEN INSERT clause is modelled by an internally generated InsertNode.

          4) The WHEN [ NOT ] MATCHED clauses are driven by an internally generated HalfOuterJoinNode which represents the following query:

          sourceTable LEFT OUTER JOIN targetTable ON searchCondition

          Along the way, I had to tweak other compile-time classes. However, I believe that these tweaks are minor. So far, the MERGE statement has not caused a lot of disruption.

          A valid MERGE statement currently raises the following error at the end of the bind phase:

          ERROR 0A000: Feature not implemented: MERGE.

          Touches the following files:

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

          M java/engine/org/apache/derby/impl/sql/compile/sqlgrammar.jj

          Grammar changes, including the introduction of two new non-reserved keywords: MERGE and MATCHED.

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

          A java/engine/org/apache/derby/impl/sql/compile/MergeNode.java
          A java/engine/org/apache/derby/impl/sql/compile/MatchingClauseNode.java

          Adds two new query nodes. MergeNode is a new DMLModStatementNode parallel to InsertNode, UpdateNode, and DeleteNode. MatchingClauseNode is a single class which can represent all three variants of the WHEN [ NOT ] MATCHED clause.

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

          M java/engine/org/apache/derby/impl/sql/compile/DeleteNode.java
          M java/engine/org/apache/derby/impl/sql/compile/UpdateNode.java

          When we internally generate an UpdateNode for a WHEN MATCHED ... THEN UPDATE clause, we have to allow references to the source table on the right side of the SET operator. For example, in the following statement...

          merge into t1
          using t2
          on t1.c1 = t2.c1
          when matched and t1.c2 != t2.c2 then update set c2 = t2.c2;

          ...we allow "set c2 = t2.c2" even though the table being updated is t1, not t2. This means that for this case we must disable a little piece of logic which was deliberately nulling out the tablename in expressions on the right side of the SET operator.

          In addition, I disabled a Sanity check which prevented us from having more than one table in the query which drives an UPDATE statement. This was necessary in order to use a left join to drive the WHEN MATCHED ... THEN UPDATE clause.

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

          M java/engine/org/apache/derby/impl/sql/compile/UnionNode.java
          M java/engine/org/apache/derby/impl/sql/compile/InsertNode.java
          M java/engine/org/apache/derby/impl/sql/compile/ResultColumnList.java

          Some extra book-keeping was added in order to support DEFAULT values for generated columns in WHEN NOT MATCHED ... THEN INSERT clauses. The handling of generated columns (including identity columns) is already a set of brittle special cases. This makes the handling more brittle and flags an area which may need extra testing.

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

          M java/engine/org/apache/derby/impl/sql/compile/JoinNode.java

          Two new methods were created in order to abstract out some bind() logic. That logic is now used both by ordinary left joins and by the internally generated left join which drives the MERGE statement.

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

          M java/engine/org/apache/derby/loc/messages.xml
          M java/shared/org/apache/derby/shared/common/reference/SQLState.java

          Adds 3 new messages for errors discovered at bind() time.

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

          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/GeneratedColumnsHelper.java
          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/_Suite.java
          A java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java

          Adds a battery of tests for the MERGE statement's bind() logic. Mostly these are negative tests, stressing error conditions.

          Show
          Rick Hillegas added a comment - Attaching derby-3155-01-ac-grammar.diff. This is the first increment of work for implementing the MERGE statement. This patch covers the grammar and bind() logic. I will run tests. Although permissions descriptors are attached at bind() time, I did not tackle any of the permissions logic. The GRANT/REVOKE behavior of the MERGE statement will have to be verified and debugged when the execution logic is written. This may cascade changes back into the bind() logic. It is also possible that I have modelled the bind() structures in a way which will be frustrating for code-generation or execution. When I tackle code-generation and execution, I may need to adjust the bind() logic further. However, at this point I think that this increment is big enough for submission. This is a convenient checkpoint of work on the MERGE statement. Here is the basic model I have adopted: 1) The WHEN MATCHED ... THEN UPDATE clause is modelled by an internally generated UpdateNode. 2) The WHEN MATCHED ... THEN DELETE clause is modelled by an internally generated DeleteNode. 3) The WHEN NOT MATCHED ... THEN INSERT clause is modelled by an internally generated InsertNode. 4) The WHEN [ NOT ] MATCHED clauses are driven by an internally generated HalfOuterJoinNode which represents the following query: sourceTable LEFT OUTER JOIN targetTable ON searchCondition Along the way, I had to tweak other compile-time classes. However, I believe that these tweaks are minor. So far, the MERGE statement has not caused a lot of disruption. A valid MERGE statement currently raises the following error at the end of the bind phase: ERROR 0A000: Feature not implemented: MERGE. Touches the following files: ------------------- M java/engine/org/apache/derby/impl/sql/compile/sqlgrammar.jj Grammar changes, including the introduction of two new non-reserved keywords: MERGE and MATCHED. ------------------- A java/engine/org/apache/derby/impl/sql/compile/MergeNode.java A java/engine/org/apache/derby/impl/sql/compile/MatchingClauseNode.java Adds two new query nodes. MergeNode is a new DMLModStatementNode parallel to InsertNode, UpdateNode, and DeleteNode. MatchingClauseNode is a single class which can represent all three variants of the WHEN [ NOT ] MATCHED clause. ------------------- M java/engine/org/apache/derby/impl/sql/compile/DeleteNode.java M java/engine/org/apache/derby/impl/sql/compile/UpdateNode.java When we internally generate an UpdateNode for a WHEN MATCHED ... THEN UPDATE clause, we have to allow references to the source table on the right side of the SET operator. For example, in the following statement... merge into t1 using t2 on t1.c1 = t2.c1 when matched and t1.c2 != t2.c2 then update set c2 = t2.c2; ...we allow "set c2 = t2.c2" even though the table being updated is t1, not t2. This means that for this case we must disable a little piece of logic which was deliberately nulling out the tablename in expressions on the right side of the SET operator. In addition, I disabled a Sanity check which prevented us from having more than one table in the query which drives an UPDATE statement. This was necessary in order to use a left join to drive the WHEN MATCHED ... THEN UPDATE clause. ------------------- M java/engine/org/apache/derby/impl/sql/compile/UnionNode.java M java/engine/org/apache/derby/impl/sql/compile/InsertNode.java M java/engine/org/apache/derby/impl/sql/compile/ResultColumnList.java Some extra book-keeping was added in order to support DEFAULT values for generated columns in WHEN NOT MATCHED ... THEN INSERT clauses. The handling of generated columns (including identity columns) is already a set of brittle special cases. This makes the handling more brittle and flags an area which may need extra testing. ------------------- M java/engine/org/apache/derby/impl/sql/compile/JoinNode.java Two new methods were created in order to abstract out some bind() logic. That logic is now used both by ordinary left joins and by the internally generated left join which drives the MERGE statement. ------------------- M java/engine/org/apache/derby/loc/messages.xml M java/shared/org/apache/derby/shared/common/reference/SQLState.java Adds 3 new messages for errors discovered at bind() time. ------------------- M java/testing/org/apache/derbyTesting/functionTests/tests/lang/GeneratedColumnsHelper.java M java/testing/org/apache/derbyTesting/functionTests/tests/lang/_Suite.java A java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java Adds a battery of tests for the MERGE statement's bind() logic. Mostly these are negative tests, stressing error conditions.
          Rick Hillegas made changes -
          Attachment derby-3155-01-ac-grammar.diff [ 12599018 ]
          Rick Hillegas made changes -
          Assignee Rick Hillegas [ rhillegas ]
          Hide
          Rick Hillegas added a comment -

          Tests passed cleanly for me on derby-3155-01-ac-grammar.diff.

          Show
          Rick Hillegas added a comment - Tests passed cleanly for me on derby-3155-01-ac-grammar.diff.
          Hide
          ASF subversion and git services added a comment -

          Commit 1516157 from Rick Hillegas in branch 'code/trunk'
          [ https://svn.apache.org/r1516157 ]

          DERBY-3155: Add grammar and bind logic for MERGE statement; commit derby-3155-01-ac-grammar.diff.

          Show
          ASF subversion and git services added a comment - Commit 1516157 from Rick Hillegas in branch 'code/trunk' [ https://svn.apache.org/r1516157 ] DERBY-3155 : Add grammar and bind logic for MERGE statement; commit derby-3155-01-ac-grammar.diff.
          Hide
          Knut Anders Hatlen added a comment -

          Thanks for working on this feature, Rick. I tried a couple of queries in ij just to see how it worked. I understand that this is work in progress, so that some things are not expected to work yet. Still, I'll post my observations just in case:

          • If I use a correlation name for the target table, and the correlation name doesn't match an existing table, I get "Table/View 'T1' does not exist." instead of the expected "Feature not implemented: MERGE".
          • The parser doesn't seem to accept more than one mergeWhenClause.
          • The examples in the functional specification are missing the THEN keyword.
          Show
          Knut Anders Hatlen added a comment - Thanks for working on this feature, Rick. I tried a couple of queries in ij just to see how it worked. I understand that this is work in progress, so that some things are not expected to work yet. Still, I'll post my observations just in case: If I use a correlation name for the target table, and the correlation name doesn't match an existing table, I get "Table/View 'T1' does not exist." instead of the expected "Feature not implemented: MERGE". The parser doesn't seem to accept more than one mergeWhenClause. The examples in the functional specification are missing the THEN keyword.
          Hide
          Rick Hillegas added a comment -

          Attaching a second rev of the functional spec, incorporating feedback from Kim and Knut. This fixes typos and adds THENs to the example MERGE statements. Thanks for the feedback, Kim and Knut.

          Show
          Rick Hillegas added a comment - Attaching a second rev of the functional spec, incorporating feedback from Kim and Knut. This fixes typos and adds THENs to the example MERGE statements. Thanks for the feedback, Kim and Knut.
          Rick Hillegas made changes -
          Attachment MergeStatement.html [ 12599439 ]
          Hide
          Rick Hillegas added a comment -

          Thanks for test-driving the parse/bind changes, Knut.

          o The issue with correlation names needs to be looked into. I'll investigate this after I come up for air on the next patch I'm hacking.

          o The issue with multiple WHEN [ NOT ] MATCHED clauses is fixed by a trivial correction to the grammar, which I'll roll onto the next patch. After applying that trivial fix, the following statement gets through the parse and bind phases:

          merge into t1
          using t2
          on t1.c1 = t2.c1
          when matched and t1.c2 != t2.c2 then update set c2 = t2.c2
          when not matched then insert ( c2 ) values ( t2.c2 );

          Thanks,
          -Rick

          Show
          Rick Hillegas added a comment - Thanks for test-driving the parse/bind changes, Knut. o The issue with correlation names needs to be looked into. I'll investigate this after I come up for air on the next patch I'm hacking. o The issue with multiple WHEN [ NOT ] MATCHED clauses is fixed by a trivial correction to the grammar, which I'll roll onto the next patch. After applying that trivial fix, the following statement gets through the parse and bind phases: merge into t1 using t2 on t1.c1 = t2.c1 when matched and t1.c2 != t2.c2 then update set c2 = t2.c2 when not matched then insert ( c2 ) values ( t2.c2 ); Thanks, -Rick
          Hide
          Dag H. Wanvik added a comment -

          just read through the functional spec of the MERGE statement. Looks good, found little to
          whine about

          > Privileges - Current privileges must include:
          > - UPDATE privilege on every updated column of targetTable.

          or on entire table (all columns)

          > - INSERT on every inserted column of targetTable.

          INSERT privilege is for the entire table right? (the wording "on every inserted column" threw me off)

          > functions - Functions mentioned in the booleanExpressions may not modify SQL data.

          Can they be non-deterministic I wonder?

          Show
          Dag H. Wanvik added a comment - just read through the functional spec of the MERGE statement. Looks good, found little to whine about > Privileges - Current privileges must include: > - UPDATE privilege on every updated column of targetTable. or on entire table (all columns) > - INSERT on every inserted column of targetTable. INSERT privilege is for the entire table right? (the wording "on every inserted column" threw me off) > functions - Functions mentioned in the booleanExpressions may not modify SQL data. Can they be non-deterministic I wonder?
          Hide
          Knut Anders Hatlen added a comment -

          I noticed that JavaCC now emits a warning. Looks like it started with revision 1516157.

          genParser:
          [echo] Generating SQL parser...
          [java] Java Compiler Compiler Version 4.0 (Parser Generator)
          [java] (type "javacc" with no arguments for help)
          [java] Reading from file sqlgrammar.jj . . .
          [java] Note: UNICODE_INPUT option is specified. Please make sure you create the parser/lexer using a Reader with the correct character encoding.
          [java] Warning: Choice conflict in (...)* construct at line 8313, column 31.
          [java] Expansion nested within construct and expansion following construct
          [java] have common prefixes, one of which is: ","
          [java] Consider using a lookahead of 2 or more for nested expansion.
          [java] File "TokenMgrError.java" does not exist. Will create one.
          [java] File "ParseException.java" does not exist. Will create one.
          [java] File "Token.java" does not exist. Will create one.
          [java] File "CharStream.java" does not exist. Will create one.
          [java] Parser generated with 0 errors and 1 warnings.

          Show
          Knut Anders Hatlen added a comment - I noticed that JavaCC now emits a warning. Looks like it started with revision 1516157. genParser: [echo] Generating SQL parser... [java] Java Compiler Compiler Version 4.0 (Parser Generator) [java] (type "javacc" with no arguments for help) [java] Reading from file sqlgrammar.jj . . . [java] Note: UNICODE_INPUT option is specified. Please make sure you create the parser/lexer using a Reader with the correct character encoding. [java] Warning: Choice conflict in (...)* construct at line 8313, column 31. [java] Expansion nested within construct and expansion following construct [java] have common prefixes, one of which is: "," [java] Consider using a lookahead of 2 or more for nested expansion. [java] File "TokenMgrError.java" does not exist. Will create one. [java] File "ParseException.java" does not exist. Will create one. [java] File "Token.java" does not exist. Will create one. [java] File "CharStream.java" does not exist. Will create one. [java] Parser generated with 0 errors and 1 warnings.
          Hide
          Rick Hillegas added a comment -

          Thanks for that feedback, Dag. Attaching a third rev of the spec, which makes the clarifications you suggest.

          Concerning non-deterministic expressions, including non-deterministic functions: Determinism isn't discussed by the SQL Standard, part 2, section 14.12 (<merge statement>). I can find no limitations related to determinism. Probably people would want to issue MERGE statements which mention non-deterministic expressions like CURRENT DATE.

          Show
          Rick Hillegas added a comment - Thanks for that feedback, Dag. Attaching a third rev of the spec, which makes the clarifications you suggest. Concerning non-deterministic expressions, including non-deterministic functions: Determinism isn't discussed by the SQL Standard, part 2, section 14.12 (<merge statement>). I can find no limitations related to determinism. Probably people would want to issue MERGE statements which mention non-deterministic expressions like CURRENT DATE.
          Rick Hillegas made changes -
          Attachment MergeStatement.html [ 12599944 ]
          Hide
          Rick Hillegas added a comment -

          Thanks for spotting that JavaCC warning, Knut. I turned on verbose compilation and saw the same warning on the trunk at the point where I last sync'd. However, I don't see that warning after applying the trivial grammar change which addressed a previous problem you discovered: lack of support for multiple MATCHED clauses. After making that trivial grammar change, the verbose parser output looks like this for me:

          genParser:
          [echo] Generating SQL parser...
          [java] Java Compiler Compiler Version 4.0 (Parser Generator)
          [java] (type "javacc" with no arguments for help)
          [java] Reading from file sqlgrammar.jj . . .
          [java] Note: UNICODE_INPUT option is specified. Please make sure you create the parser/lexer using a Reader with the correct character encoding.
          [java] File "TokenMgrError.java" does not exist. Will create one.
          [java] File "ParseException.java" does not exist. Will create one.
          [java] File "Token.java" does not exist. Will create one.
          [java] File "CharStream.java" does not exist. Will create one.
          [java] Parser generated successfully.

          So I think that the parser warning will disappear on trunk when I submit my next patch. Thanks.

          Show
          Rick Hillegas added a comment - Thanks for spotting that JavaCC warning, Knut. I turned on verbose compilation and saw the same warning on the trunk at the point where I last sync'd. However, I don't see that warning after applying the trivial grammar change which addressed a previous problem you discovered: lack of support for multiple MATCHED clauses. After making that trivial grammar change, the verbose parser output looks like this for me: genParser: [echo] Generating SQL parser... [java] Java Compiler Compiler Version 4.0 (Parser Generator) [java] (type "javacc" with no arguments for help) [java] Reading from file sqlgrammar.jj . . . [java] Note: UNICODE_INPUT option is specified. Please make sure you create the parser/lexer using a Reader with the correct character encoding. [java] File "TokenMgrError.java" does not exist. Will create one. [java] File "ParseException.java" does not exist. Will create one. [java] File "Token.java" does not exist. Will create one. [java] File "CharStream.java" does not exist. Will create one. [java] Parser generated successfully. So I think that the parser warning will disappear on trunk when I submit my next patch. Thanks.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-02-ag-fixParserWarning.diff. This patch removes the spurious comma separating WHEN [ NOT ] MATCHED clauses in the grammar. I am running tests now.

          This addresses two problems noted by Knut:

          o You can't issue a MERGE statement with multiple WHEN [ NOT ] MATCHED clauses using the grammar described by the functional spec.

          o If you compile verbosely, you see the following warning during parser generation:

          [java] Note: UNICODE_INPUT option is specified. Please make sure you create the parser/lexer using a Reader with the correct character encoding.
          [java] Warning: Choice conflict in (...)* construct at line 8313, column 31.
          [java] Expansion nested within construct and expansion following construct
          [java] have common prefixes, one of which is: ","
          [java] Consider using a lookahead of 2 or more for nested expansion.

          I had planned to roll this fix onto a patch with more functionality, but that patch is taking longer to write than I originally thought. So I'm submitting this small change by itself.

          Touches the following file:

          M java/engine/org/apache/derby/impl/sql/compile/sqlgrammar.jj

          Show
          Rick Hillegas added a comment - Attaching derby-3155-02-ag-fixParserWarning.diff. This patch removes the spurious comma separating WHEN [ NOT ] MATCHED clauses in the grammar. I am running tests now. This addresses two problems noted by Knut: o You can't issue a MERGE statement with multiple WHEN [ NOT ] MATCHED clauses using the grammar described by the functional spec. o If you compile verbosely, you see the following warning during parser generation: [java] Note: UNICODE_INPUT option is specified. Please make sure you create the parser/lexer using a Reader with the correct character encoding. [java] Warning: Choice conflict in (...)* construct at line 8313, column 31. [java] Expansion nested within construct and expansion following construct [java] have common prefixes, one of which is: "," [java] Consider using a lookahead of 2 or more for nested expansion. I had planned to roll this fix onto a patch with more functionality, but that patch is taking longer to write than I originally thought. So I'm submitting this small change by itself. Touches the following file: M java/engine/org/apache/derby/impl/sql/compile/sqlgrammar.jj
          Rick Hillegas made changes -
          Attachment derby-3155-02-ag-fixParserWarning.diff [ 12603815 ]
          Hide
          ASF subversion and git services added a comment -

          Commit 1524432 from Rick Hillegas in branch 'code/trunk'
          [ https://svn.apache.org/r1524432 ]

          DERBY-3155: Remove spurious comma from grammar production for multiple WHEN [NOT] MATCHED clauses in MERGE statement; tests ran cleanly on derby-3155-02-ag-fixParserWarning.diff.

          Show
          ASF subversion and git services added a comment - Commit 1524432 from Rick Hillegas in branch 'code/trunk' [ https://svn.apache.org/r1524432 ] DERBY-3155 : Remove spurious comma from grammar production for multiple WHEN [NOT] MATCHED clauses in MERGE statement; tests ran cleanly on derby-3155-02-ag-fixParserWarning.diff.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-03-ae-backingStoreHashtableWithRowLocation.diff. This patch adds support for BackingStoreHashtables which include a RowLocation for each row. I am running tests now.

          This patch intends to not degrade the performance of the existing code paths for BackingStoreHashtables which don't include RowLocations. BackingStoreHashtables which include RowLocations may incur some extra performance drag.

          I have hand-tested the changes by running some ad-hoc tests which use in-memory hash tables and by running SpillHashTest. I ran these tests with the current patch and with a dummy version which forced all BackingStoreHashtables to include RowLocation information.

          This patch makes the following changes:

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

          A java/engine/org/apache/derby/iapi/types/LocatedRow.java

          1) Introduces a new class, LocatedRow, which is basically a struct containing an array of column values followed by a RowLocation field.

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

          M java/engine/org/apache/derby/iapi/store/access/BackingStoreHashtable.java
          M java/engine/org/apache/derby/impl/store/access/BackingStoreHashTableFromScan.java

          2) Makes a number of changes to BackingStoreHashtable:

          a) Introduces a new public method, includeRowLocations(). This defaults to return false. However, the BackingStoreHashTableFromScan subclass overrides this method and may return true depending on its constructor args.

          b) Changes the signature of add_row_to_hash_table() and putRow() to include a RowLocation arg. This arg will be null when includeRowLocations() returns false.

          c) When includeRowLocations() returns false, the behavior of the class is pretty much unchanged. That is, the in-memory hash table continues to contain DataValueDescriptor[] rows or buckets (lists) of those rows. If the hash table spills to disk, DataValueDescriptor[] rows are written to disk. When they are read back in, they continue to be either standalone DataValueDescriptor[] rows or buckets (lists) of those rows.

          d) When includeRowLocations() returns true, the in-memory hash table contains LocatedRows and buckets (lists) of LocatedRows. If the hash table spills to disk, DataValueDescriptor[] rows are written to disk; the last cell of these rows is the RowLocation. When they are read back in, they are re-packaged as LocatedRows or buckets of LocatedRows.

          e) The memory usage methods have been adjusted to account for the extra overhead when a LocatedRow is used instead of a plain DataValueDescriptor[].

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

          M java/engine/org/apache/derby/impl/store/access/conglomerate/GenericScanController.java
          M java/engine/org/apache/derby/impl/store/access/btree/BTreeForwardScan.java
          M java/engine/org/apache/derby/impl/store/access/heap/HeapScan.java

          3) Store changes to account for the new RowLocation arg added to the signatures of add_row_to_hash_table() and putRow(). A new method was added to HeapScan for constructing RowLocations when necessary.

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

          M java/storeless/org/apache/derby/impl/storeless/NoOpTransaction.java
          M java/engine/org/apache/derby/iapi/store/access/TransactionController.java
          M java/engine/org/apache/derby/impl/store/access/RAMTransaction.java
          M java/testing/org/apache/derbyTesting/unitTests/store/T_QualifierTest.java

          4) TransactionController was adjusted to account for the new constructor arg for BackingStoreHashTableFromScan.

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

          M java/engine/org/apache/derby/iapi/store/build.xml

          5) The build target for this package was changed to uncomment the lint diagnostic for unchecked casts.

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

          M java/engine/org/apache/derby/impl/sql/execute/DistinctScanResultSet.java
          M java/engine/org/apache/derby/impl/sql/execute/HashScanResultSet.java
          M java/engine/org/apache/derby/impl/sql/execute/NoPutResultSetImpl.java
          M java/engine/org/apache/derby/impl/sql/execute/ScrollInsensitiveResultSet.java
          M java/engine/org/apache/derby/impl/sql/execute/UpdateResultSet.java

          6) ResultSets in the execution layer have been changed to account for the new constructor arg of BackingStoreHashTableFromScan. Changes have also been made to account for the fact that the hash table can now return LocatedRows or buckets of LocatedRows.

          Show
          Rick Hillegas added a comment - Attaching derby-3155-03-ae-backingStoreHashtableWithRowLocation.diff. This patch adds support for BackingStoreHashtables which include a RowLocation for each row. I am running tests now. This patch intends to not degrade the performance of the existing code paths for BackingStoreHashtables which don't include RowLocations. BackingStoreHashtables which include RowLocations may incur some extra performance drag. I have hand-tested the changes by running some ad-hoc tests which use in-memory hash tables and by running SpillHashTest. I ran these tests with the current patch and with a dummy version which forced all BackingStoreHashtables to include RowLocation information. This patch makes the following changes: ------------------------- A java/engine/org/apache/derby/iapi/types/LocatedRow.java 1) Introduces a new class, LocatedRow, which is basically a struct containing an array of column values followed by a RowLocation field. ------------------------- M java/engine/org/apache/derby/iapi/store/access/BackingStoreHashtable.java M java/engine/org/apache/derby/impl/store/access/BackingStoreHashTableFromScan.java 2) Makes a number of changes to BackingStoreHashtable: a) Introduces a new public method, includeRowLocations(). This defaults to return false. However, the BackingStoreHashTableFromScan subclass overrides this method and may return true depending on its constructor args. b) Changes the signature of add_row_to_hash_table() and putRow() to include a RowLocation arg. This arg will be null when includeRowLocations() returns false. c) When includeRowLocations() returns false, the behavior of the class is pretty much unchanged. That is, the in-memory hash table continues to contain DataValueDescriptor[] rows or buckets (lists) of those rows. If the hash table spills to disk, DataValueDescriptor[] rows are written to disk. When they are read back in, they continue to be either standalone DataValueDescriptor[] rows or buckets (lists) of those rows. d) When includeRowLocations() returns true, the in-memory hash table contains LocatedRows and buckets (lists) of LocatedRows. If the hash table spills to disk, DataValueDescriptor[] rows are written to disk; the last cell of these rows is the RowLocation. When they are read back in, they are re-packaged as LocatedRows or buckets of LocatedRows. e) The memory usage methods have been adjusted to account for the extra overhead when a LocatedRow is used instead of a plain DataValueDescriptor[]. ------------------------- M java/engine/org/apache/derby/impl/store/access/conglomerate/GenericScanController.java M java/engine/org/apache/derby/impl/store/access/btree/BTreeForwardScan.java M java/engine/org/apache/derby/impl/store/access/heap/HeapScan.java 3) Store changes to account for the new RowLocation arg added to the signatures of add_row_to_hash_table() and putRow(). A new method was added to HeapScan for constructing RowLocations when necessary. ------------------------- M java/storeless/org/apache/derby/impl/storeless/NoOpTransaction.java M java/engine/org/apache/derby/iapi/store/access/TransactionController.java M java/engine/org/apache/derby/impl/store/access/RAMTransaction.java M java/testing/org/apache/derbyTesting/unitTests/store/T_QualifierTest.java 4) TransactionController was adjusted to account for the new constructor arg for BackingStoreHashTableFromScan. ------------------------- M java/engine/org/apache/derby/iapi/store/build.xml 5) The build target for this package was changed to uncomment the lint diagnostic for unchecked casts. ------------------------- M java/engine/org/apache/derby/impl/sql/execute/DistinctScanResultSet.java M java/engine/org/apache/derby/impl/sql/execute/HashScanResultSet.java M java/engine/org/apache/derby/impl/sql/execute/NoPutResultSetImpl.java M java/engine/org/apache/derby/impl/sql/execute/ScrollInsensitiveResultSet.java M java/engine/org/apache/derby/impl/sql/execute/UpdateResultSet.java 6) ResultSets in the execution layer have been changed to account for the new constructor arg of BackingStoreHashTableFromScan. Changes have also been made to account for the fact that the hash table can now return LocatedRows or buckets of LocatedRows.
          Rick Hillegas made changes -
          Hide
          Rick Hillegas added a comment -

          Tests passed cleanly for me on derby-3155-03-ae-backingStoreHashtableWithRowLocation.diff.

          Show
          Rick Hillegas added a comment - Tests passed cleanly for me on derby-3155-03-ae-backingStoreHashtableWithRowLocation.diff.
          Hide
          Mike Matrigali added a comment -

          initial high level thought on store api changes, still looking at detail code.

          1) I was hoping that the interface change could somehow be made specific to just the merge usage. Instead it looks like you changed
          the generic hash scan result set code to now handle this mode in addition to original mode. While better for code sharing, there
          is obviously more overhead for the original code and the original fast path to handle more arguments and more if/then/elses for the
          new row passing scheme. I know the overhead is likely small, but would like to avoid if possible. This may be acceptable, for me it makes a complicated interface even more complicated. Was looking maybe for
          a brand new backing store hash table that only handled this new LocatedRow. Downside is probably this causes less code sharing and new interfaces that are similar to old ones with just handling the new row holder rather than the old.

          2) also see that you have a new LocatedRow which seems like a very clean way to handle a row and it's associated row location. But there are interfaces to shove the RowLocation onto the end of the row, which I think is a bad idea. If this is really needed I wonder if we can move that representation out of the store interface and into the sql layer somehow. It may become obvious on more detailed review but can you say why this is needed.

          Show
          Mike Matrigali added a comment - initial high level thought on store api changes, still looking at detail code. 1) I was hoping that the interface change could somehow be made specific to just the merge usage. Instead it looks like you changed the generic hash scan result set code to now handle this mode in addition to original mode. While better for code sharing, there is obviously more overhead for the original code and the original fast path to handle more arguments and more if/then/elses for the new row passing scheme. I know the overhead is likely small, but would like to avoid if possible. This may be acceptable, for me it makes a complicated interface even more complicated. Was looking maybe for a brand new backing store hash table that only handled this new LocatedRow. Downside is probably this causes less code sharing and new interfaces that are similar to old ones with just handling the new row holder rather than the old. 2) also see that you have a new LocatedRow which seems like a very clean way to handle a row and it's associated row location. But there are interfaces to shove the RowLocation onto the end of the row, which I think is a bad idea. If this is really needed I wonder if we can move that representation out of the store interface and into the sql layer somehow. It may become obvious on more detailed review but can you say why this is needed.
          Hide
          Mike Matrigali added a comment -

          start of detail review:

          initial high level thought on store api changes, still looking at detail code.
          nits: in LocatedRow: some code more than 80 char line. not sure where derby code standard is, but I find reading 1-line for loops and 1 line procedures distracting.
          BackingStoreHashTable:
          +unnecessary +80 col lines
          +should look at interface and see if external users of this class actually need to know the format of data spilled to disk. If this were private to the implementation then maybe it is less a problem to add the rowlocation to end of row on write to disk and read back from disk. maybe a comment in the routine that creates a BackingStoreHashtable from a rowsource that is never creates the "new" style hash table (or should it be paying attention to include row source?) It is kind of ugly already that there is implementation code in the
          interface - but that is an existing problem - not anything you added.
          + the following call is extra per row in the new code, and is not needed for non-merge system:
          Object hashValue = makeHashValue( columnValues, rowLocation );
          maybe some comments why it ok to pass hashValue into the space accounting rather than row. I think it is ok, just lost the logic while reading - i think hashValue has the row in it at this point.
          more single line if body else body

          as far in detail i got though patch so far (around line 300 of 1000).

          Show
          Mike Matrigali added a comment - start of detail review: initial high level thought on store api changes, still looking at detail code. nits: in LocatedRow: some code more than 80 char line. not sure where derby code standard is, but I find reading 1-line for loops and 1 line procedures distracting. BackingStoreHashTable: +unnecessary +80 col lines +should look at interface and see if external users of this class actually need to know the format of data spilled to disk. If this were private to the implementation then maybe it is less a problem to add the rowlocation to end of row on write to disk and read back from disk. maybe a comment in the routine that creates a BackingStoreHashtable from a rowsource that is never creates the "new" style hash table (or should it be paying attention to include row source?) It is kind of ugly already that there is implementation code in the interface - but that is an existing problem - not anything you added. + the following call is extra per row in the new code, and is not needed for non-merge system: Object hashValue = makeHashValue( columnValues, rowLocation ); maybe some comments why it ok to pass hashValue into the space accounting rather than row. I think it is ok, just lost the logic while reading - i think hashValue has the row in it at this point. more single line if body else body as far in detail i got though patch so far (around line 300 of 1000).
          Hide
          Rick Hillegas added a comment -

          Hi Mike,

          Thanks for the initial review of derby-3155-03-ae-backingStoreHashtableWithRowLocation.diff. Some responses follow:

          1) You have put your finger on the big design problem for BackingStoreHashtable: the desire for code re-use vs. the desire to not perturb the existing code path. These were the options I considered:

          a) Clone BackingStoreHashtable in order to create a new implementation just for RowLocations.

          + With this solution, it's easy to prove to yourself that the existing code path is not perturbed.

          • This is a very large, complicated class to begin with. I did not want to multiply the code complexity by 2. Cloning creates the brittle situation where even experts can forget to apply a bug fix in two parallel places.

          b) Add the RowLocation support inline inside the existing BackingStoreHashtable.

          + The additional code complexity and brittleness is minimal.

          • However, the existing code path is perturbed.

          c) Subclass BackingStoreHashtable.

          + This would add less complexity to the existing code path than (b).

          • This is probably less brittle than (a) but more brittle than (b).
          • The additional overhead for the existing code path is pretty much the same as for (b). Basically some conditional blocks could be turned into method calls.
          • But now BackingStoreHashTableFromScan would probably need to be subclassed too. We would have 4 classes doing the job of 1.

          d) Subclass BackingStoreHashTableFromScan.

          +/ This has pretty much the same advantages and disadvantages as (c). In addition...

          • This would make it more difficult to add support for RowLocations to hash tables which AREN'T driven by scans.

          e) Instead of subclassing BackingStoreHashTableFromScan, just add some new methods to it.

          +/ This has pretty much the same advantages and disadvantages as (d). In addition...

          + Creates 1 fewer class.

          To summarize:

          (a) I rejected this option because it seemed too complex.

          (b) I settled on this option because it seemed to strike the best balance between complexity and performance. I do think that the additional overhead for the existing code path is not measurable.

          (c) I rejected this option because it seemed too complex.

          (d) (e) I rejected these because of the difficulty they pose for adding future RowLocation support to hash tables which AREN'T driven by scans.

          Other suggestions are certainly welcome!

          2) As you note, LocatedRow contains the logic for appending the RowLocation onto the end of a DataValueDescriptor[]. The logic is indeed needed by the language layer. But it's also needed by BackingStoreHashtable when it spills rows to disk. These were the options I considered:

          a) Put the logic in LocatedRow, where it can be shared by both the language and Store layers.

          b) Put the logic somewhere else, where it can be shared.

          c) Clone the logic, keeping one copy of the code in the language layer and another copy in the Store.

          In the interests of reduced complexity and brittleness, I thought that (a) was the best choice. But other suggestions are certainly welcome.

          Thanks,
          -Rick

          Show
          Rick Hillegas added a comment - Hi Mike, Thanks for the initial review of derby-3155-03-ae-backingStoreHashtableWithRowLocation.diff. Some responses follow: 1) You have put your finger on the big design problem for BackingStoreHashtable: the desire for code re-use vs. the desire to not perturb the existing code path. These were the options I considered: a) Clone BackingStoreHashtable in order to create a new implementation just for RowLocations. + With this solution, it's easy to prove to yourself that the existing code path is not perturbed. This is a very large, complicated class to begin with. I did not want to multiply the code complexity by 2. Cloning creates the brittle situation where even experts can forget to apply a bug fix in two parallel places. b) Add the RowLocation support inline inside the existing BackingStoreHashtable. + The additional code complexity and brittleness is minimal. However, the existing code path is perturbed. c) Subclass BackingStoreHashtable. + This would add less complexity to the existing code path than (b). This is probably less brittle than (a) but more brittle than (b). The additional overhead for the existing code path is pretty much the same as for (b). Basically some conditional blocks could be turned into method calls. But now BackingStoreHashTableFromScan would probably need to be subclassed too. We would have 4 classes doing the job of 1. d) Subclass BackingStoreHashTableFromScan. +/ This has pretty much the same advantages and disadvantages as (c). In addition... This would make it more difficult to add support for RowLocations to hash tables which AREN'T driven by scans. e) Instead of subclassing BackingStoreHashTableFromScan, just add some new methods to it. +/ This has pretty much the same advantages and disadvantages as (d). In addition... + Creates 1 fewer class. To summarize: (a) I rejected this option because it seemed too complex. (b) I settled on this option because it seemed to strike the best balance between complexity and performance. I do think that the additional overhead for the existing code path is not measurable. (c) I rejected this option because it seemed too complex. (d) (e) I rejected these because of the difficulty they pose for adding future RowLocation support to hash tables which AREN'T driven by scans. Other suggestions are certainly welcome! 2) As you note, LocatedRow contains the logic for appending the RowLocation onto the end of a DataValueDescriptor[]. The logic is indeed needed by the language layer. But it's also needed by BackingStoreHashtable when it spills rows to disk. These were the options I considered: a) Put the logic in LocatedRow, where it can be shared by both the language and Store layers. b) Put the logic somewhere else, where it can be shared. c) Clone the logic, keeping one copy of the code in the language layer and another copy in the Store. In the interests of reduced complexity and brittleness, I thought that (a) was the best choice. But other suggestions are certainly welcome. Thanks, -Rick
          Hide
          Rick Hillegas added a comment -

          Hi Mike,

          Thanks for the second set of comments on derby-3155-03-ae-backingStoreHashtableWithRowLocation.diff. I'm happy to make the formatting changes you suggest. Some more responses follow:

          > should look at interface and see if external users of this class actually need to know the format of data spilled to disk. If this were private to the implementation then maybe it is less a problem to add the rowlocation to end of row on write to disk and read back from disk.

          I may not understand this question, but here are some more thoughts:

          The api for BackingStoreHashtable has certainly become uglier. We go from a bad situation where BackingStoreHashtable can return 2 kinds of objects and the calling code has to figure out the type of what's being retrieved. The 2 types are...

          i) Rows
          ii) Buckets of rows

          ...and the patch takes us to a situation where BackingStoreHashtable can now return 4 types of objects:

          i) Rows
          ii) Buckets of rows
          iii) LocatedRows
          iv) Buckets of LocatedRows

          It might be cleaner if the existing methods always returned buckets of rows and if we added some new methods to return buckets of LocatedRows. However, a change like this would perturb more code. In addition, wrapping single rows in vacuous buckets might result in a measurable performance hit on old code paths.

          > maybe a comment in the routine that creates a BackingStoreHashtable from a rowsource that is never creates the "new" style hash table (or should it be paying attention to include row source?) It is kind of ugly already that there is implementation code in the
          interface - but that is an existing problem - not anything you added.

          This sounds like a good idea.

          > the following call is extra per row in the new code, and is not needed for non-merge system:
          Object hashValue = makeHashValue( columnValues, rowLocation );
          maybe some comments why it ok to pass hashValue into the space accounting rather than row. I think it is ok, just lost the logic while reading - i think hashValue has the row in it at this point.

          Adding a comment sounds like a good idea. The point of the code is this: Space accounting is responsible for calculating how much space is taken up by the whole value which is stored in the hash table. For rows which don't have RowLocations, the value is just a row (a DataValueDescriptor[]). But for LocatedRows, there is some additional space consumed by the RowLocation and by the wrapping LocatedRow object.

          Thanks,
          -Rick

          Show
          Rick Hillegas added a comment - Hi Mike, Thanks for the second set of comments on derby-3155-03-ae-backingStoreHashtableWithRowLocation.diff. I'm happy to make the formatting changes you suggest. Some more responses follow: > should look at interface and see if external users of this class actually need to know the format of data spilled to disk. If this were private to the implementation then maybe it is less a problem to add the rowlocation to end of row on write to disk and read back from disk. I may not understand this question, but here are some more thoughts: The api for BackingStoreHashtable has certainly become uglier. We go from a bad situation where BackingStoreHashtable can return 2 kinds of objects and the calling code has to figure out the type of what's being retrieved. The 2 types are... i) Rows ii) Buckets of rows ...and the patch takes us to a situation where BackingStoreHashtable can now return 4 types of objects: i) Rows ii) Buckets of rows iii) LocatedRows iv) Buckets of LocatedRows It might be cleaner if the existing methods always returned buckets of rows and if we added some new methods to return buckets of LocatedRows. However, a change like this would perturb more code. In addition, wrapping single rows in vacuous buckets might result in a measurable performance hit on old code paths. > maybe a comment in the routine that creates a BackingStoreHashtable from a rowsource that is never creates the "new" style hash table (or should it be paying attention to include row source?) It is kind of ugly already that there is implementation code in the interface - but that is an existing problem - not anything you added. This sounds like a good idea. > the following call is extra per row in the new code, and is not needed for non-merge system: Object hashValue = makeHashValue( columnValues, rowLocation ); maybe some comments why it ok to pass hashValue into the space accounting rather than row. I think it is ok, just lost the logic while reading - i think hashValue has the row in it at this point. Adding a comment sounds like a good idea. The point of the code is this: Space accounting is responsible for calculating how much space is taken up by the whole value which is stored in the hash table. For rows which don't have RowLocations, the value is just a row (a DataValueDescriptor[]). But for LocatedRows, there is some additional space consumed by the RowLocation and by the wrapping LocatedRow object. Thanks, -Rick
          Hide
          Mike Matrigali added a comment -

          thanks for the thought process behind the decisions on the change. Given that and I can't come up with anything better I am ok with the overall approach.

          Obviously will need some tests for the code path, but I assume that will be covered by your overall changes to use this. I really like that
          you ran all the code in your path with this mode as default, maybe worth doing that one last time when you have everything checked in.

          if it makes sense for you i will wait for your next patch to do detailed review of the rest. I understand this one was to make sure you were
          on a good path - which I think you are.

          Show
          Mike Matrigali added a comment - thanks for the thought process behind the decisions on the change. Given that and I can't come up with anything better I am ok with the overall approach. Obviously will need some tests for the code path, but I assume that will be covered by your overall changes to use this. I really like that you ran all the code in your path with this mode as default, maybe worth doing that one last time when you have everything checked in. if it makes sense for you i will wait for your next patch to do detailed review of the rest. I understand this one was to make sure you were on a good path - which I think you are.
          Hide
          Rick Hillegas added a comment - - edited

          Attaching derby-3155-03-af-backingStoreHashtableWithRowLocation.diff. This patch incorporates Mike's review comments. I have run SpillHashTest as is and also with some dummy code which forces backing hash tables to always include RowLocations. I am running the full tests now.

          This patch makes the following changes to the previous rev. Most of these changes are in BackingStoreHashtable:

          1) Observes an 80 character line limit.

          2) Regularizes the formatting of "if" statements: Consequents are always enclosed in curly braces. Consequents no longer appear on the same line as the "if" condition. The occasional form "if(" is replaced with "if (".

          3) The makeHashValue() method has been eliminated. Its code has been inlined in the two places where it was called.

          4) Extra comments have been added. In particular, comments have been added to point out that RowLocations are not currently supported for RowSource-based hash tables. Also, comments have been added to the elements() and get() methods, explaining what kind of returned Objects the caller must expect.

          Touches the same files as the previous rev of the patch.

          Show
          Rick Hillegas added a comment - - edited Attaching derby-3155-03-af-backingStoreHashtableWithRowLocation.diff. This patch incorporates Mike's review comments. I have run SpillHashTest as is and also with some dummy code which forces backing hash tables to always include RowLocations. I am running the full tests now. This patch makes the following changes to the previous rev. Most of these changes are in BackingStoreHashtable: 1) Observes an 80 character line limit. 2) Regularizes the formatting of "if" statements: Consequents are always enclosed in curly braces. Consequents no longer appear on the same line as the "if" condition. The occasional form "if(" is replaced with "if (". 3) The makeHashValue() method has been eliminated. Its code has been inlined in the two places where it was called. 4) Extra comments have been added. In particular, comments have been added to point out that RowLocations are not currently supported for RowSource-based hash tables. Also, comments have been added to the elements() and get() methods, explaining what kind of returned Objects the caller must expect. Touches the same files as the previous rev of the patch.
          Rick Hillegas made changes -
          Hide
          Rick Hillegas added a comment -

          Tests passed cleanly for me on derby-3155-04-ab-deleteClauseNoStore.diff.

          Show
          Rick Hillegas added a comment - Tests passed cleanly for me on derby-3155-04-ab-deleteClauseNoStore.diff.
          Rick Hillegas made changes -
          Issue & fix info Patch Available [ 10102 ]
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-04-ae-deleteAction.diff. This patch adds code-generation and execution support for the DELETE actions of MERGE statements. With this patch I am able to successfully use a MERGE statement to DELETE rows from a table. Cascaded foreign keys and DELETE trigger actions are handled correctly. I have only been able to test this functionality with search conditions which cause the optimizer to pick a nested loop strategy for the driving left join of the MERGE statement. This patch will need to be adjusted after the review of derby-3155-03-af-backingStoreHashtableWithRowLocation.diff is done and that functionality is committed. However, I am attaching this patch now just in case my laptop suffers a disaster in the meantime.

          Even after adjustment, the DELETE functionality will need more work, particularly in the areas of permissions checking and dependency tracking.

          For people who are interested in a preliminary peek at the implementation, the best place to start is the header comment on MergeNode.

          Show
          Rick Hillegas added a comment - Attaching derby-3155-04-ae-deleteAction.diff. This patch adds code-generation and execution support for the DELETE actions of MERGE statements. With this patch I am able to successfully use a MERGE statement to DELETE rows from a table. Cascaded foreign keys and DELETE trigger actions are handled correctly. I have only been able to test this functionality with search conditions which cause the optimizer to pick a nested loop strategy for the driving left join of the MERGE statement. This patch will need to be adjusted after the review of derby-3155-03-af-backingStoreHashtableWithRowLocation.diff is done and that functionality is committed. However, I am attaching this patch now just in case my laptop suffers a disaster in the meantime. Even after adjustment, the DELETE functionality will need more work, particularly in the areas of permissions checking and dependency tracking. For people who are interested in a preliminary peek at the implementation, the best place to start is the header comment on MergeNode.
          Rick Hillegas made changes -
          Attachment derby-3155-04-ae-deleteAction.diff [ 12609193 ]
          Hide
          Mike Matrigali added a comment -

          hi, I am reviewing in detail. could you expand on the following:
          4) Extra comments have been added. In particular, comments have been added to point out that RowLocations are not currently supported for RowSource-based hash tables.

          Is the plan to support this with future changes? Or are RowSource-based hash tables always indexes?

          Show
          Mike Matrigali added a comment - hi, I am reviewing in detail. could you expand on the following: 4) Extra comments have been added. In particular, comments have been added to point out that RowLocations are not currently supported for RowSource-based hash tables. Is the plan to support this with future changes? Or are RowSource-based hash tables always indexes?
          Hide
          Mike Matrigali added a comment -

          overall comments:
          i am fine with approach and implementation.
          would like to understand the includeLocations use in the code, if this is in progress or what is expected for whole project.
          might be good to add an assert somewhere if code tries to ask for a RowLocation on a btree, or at least understand what
          happens in checked in code if that happened.

          Here are file by file, mostly just more style and 80 char comments - and content comments are about the above question:
          Index: java/storeless/org/apache/derby/impl/storeless/NoOpTransaction.java
          no comments.

          java/engine/org/apache/derby/iapi/types/LocatedRow.java (revision 0)
          no comments.

          Index: java/engine/org/apache/derby/iapi/store/access/BackingStoreHashtable.java

          psuedo code in BackingStoreHashTable javdoc is not right anymore, it shows row
          as being Object[]. Not sure if the hash description is right or not still.

          nit: comments added, over 80 char line

          this comment is confusing to me:
          Currently, if the RowSource is not null, then there is no su
          pport for including RowLocations in the returned rows. That functionality is
          only supported for scans from base tables.

          I think BackingStoreHashTable constructor action is now dependent on info
          provided from the passed in row source, some comment on that would help - which
          may be what the above is trying to say.

          is it right that includeRowLocations is hard coded to return false?

          nit: the following style still in code (this could just be my issue):
          + if ( !includeRowLocations() )
          +

          { return diskRow; }

          + else

          { return new LocatedRow( diskRow ); }

          I think the 2 acceptable forms are either:
          if ( !includeRowLocations() )

          { return diskRow; }

          else

          { return new LocatedRow( diskRow ); }

          if ( !includeRowLocations() )

          { return diskRow; }

          else

          { return new LocatedRow( diskRow ); }

          Index: java/engine/org/apache/derby/iapi/store/access/TransactionController.java

          nit: over 80 char lines

          Index: java/engine/org/apache/derby/iapi/store/build.xml

          no comments.

          Index: java/engine/org/apache/derby/impl/sql/execute/DistinctScanResultSet.java

          no comments

          Index: java/engine/org/apache/derby/impl/sql/execute/HashScanResultSet.java

          no comments
          Index: java/engine/org/apache/derby/impl/sql/execute/NoPutResultSetImpl.java

          nit: single line code style
          no other comments

          Index: java/engine/org/apache/derby/impl/sql/execute/ScrollInsensitiveResultSet.
          java
          no comments, not reviewed.

          Index: java/engine/org/apache/derby/impl/sql/execute/UpdateResultSet.java

          no comments

          Index: java/engine/org/apache/derby/impl/store/access/conglomerate/GenericScanCo
          ntroller.java
          nit: over 80 char

          Index: java/engine/org/apache/derby/impl/store/access/BackingStoreHashTableFromS
          can.java

          no comments

          Index: java/engine/org/apache/derby/impl/store/access/btree/BTreeForwardScan.jav
          a
          no comments.

          Index: java/engine/org/apache/derby/impl/store/access/RAMTransaction.java

          no comments.

          Index: java/engine/org/apache/derby/impl/store/access/heap/HeapScan.java

          no comments.

          Index: java/testing/org/apache/derbyTesting/unitTests/store/T_QualifierTest.java

          no comments.

          Show
          Mike Matrigali added a comment - overall comments: i am fine with approach and implementation. would like to understand the includeLocations use in the code, if this is in progress or what is expected for whole project. might be good to add an assert somewhere if code tries to ask for a RowLocation on a btree, or at least understand what happens in checked in code if that happened. Here are file by file, mostly just more style and 80 char comments - and content comments are about the above question: Index: java/storeless/org/apache/derby/impl/storeless/NoOpTransaction.java no comments. java/engine/org/apache/derby/iapi/types/LocatedRow.java (revision 0) no comments. Index: java/engine/org/apache/derby/iapi/store/access/BackingStoreHashtable.java psuedo code in BackingStoreHashTable javdoc is not right anymore, it shows row as being Object[]. Not sure if the hash description is right or not still. nit: comments added, over 80 char line this comment is confusing to me: Currently, if the RowSource is not null, then there is no su pport for including RowLocations in the returned rows. That functionality is only supported for scans from base tables. I think BackingStoreHashTable constructor action is now dependent on info provided from the passed in row source, some comment on that would help - which may be what the above is trying to say. is it right that includeRowLocations is hard coded to return false? nit: the following style still in code (this could just be my issue): + if ( !includeRowLocations() ) + { return diskRow; } + else { return new LocatedRow( diskRow ); } I think the 2 acceptable forms are either: if ( !includeRowLocations() ) { return diskRow; } else { return new LocatedRow( diskRow ); } if ( !includeRowLocations() ) { return diskRow; } else { return new LocatedRow( diskRow ); } Index: java/engine/org/apache/derby/iapi/store/access/TransactionController.java nit: over 80 char lines Index: java/engine/org/apache/derby/iapi/store/build.xml no comments. Index: java/engine/org/apache/derby/impl/sql/execute/DistinctScanResultSet.java no comments Index: java/engine/org/apache/derby/impl/sql/execute/HashScanResultSet.java no comments Index: java/engine/org/apache/derby/impl/sql/execute/NoPutResultSetImpl.java nit: single line code style no other comments Index: java/engine/org/apache/derby/impl/sql/execute/ScrollInsensitiveResultSet. java no comments, not reviewed. Index: java/engine/org/apache/derby/impl/sql/execute/UpdateResultSet.java no comments Index: java/engine/org/apache/derby/impl/store/access/conglomerate/GenericScanCo ntroller.java nit: over 80 char Index: java/engine/org/apache/derby/impl/store/access/BackingStoreHashTableFromS can.java no comments Index: java/engine/org/apache/derby/impl/store/access/btree/BTreeForwardScan.jav a no comments. Index: java/engine/org/apache/derby/impl/store/access/RAMTransaction.java no comments. Index: java/engine/org/apache/derby/impl/store/access/heap/HeapScan.java no comments. Index: java/testing/org/apache/derbyTesting/unitTests/store/T_QualifierTest.java no comments.
          Hide
          Rick Hillegas added a comment -

          Hi Mike,

          Responding to your first comment:

          If someone needs RowLocations in RowSource-driven hash tables, then they are welcome to add that functionality. I was not planning to add that functionality as part of this effort.

          BackingStoreHashtables appear to be created in two ways right now:

          1) Directly by invoking the BackingStoreHashtable constructor.

          2) Indirectly by calling TransactionController.createBackingStoreHashtableFromScan().

          In the execution layer, ScrollInsensitiveResultSet and UpdateResultSet directly create BackingStoreHashtables with null RowSource arguments. Also in the execution layer, HashTableResultSet directly creates a BackingStoreHashtable with a RowSource which is itself, that is, essentially a filtered version of its child node (and therefore not a scan).

          In the Store layer, BackingStoreHashTableFromScan directly invokes the BackingStoreHashtable constructor (its superclass) with a null RowSource.

          RAMTransaction.createBackingStoreHashtableFromScan() itself constructs a BackingStoreHashTableFromScan and therefore the RowSource is null.

          So I am only seeing one code path where a non-null RowSource is specified. That is in HashTableResultSet. But I must be missing something because your question implies that these are created by index scans.

          Thanks,
          -Rick

          Show
          Rick Hillegas added a comment - Hi Mike, Responding to your first comment: If someone needs RowLocations in RowSource-driven hash tables, then they are welcome to add that functionality. I was not planning to add that functionality as part of this effort. BackingStoreHashtables appear to be created in two ways right now: 1) Directly by invoking the BackingStoreHashtable constructor. 2) Indirectly by calling TransactionController.createBackingStoreHashtableFromScan(). In the execution layer, ScrollInsensitiveResultSet and UpdateResultSet directly create BackingStoreHashtables with null RowSource arguments. Also in the execution layer, HashTableResultSet directly creates a BackingStoreHashtable with a RowSource which is itself, that is, essentially a filtered version of its child node (and therefore not a scan). In the Store layer, BackingStoreHashTableFromScan directly invokes the BackingStoreHashtable constructor (its superclass) with a null RowSource. RAMTransaction.createBackingStoreHashtableFromScan() itself constructs a BackingStoreHashTableFromScan and therefore the RowSource is null. So I am only seeing one code path where a non-null RowSource is specified. That is in HashTableResultSet. But I must be missing something because your question implies that these are created by index scans. Thanks, -Rick
          Hide
          Rick Hillegas added a comment -

          Hi Mike,

          Here are responses to your later comments:

          > would like to understand the includeLocations use in the code, if this
          > is in progress or what is expected for whole project.

          In a nutshell, includeRowLocations() == true iff the contents of the hash table are LocatedRows. Conversely, includeRowLocations() == false iff the contents of the hash table are DataValueDescriptor[]. Support for LocatedRows is provided for scan-driven (rather than RowSource-driven) hashtables. If someone wants to add LocatedRow support for RowSource-driven hash tables, they are welcome to do so. But I don't recommend doing this until someone has an actual use-case which would benefit from this feature and prove that it works. At this time, my MERGE statement work hasn't created such a use-case.

          > might be good to add an assert somewhere if code tries to ask for a
          > RowLocation on a btree, or at least understand what
          > happens in checked in code if that happened.

          I will put the following assertion at the beginning of BTreeForwardScan.fetchRows():

          if (SanityManager.DEBUG)

          { // RowLocations in the BTree itself are unstable and should // not be put in long-lived structures like persistent hash tables. SanityManager.ASSERT ( (hash_table == null) || !hash_table.includeRowLocations() ); }

          > Index: java/engine/org/apache/derby/iapi/store/access/BackingStoreHashtable.java
          >
          > psuedo code in BackingStoreHashTable javdoc is not right anymore, it shows row
          > as being Object[]. Not sure if the hash description is right or not still.

          I will change the row declaration in the pseudo-code to:

          Object row; // is a DataValueDescriptor[] or a LocatedRow

          The hash description looks right to me, but I am happy to correct it if you see a problem.

          > nit: comments added, over 80 char line

          I don't see any added lines which bust this limit now. There are some original lines which bust this limit, however. If you still think some lines should be corrected, please give me the offending line numbers.

          > this comment is confusing to me:
          > Currently, if the RowSource is not null, then there is no su
          > pport for including RowLocations in the returned rows. That functionality is
          > only supported for scans from base tables.
          >
          > I think BackingStoreHashTable constructor action is now dependent on info
          > provided from the passed in row source, some comment on that would help - which
          > may be what the above is trying to say.

          I will change the comment to the following. I hope this is less confusing...

          • constructor. RowLocations are supported iff row_source is null.
          • RowLocations in a non-null row_source can be added later
          • if there is a use-case that stresses this behavior.

          ...and I will add the following sanity check at the beginning of the constructor:

          if (SanityManager.DEBUG)
          {
          // RowLocations are not currently supported if the
          // hash table is being filled from a non-null
          // row_source arg.
          if ( row_source != null )

          { SanityManager.ASSERT( !includeRowLocations() ); }

          }

          > is it right that includeRowLocations is hard coded to return false?

          That's correct. That method is overridden by the BackingStoreHashTableFromScan subclass, based on its constructor args.

          > nit: the following style still in code (this could just be my issue):
          > + if ( !includeRowLocations() )
          > +
          >

          { return diskRow; }
          >
          > + else
          > { return new LocatedRow( diskRow ); }
          >
          > I think the 2 acceptable forms are either:
          > if ( !includeRowLocations() )
          > { return diskRow; }

          >
          > else
          >

          { return new LocatedRow( diskRow ); }
          >
          > if ( !includeRowLocations() )
          > { return diskRow; }
          >
          > else
          > { return new LocatedRow( diskRow ); }

          I'm afraid I'm not following this one. I don't see any instances of a blank line following "if ( !includeRowLocations() )" in the diff file. Can you give me the line number of the problematic usage?

          > Index: java/engine/org/apache/derby/iapi/store/access/TransactionController.java
          >
          > nit: over 80 char lines

          Done.

          > Index: java/engine/org/apache/derby/impl/store/access/conglomerate/GenericScanCo
          > ntroller.java
          > nit: over 80 char

          Done.

          Thanks,
          -Rick

          Show
          Rick Hillegas added a comment - Hi Mike, Here are responses to your later comments: > would like to understand the includeLocations use in the code, if this > is in progress or what is expected for whole project. In a nutshell, includeRowLocations() == true iff the contents of the hash table are LocatedRows. Conversely, includeRowLocations() == false iff the contents of the hash table are DataValueDescriptor[]. Support for LocatedRows is provided for scan-driven (rather than RowSource-driven) hashtables. If someone wants to add LocatedRow support for RowSource-driven hash tables, they are welcome to do so. But I don't recommend doing this until someone has an actual use-case which would benefit from this feature and prove that it works. At this time, my MERGE statement work hasn't created such a use-case. > might be good to add an assert somewhere if code tries to ask for a > RowLocation on a btree, or at least understand what > happens in checked in code if that happened. I will put the following assertion at the beginning of BTreeForwardScan.fetchRows(): if (SanityManager.DEBUG) { // RowLocations in the BTree itself are unstable and should // not be put in long-lived structures like persistent hash tables. SanityManager.ASSERT ( (hash_table == null) || !hash_table.includeRowLocations() ); } > Index: java/engine/org/apache/derby/iapi/store/access/BackingStoreHashtable.java > > psuedo code in BackingStoreHashTable javdoc is not right anymore, it shows row > as being Object[]. Not sure if the hash description is right or not still. I will change the row declaration in the pseudo-code to: Object row; // is a DataValueDescriptor[] or a LocatedRow The hash description looks right to me, but I am happy to correct it if you see a problem. > nit: comments added, over 80 char line I don't see any added lines which bust this limit now. There are some original lines which bust this limit, however. If you still think some lines should be corrected, please give me the offending line numbers. > this comment is confusing to me: > Currently, if the RowSource is not null, then there is no su > pport for including RowLocations in the returned rows. That functionality is > only supported for scans from base tables. > > I think BackingStoreHashTable constructor action is now dependent on info > provided from the passed in row source, some comment on that would help - which > may be what the above is trying to say. I will change the comment to the following. I hope this is less confusing... constructor. RowLocations are supported iff row_source is null. RowLocations in a non-null row_source can be added later if there is a use-case that stresses this behavior. ...and I will add the following sanity check at the beginning of the constructor: if (SanityManager.DEBUG) { // RowLocations are not currently supported if the // hash table is being filled from a non-null // row_source arg. if ( row_source != null ) { SanityManager.ASSERT( !includeRowLocations() ); } } > is it right that includeRowLocations is hard coded to return false? That's correct. That method is overridden by the BackingStoreHashTableFromScan subclass, based on its constructor args. > nit: the following style still in code (this could just be my issue): > + if ( !includeRowLocations() ) > + > { return diskRow; } > > + else > { return new LocatedRow( diskRow ); } > > I think the 2 acceptable forms are either: > if ( !includeRowLocations() ) > { return diskRow; } > > else > { return new LocatedRow( diskRow ); } > > if ( !includeRowLocations() ) > { return diskRow; } > > else > { return new LocatedRow( diskRow ); } I'm afraid I'm not following this one. I don't see any instances of a blank line following "if ( !includeRowLocations() )" in the diff file. Can you give me the line number of the problematic usage? > Index: java/engine/org/apache/derby/iapi/store/access/TransactionController.java > > nit: over 80 char lines Done. > Index: java/engine/org/apache/derby/impl/store/access/conglomerate/GenericScanCo > ntroller.java > nit: over 80 char Done. Thanks, -Rick
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-03-ag-backingStoreHashtableWithRowLocation.diff. This patch makes the changes which Mike recommended. I will run tests again.

          Show
          Rick Hillegas added a comment - Attaching derby-3155-03-ag-backingStoreHashtableWithRowLocation.diff. This patch makes the changes which Mike recommended. I will run tests again.
          Rick Hillegas made changes -
          Hide
          Mike Matrigali added a comment -

          sorry posting to JIRA is messing up the format that I am trying to talk about and turned it into exactly
          what i was trying to say was not ok. Never have figured out how to get a JIRA comment to properly
          display code formatting. I was not talking about blank lines, but code and both open and close
          parens on same line.

          so line 639 of BackingStoreHashTable.java has code and parens all on same line, i think it should
          be 3 lines. obviously a nit.

          In vi if you search for

          {.*}

          you will see other lines i was talking about.

          Your new comment changes and other change look good to me.

          Show
          Mike Matrigali added a comment - sorry posting to JIRA is messing up the format that I am trying to talk about and turned it into exactly what i was trying to say was not ok. Never have figured out how to get a JIRA comment to properly display code formatting. I was not talking about blank lines, but code and both open and close parens on same line. so line 639 of BackingStoreHashTable.java has code and parens all on same line, i think it should be 3 lines. obviously a nit. In vi if you search for {.*} you will see other lines i was talking about. Your new comment changes and other change look good to me.
          Hide
          Rick Hillegas added a comment -

          Tests passed cleanly for me on derby-3155-03-ag-backingStoreHashtableWithRowLocation.diff.

          Show
          Rick Hillegas added a comment - Tests passed cleanly for me on derby-3155-03-ag-backingStoreHashtableWithRowLocation.diff.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-03-ah-backingStoreHashtableWithRowLocation.diff. This patch re-formats some conditional blocks in BackingStoreHashtable as Mike recommended. I will run tests again.

          Show
          Rick Hillegas added a comment - Attaching derby-3155-03-ah-backingStoreHashtableWithRowLocation.diff. This patch re-formats some conditional blocks in BackingStoreHashtable as Mike recommended. I will run tests again.
          Rick Hillegas made changes -
          Hide
          Rick Hillegas added a comment -

          Tests passed cleanly for me on derby-3155-03-ah-backingStoreHashtableWithRowLocation.diff.

          Show
          Rick Hillegas added a comment - Tests passed cleanly for me on derby-3155-03-ah-backingStoreHashtableWithRowLocation.diff.
          Hide
          ASF subversion and git services added a comment -

          Commit 1535001 from Rick Hillegas in branch 'code/trunk'
          [ https://svn.apache.org/r1535001 ]

          DERBY-3155: Add (optional) row locations to backing hash tables: derby-3155-03-ag-backingStoreHashtableWithRowLocation.diff.

          Show
          ASF subversion and git services added a comment - Commit 1535001 from Rick Hillegas in branch 'code/trunk' [ https://svn.apache.org/r1535001 ] DERBY-3155 : Add (optional) row locations to backing hash tables: derby-3155-03-ag-backingStoreHashtableWithRowLocation.diff.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-04-af-deleteAction.diff. This patch enables the DELETE action of the MERGE statement, capitalizing on the row location work done by derby-3155-03-ah-backingStoreHashtableWithRowLocation.diff. I am running tests now.

          With this patch, I am able to use the MERGE statement to DELETE rows. Various combinations of the following conditions have been tested:

          1) MERGE statements with and without refined conditions in the WHEN MATCHED clause.

          2) Search conditions which cause the optimizer to choose a nested-loop strategy.

          3) Search conditions which cause the optimizer to choose a hash-join strategy.

          4) Target tables with before and after per-row triggers.

          5) Target tables with before and after per-statement triggers.

          The basic operation of the MERGE statement is described by the header comment in MergeNode. The following additional execution behavior is worth mentioning:

          The MergeResultSet loops through the rows coming from the driving left-join, assigning each row to a WHEN [ NOT ] MATCHED action. After all of the rows have been assigned, the MergeResultSet then calls logic in each matching clause action to process the buffered rows assigned to that action. The matching clause action does the following:

          i) Pushes the corresponding INSERT/UPDATE/DELETE ConstantAction onto a stack of constant actions, introduced by this patch. This makes that INSERT/UPDATE/DELETE ConstantAction the current ConstantAction for the Activation. Introducing a stack of ConstantActions caused fewer code changes than wrenching around the constructors and code generators for the Insert/Update/DeleteResultSets so that they could handle fetching a ConstantAction out of a variable in the Activation. The Insert/Update/DeleteResultSets continue to take whatever ConstantAction is at the top of the stack.

          ii) Pokes the temporary ResultSet of buffered rows into a variable which is pushed as an argument to the Insert/Update/DeleteResultSet.

          iii) Invokes a method to instantiate the Insert/Update/DeleteResultSet.

          iv) Opens the Insert/Update/DeleteResultSet in order to cause it to execute.

          Touches the following files:

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

          M java/engine/org/apache/derby/impl/sql/compile/MergeNode.java
          M java/engine/org/apache/derby/impl/sql/compile/MatchingClauseNode.java
          M java/engine/org/apache/derby/impl/sql/compile/QueryTreeNode.java
          M java/engine/org/apache/derby/impl/sql/compile/DeleteNode.java
          M java/engine/org/apache/derby/impl/sql/compile/UpdateNode.java
          M java/engine/org/apache/derby/impl/sql/compile/FromBaseTable.java
          M java/engine/org/apache/derby/impl/sql/compile/DMLModStatementNode.java
          M java/engine/org/apache/derby/impl/sql/compile/InsertNode.java
          M java/engine/org/apache/derby/impl/sql/compile/CurrentOfNode.java
          M java/engine/org/apache/derby/impl/sql/compile/sqlgrammar.jj

          Changes for binding and code-generating MERGE statements with DELETE actions. See the header comment in MergeNode for more details.

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

          M java/engine/org/apache/derby/iapi/sql/Activation.java
          M java/engine/org/apache/derby/impl/sql/execute/BaseActivation.java
          M java/engine/org/apache/derby/impl/sql/GenericActivationHolder.java

          Introduces a stack of ConstantActions.

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

          M java/engine/org/apache/derby/iapi/sql/execute/ResultSetFactory.java
          M java/engine/org/apache/derby/impl/sql/execute/GenericResultSetFactory.java
          M java/engine/org/apache/derby/impl/sql/execute/DMLWriteResultSet.java
          A java/engine/org/apache/derby/impl/sql/execute/MergeResultSet.java

          Execution machinery to process the left-join, assign qualifying rows to the appropriate MatchingClauseConstantAction, and then invoke those ConstantActions in order after the left-join is drained.

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

          M java/engine/org/apache/derby/iapi/sql/execute/ConstantAction.java
          M java/engine/org/apache/derby/iapi/services/io/RegisteredFormatIds.java
          M java/engine/org/apache/derby/iapi/services/io/StoredFormatIds.java
          A java/engine/org/apache/derby/impl/sql/execute/MergeConstantAction.java
          A java/engine/org/apache/derby/impl/sql/execute/MatchingClauseConstantAction.java
          M java/engine/org/apache/derby/impl/sql/execute/GenericConstantActionFactory.java

          New Formatable ConstantActions: one for the overall MERGE statement and another for its WHEN [ NOT ] MATCHED clauses.

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

          M java/engine/org/apache/derby/impl/sql/execute/ScanResultSet.java
          M java/engine/org/apache/derby/impl/sql/execute/HashScanResultSet.java
          M java/engine/org/apache/derby/impl/sql/execute/BulkTableScanResultSet.java

          Changes to retrieve the target table's RowLocation during the processing of the driving left-join.

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

          M java/engine/org/apache/derby/impl/sql/execute/DeleteResultSet.java
          M java/engine/org/apache/derby/impl/store/access/heap/HeapRowLocation.java

          Change HeapRowLocation.getObject() to return itself rather than null. This is the economical way to retrieve a RowLocation from a bulk scan.

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

          M java/engine/org/apache/derby/loc/messages.xml

          Re-worded an error message so it could be re-used for a MERGE-related error condition.

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

          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/GeneratedColumnsHelper.java
          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java

          Tests for the new functionality.

          Show
          Rick Hillegas added a comment - Attaching derby-3155-04-af-deleteAction.diff. This patch enables the DELETE action of the MERGE statement, capitalizing on the row location work done by derby-3155-03-ah-backingStoreHashtableWithRowLocation.diff. I am running tests now. With this patch, I am able to use the MERGE statement to DELETE rows. Various combinations of the following conditions have been tested: 1) MERGE statements with and without refined conditions in the WHEN MATCHED clause. 2) Search conditions which cause the optimizer to choose a nested-loop strategy. 3) Search conditions which cause the optimizer to choose a hash-join strategy. 4) Target tables with before and after per-row triggers. 5) Target tables with before and after per-statement triggers. The basic operation of the MERGE statement is described by the header comment in MergeNode. The following additional execution behavior is worth mentioning: The MergeResultSet loops through the rows coming from the driving left-join, assigning each row to a WHEN [ NOT ] MATCHED action. After all of the rows have been assigned, the MergeResultSet then calls logic in each matching clause action to process the buffered rows assigned to that action. The matching clause action does the following: i) Pushes the corresponding INSERT/UPDATE/DELETE ConstantAction onto a stack of constant actions, introduced by this patch. This makes that INSERT/UPDATE/DELETE ConstantAction the current ConstantAction for the Activation. Introducing a stack of ConstantActions caused fewer code changes than wrenching around the constructors and code generators for the Insert/Update/DeleteResultSets so that they could handle fetching a ConstantAction out of a variable in the Activation. The Insert/Update/DeleteResultSets continue to take whatever ConstantAction is at the top of the stack. ii) Pokes the temporary ResultSet of buffered rows into a variable which is pushed as an argument to the Insert/Update/DeleteResultSet. iii) Invokes a method to instantiate the Insert/Update/DeleteResultSet. iv) Opens the Insert/Update/DeleteResultSet in order to cause it to execute. Touches the following files: --------------- M java/engine/org/apache/derby/impl/sql/compile/MergeNode.java M java/engine/org/apache/derby/impl/sql/compile/MatchingClauseNode.java M java/engine/org/apache/derby/impl/sql/compile/QueryTreeNode.java M java/engine/org/apache/derby/impl/sql/compile/DeleteNode.java M java/engine/org/apache/derby/impl/sql/compile/UpdateNode.java M java/engine/org/apache/derby/impl/sql/compile/FromBaseTable.java M java/engine/org/apache/derby/impl/sql/compile/DMLModStatementNode.java M java/engine/org/apache/derby/impl/sql/compile/InsertNode.java M java/engine/org/apache/derby/impl/sql/compile/CurrentOfNode.java M java/engine/org/apache/derby/impl/sql/compile/sqlgrammar.jj Changes for binding and code-generating MERGE statements with DELETE actions. See the header comment in MergeNode for more details. --------------- M java/engine/org/apache/derby/iapi/sql/Activation.java M java/engine/org/apache/derby/impl/sql/execute/BaseActivation.java M java/engine/org/apache/derby/impl/sql/GenericActivationHolder.java Introduces a stack of ConstantActions. --------------- M java/engine/org/apache/derby/iapi/sql/execute/ResultSetFactory.java M java/engine/org/apache/derby/impl/sql/execute/GenericResultSetFactory.java M java/engine/org/apache/derby/impl/sql/execute/DMLWriteResultSet.java A java/engine/org/apache/derby/impl/sql/execute/MergeResultSet.java Execution machinery to process the left-join, assign qualifying rows to the appropriate MatchingClauseConstantAction, and then invoke those ConstantActions in order after the left-join is drained. --------------- M java/engine/org/apache/derby/iapi/sql/execute/ConstantAction.java M java/engine/org/apache/derby/iapi/services/io/RegisteredFormatIds.java M java/engine/org/apache/derby/iapi/services/io/StoredFormatIds.java A java/engine/org/apache/derby/impl/sql/execute/MergeConstantAction.java A java/engine/org/apache/derby/impl/sql/execute/MatchingClauseConstantAction.java M java/engine/org/apache/derby/impl/sql/execute/GenericConstantActionFactory.java New Formatable ConstantActions: one for the overall MERGE statement and another for its WHEN [ NOT ] MATCHED clauses. --------------- M java/engine/org/apache/derby/impl/sql/execute/ScanResultSet.java M java/engine/org/apache/derby/impl/sql/execute/HashScanResultSet.java M java/engine/org/apache/derby/impl/sql/execute/BulkTableScanResultSet.java Changes to retrieve the target table's RowLocation during the processing of the driving left-join. --------------- M java/engine/org/apache/derby/impl/sql/execute/DeleteResultSet.java M java/engine/org/apache/derby/impl/store/access/heap/HeapRowLocation.java Change HeapRowLocation.getObject() to return itself rather than null. This is the economical way to retrieve a RowLocation from a bulk scan. --------------- M java/engine/org/apache/derby/loc/messages.xml Re-worded an error message so it could be re-used for a MERGE-related error condition. --------------- M java/testing/org/apache/derbyTesting/functionTests/tests/lang/GeneratedColumnsHelper.java M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java Tests for the new functionality.
          Rick Hillegas made changes -
          Attachment derby-3155-04-af-deleteAction.diff [ 12609886 ]
          Hide
          Rick Hillegas added a comment -

          Tests passed cleanly for me on derby-3155-04-af-deleteAction.diff.

          Show
          Rick Hillegas added a comment - Tests passed cleanly for me on derby-3155-04-af-deleteAction.diff.
          Hide
          ASF subversion and git services added a comment -

          Commit 1535360 from Rick Hillegas in branch 'code/trunk'
          [ https://svn.apache.org/r1535360 ]

          DERBY-3155: Add support for DELETE action of MERGE statement; checking in derby-3155-04-af-deleteAction.diff.

          Show
          ASF subversion and git services added a comment - Commit 1535360 from Rick Hillegas in branch 'code/trunk' [ https://svn.apache.org/r1535360 ] DERBY-3155 : Add support for DELETE action of MERGE statement; checking in derby-3155-04-af-deleteAction.diff.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-05-aa-triggerTransitionTableAsTarget.diff. This patch uncomments a test case which used to fail but which now succeeds.

          This case is supposed to verify that trigger transition tables can't be used as the target tables of MERGE statements. This test used to die on an assertion. The test now runs correctly, probably as a result of recent improvements in how the compiler handles transition tables.

          Touches the following file:

          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java

          Show
          Rick Hillegas added a comment - Attaching derby-3155-05-aa-triggerTransitionTableAsTarget.diff. This patch uncomments a test case which used to fail but which now succeeds. This case is supposed to verify that trigger transition tables can't be used as the target tables of MERGE statements. This test used to die on an assertion. The test now runs correctly, probably as a result of recent improvements in how the compiler handles transition tables. Touches the following file: M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java
          Rick Hillegas made changes -
          Hide
          ASF subversion and git services added a comment -

          Commit 1535397 from Rick Hillegas in branch 'code/trunk'
          [ https://svn.apache.org/r1535397 ]

          DERBY-3155: Uncomment a test verifying that trigger transition tables cannot be used as the target tables of MERGE statements: derby-3155-05-aa-triggerTransitionTableAsTarget.diff.

          Show
          ASF subversion and git services added a comment - Commit 1535397 from Rick Hillegas in branch 'code/trunk' [ https://svn.apache.org/r1535397 ] DERBY-3155 : Uncomment a test verifying that trigger transition tables cannot be used as the target tables of MERGE statements: derby-3155-05-aa-triggerTransitionTableAsTarget.diff.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-06-aa-triggerTransitionTableAsSource.diff. This patch fixes some bugs in the serialization of the newly added ConstantActions and adds test cases for using trigger transition tables as the source tables for MERGE statements.

          Touches the following files:

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

          M java/engine/org/apache/derby/impl/sql/execute/MergeConstantAction.java
          M java/engine/org/apache/derby/impl/sql/execute/MatchingClauseConstantAction.java

          Fixes Formatable serialization for these classes.

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

          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java

          New test cases.

          Show
          Rick Hillegas added a comment - Attaching derby-3155-06-aa-triggerTransitionTableAsSource.diff. This patch fixes some bugs in the serialization of the newly added ConstantActions and adds test cases for using trigger transition tables as the source tables for MERGE statements. Touches the following files: ------------------ M java/engine/org/apache/derby/impl/sql/execute/MergeConstantAction.java M java/engine/org/apache/derby/impl/sql/execute/MatchingClauseConstantAction.java Fixes Formatable serialization for these classes. ------------------ M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java New test cases.
          Rick Hillegas made changes -
          Hide
          ASF subversion and git services added a comment -

          Commit 1535447 from Rick Hillegas in branch 'code/trunk'
          [ https://svn.apache.org/r1535447 ]

          DERBY-3155: Fix some serialization bugs in the MERGE ConstantActions and add test cases for using trigger transition tables as the source tables of MERGE statements; check in derby-3155-06-aa-triggerTransitionTableAsSource.diff.

          Show
          ASF subversion and git services added a comment - Commit 1535447 from Rick Hillegas in branch 'code/trunk' [ https://svn.apache.org/r1535447 ] DERBY-3155 : Fix some serialization bugs in the MERGE ConstantActions and add test cases for using trigger transition tables as the source tables of MERGE statements; check in derby-3155-06-aa-triggerTransitionTableAsSource.diff.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-07-ad-insertAction.diff. This patch adds support for the INSERT action of MERGE statements. I am running tests now.

          The patch includes tests for the following use-cases:

          i) INSERT actions with DEFAULT expressions for identity, generated, and default columns

          ii) INSERT actions on tables with CHECK, UNIQUE, PRIMARY KEY, and FOREIGN KEY constraints.

          iii) INSERT actions in MERGE statements fired by triggers.

          iv) INSERT actions which fire triggers.

          v) MERGE statements with INSERT and DELETE actions operating together.

          The following behaviors have not been implemented/tested yet:

          a) UPDATE actions

          b) Permissions checking

          c) Dependency checking and statement invalidation

          So far the bulk of the complexity remains concentrated in compilation.

          Touches the following files:

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

          M java/engine/org/apache/derby/iapi/sql/execute/NoPutResultSet.java
          M java/engine/org/apache/derby/impl/sql/execute/TemporaryRowHolderResultSet.java

          Replaced a magic number with a named constant. Temporary tables don't have result set numbers assigned to them. But now they are being used to drive the INSERT/UPDATE/DELETE actions of MERGE statements. In order to get CHECK constraints to work properly, the row being checked is stuffed into slot 0 of the result set row array. That is usually the result set number corresponding to the driving result set of an INSERT/UPDATE/DELETE statement. And it was, coincidentally, the magic result set number of temporary tables. This decision may need to be re-visited if we invent other situations for which temporary tables need result set numbers.

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

          M java/engine/org/apache/derby/iapi/sql/dictionary/TableDescriptor.java

          Improved a method header comment.

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

          M java/engine/org/apache/derby/impl/sql/compile/ResultSetNode.java

          Added a new way to determine the data type of a ResultColumn. There is now a path through the pinball machine where the old technique didn't work.

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

          M java/engine/org/apache/derby/impl/sql/compile/MergeNode.java
          M java/engine/org/apache/derby/impl/sql/compile/MatchingClauseNode.java
          M java/engine/org/apache/derby/impl/sql/compile/InsertNode.java

          Compilation support for the INSERT action of a MERGE statement. The bulk of the new complexity has to do with generating the rows which are stuffed into a temporary table at execution time. There is considerable complexity involved in handling the DEFAULT keyword, which can appear as the value to be inserted into an identity, generated, or default column, as for instance, in this situation:

          { create table t1_007 ( c1 int generated always as identity, c2 int, c3 int generated always as ( c1 + c2 ), c1_4 int, c5 int default 1000 ); ... merge into t1_007 using t2_007 on t1_007.c2 = t2_007.c2 when not matched and t2_007.c5 = 'three' then insert ( c1, c2, c3, c1_4, c5 ) values ( default, 100 * t2_007.c2, default, t2_007.c3, default ); }

          For an ordinary INSERT statement, much of that complexity is handled by the compilation of the driving SELECT. For a MERGE statement, that SELECT is a dummy statement which is not optimized or generated. That, in turn, short-circuits some of the complex handling of the DEFAULT keyword. So that logic had to be reproduced in MatchingClauseNode.

          There is already too much complexity in the handling of these special columns and now I have made the situation even more complicated. In particular, I don't understand why GENERATED BY DEFAULT AS IDENTITY and GENERATED ALWAYS AS IDENTITY take such different trajectories through the pinball machine. And I don't understand why the special columns are compiled with the driving SELECT rather than with the INSERT/UPDATE action itself. This is a source of brittleness which we should be aware of. However, I thought that cleaning this up was outside the scope of this JIRA.

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

          M java/engine/org/apache/derby/impl/sql/compile/DeleteNode.java
          M java/engine/org/apache/derby/impl/sql/compile/UpdateNode.java
          M java/engine/org/apache/derby/impl/sql/execute/WriteCursorConstantAction.java
          M java/engine/org/apache/derby/impl/sql/execute/DMLVTIResultSet.java
          M java/engine/org/apache/derby/impl/sql/execute/UpdateVTIResultSet.java
          M java/engine/org/apache/derby/impl/sql/execute/CallStatementResultSet.java
          M java/engine/org/apache/derby/impl/sql/execute/UpdatableVTIConstantAction.java
          M java/engine/org/apache/derby/impl/sql/execute/InsertVTIResultSet.java
          M java/engine/org/apache/derby/impl/sql/execute/DeleteResultSet.java
          M java/engine/org/apache/derby/impl/sql/execute/UpdateResultSet.java
          M java/engine/org/apache/derby/impl/sql/execute/MiscResultSet.java
          M java/engine/org/apache/derby/impl/sql/execute/NoRowsResultSetImpl.java
          M java/engine/org/apache/derby/impl/sql/execute/DeleteConstantAction.java
          M java/engine/org/apache/derby/impl/sql/execute/DeleteVTIResultSet.java
          M java/engine/org/apache/derby/impl/sql/execute/GenericConstantActionFactory.java
          M java/engine/org/apache/derby/impl/sql/execute/UpdateConstantAction.java

          Added a small amount of logic so that INSERT/UPDATE/DELETE actions know at execution time whether they are running under a MERGE statement. If so, then we don't close the Activation at the end of the action. The Activation is closed by the MergeResultSet at the very end after all of the actions have run. This, in turn, makes it possible to run multiple INSERT/DELETE/UPDATE actions in a single MERGE statement.

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

          M java/engine/org/apache/derby/impl/sql/compile/DMLModStatementNode.java

          Added some special processing for generation clauses invoked by actions of MERGE statements. This is more of the fallout from the fact that the dummy SELECTs are not optimized and generated.

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

          M java/engine/org/apache/derby/impl/sql/compile/ResultColumnList.java

          Abstracted some logic into a separate method so that it could be called when generating the temporary row for an INSERT action of a MERGE statement.

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

          M java/engine/org/apache/derby/impl/sql/execute/InsertResultSet.java
          M java/engine/org/apache/derby/impl/sql/execute/MergeResultSet.java
          M java/engine/org/apache/derby/impl/sql/execute/InsertConstantAction.java
          M java/engine/org/apache/derby/impl/sql/execute/MatchingClauseConstantAction.java

          Changes to support INSERT actions under MERGE statements.

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

          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java

          Tests for the new behavior.

          Show
          Rick Hillegas added a comment - Attaching derby-3155-07-ad-insertAction.diff. This patch adds support for the INSERT action of MERGE statements. I am running tests now. The patch includes tests for the following use-cases: i) INSERT actions with DEFAULT expressions for identity, generated, and default columns ii) INSERT actions on tables with CHECK, UNIQUE, PRIMARY KEY, and FOREIGN KEY constraints. iii) INSERT actions in MERGE statements fired by triggers. iv) INSERT actions which fire triggers. v) MERGE statements with INSERT and DELETE actions operating together. The following behaviors have not been implemented/tested yet: a) UPDATE actions b) Permissions checking c) Dependency checking and statement invalidation So far the bulk of the complexity remains concentrated in compilation. Touches the following files: --------------- M java/engine/org/apache/derby/iapi/sql/execute/NoPutResultSet.java M java/engine/org/apache/derby/impl/sql/execute/TemporaryRowHolderResultSet.java Replaced a magic number with a named constant. Temporary tables don't have result set numbers assigned to them. But now they are being used to drive the INSERT/UPDATE/DELETE actions of MERGE statements. In order to get CHECK constraints to work properly, the row being checked is stuffed into slot 0 of the result set row array. That is usually the result set number corresponding to the driving result set of an INSERT/UPDATE/DELETE statement. And it was, coincidentally, the magic result set number of temporary tables. This decision may need to be re-visited if we invent other situations for which temporary tables need result set numbers. --------------- M java/engine/org/apache/derby/iapi/sql/dictionary/TableDescriptor.java Improved a method header comment. --------------- M java/engine/org/apache/derby/impl/sql/compile/ResultSetNode.java Added a new way to determine the data type of a ResultColumn. There is now a path through the pinball machine where the old technique didn't work. --------------- M java/engine/org/apache/derby/impl/sql/compile/MergeNode.java M java/engine/org/apache/derby/impl/sql/compile/MatchingClauseNode.java M java/engine/org/apache/derby/impl/sql/compile/InsertNode.java Compilation support for the INSERT action of a MERGE statement. The bulk of the new complexity has to do with generating the rows which are stuffed into a temporary table at execution time. There is considerable complexity involved in handling the DEFAULT keyword, which can appear as the value to be inserted into an identity, generated, or default column, as for instance, in this situation: { create table t1_007 ( c1 int generated always as identity, c2 int, c3 int generated always as ( c1 + c2 ), c1_4 int, c5 int default 1000 ); ... merge into t1_007 using t2_007 on t1_007.c2 = t2_007.c2 when not matched and t2_007.c5 = 'three' then insert ( c1, c2, c3, c1_4, c5 ) values ( default, 100 * t2_007.c2, default, t2_007.c3, default ); } For an ordinary INSERT statement, much of that complexity is handled by the compilation of the driving SELECT. For a MERGE statement, that SELECT is a dummy statement which is not optimized or generated. That, in turn, short-circuits some of the complex handling of the DEFAULT keyword. So that logic had to be reproduced in MatchingClauseNode. There is already too much complexity in the handling of these special columns and now I have made the situation even more complicated. In particular, I don't understand why GENERATED BY DEFAULT AS IDENTITY and GENERATED ALWAYS AS IDENTITY take such different trajectories through the pinball machine. And I don't understand why the special columns are compiled with the driving SELECT rather than with the INSERT/UPDATE action itself. This is a source of brittleness which we should be aware of. However, I thought that cleaning this up was outside the scope of this JIRA. --------------- M java/engine/org/apache/derby/impl/sql/compile/DeleteNode.java M java/engine/org/apache/derby/impl/sql/compile/UpdateNode.java M java/engine/org/apache/derby/impl/sql/execute/WriteCursorConstantAction.java M java/engine/org/apache/derby/impl/sql/execute/DMLVTIResultSet.java M java/engine/org/apache/derby/impl/sql/execute/UpdateVTIResultSet.java M java/engine/org/apache/derby/impl/sql/execute/CallStatementResultSet.java M java/engine/org/apache/derby/impl/sql/execute/UpdatableVTIConstantAction.java M java/engine/org/apache/derby/impl/sql/execute/InsertVTIResultSet.java M java/engine/org/apache/derby/impl/sql/execute/DeleteResultSet.java M java/engine/org/apache/derby/impl/sql/execute/UpdateResultSet.java M java/engine/org/apache/derby/impl/sql/execute/MiscResultSet.java M java/engine/org/apache/derby/impl/sql/execute/NoRowsResultSetImpl.java M java/engine/org/apache/derby/impl/sql/execute/DeleteConstantAction.java M java/engine/org/apache/derby/impl/sql/execute/DeleteVTIResultSet.java M java/engine/org/apache/derby/impl/sql/execute/GenericConstantActionFactory.java M java/engine/org/apache/derby/impl/sql/execute/UpdateConstantAction.java Added a small amount of logic so that INSERT/UPDATE/DELETE actions know at execution time whether they are running under a MERGE statement. If so, then we don't close the Activation at the end of the action. The Activation is closed by the MergeResultSet at the very end after all of the actions have run. This, in turn, makes it possible to run multiple INSERT/DELETE/UPDATE actions in a single MERGE statement. --------------- M java/engine/org/apache/derby/impl/sql/compile/DMLModStatementNode.java Added some special processing for generation clauses invoked by actions of MERGE statements. This is more of the fallout from the fact that the dummy SELECTs are not optimized and generated. --------------- M java/engine/org/apache/derby/impl/sql/compile/ResultColumnList.java Abstracted some logic into a separate method so that it could be called when generating the temporary row for an INSERT action of a MERGE statement. --------------- M java/engine/org/apache/derby/impl/sql/execute/InsertResultSet.java M java/engine/org/apache/derby/impl/sql/execute/MergeResultSet.java M java/engine/org/apache/derby/impl/sql/execute/InsertConstantAction.java M java/engine/org/apache/derby/impl/sql/execute/MatchingClauseConstantAction.java Changes to support INSERT actions under MERGE statements. --------------- M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java Tests for the new behavior.
          Rick Hillegas made changes -
          Attachment derby-3155-07-ad-insertAction.diff [ 12612829 ]
          Hide
          Rick Hillegas added a comment -

          Tests passed cleanly for me on derby-3155-07-ad-insertAction.diff.

          Show
          Rick Hillegas added a comment - Tests passed cleanly for me on derby-3155-07-ad-insertAction.diff.
          Hide
          ASF subversion and git services added a comment -

          Commit 1540713 from Rick Hillegas in branch 'code/trunk'
          [ https://svn.apache.org/r1540713 ]

          DERBY-3155: Add support for INSERT action of MERGE statement; commit derby-3155-07-ad-insertAction.diff.

          Show
          ASF subversion and git services added a comment - Commit 1540713 from Rick Hillegas in branch 'code/trunk' [ https://svn.apache.org/r1540713 ] DERBY-3155 : Add support for INSERT action of MERGE statement; commit derby-3155-07-ad-insertAction.diff.
          Rick Hillegas made changes -
          Link This issue is related to DERBY-6414 [ DERBY-6414 ]
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-08-ah-updateAction.diff. This patch adds basic support for the UPDATE action of MERGE statements. I am running tests now.

          This patch includes tests to stress simple examples of the following scenarios:

          1) Target tables with identity, generated, and default columns.

          2) Target tables with before/after row/statement UPDATE triggers.

          3) Target tables with primary, foreign, and check constraints.

          4) Source tables which are the OLD and NEW transition tables of triggers.

          5) MERGE statements combining all three kinds of actions: INSERT, UPDATE, and DELETE.

          When setting an identity column to the literal DEFAULT value, the UPDATE action of a MERGE statement behaves in the same incorrect way as an ordinary UPDATE statement. This behavior should be corrected when we fix DERBY-6414.

          Touches the following files:

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

          M java/engine/org/apache/derby/impl/sql/compile/MergeNode.java
          M java/engine/org/apache/derby/impl/sql/compile/MatchingClauseNode.java

          Bulk of the compile-time changes needed to support UPDATE actions.

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

          M java/engine/org/apache/derby/impl/sql/compile/UpdateNode.java

          Changes to distinguish MERGE actions from UPDATEs driven by VTIs. It seems that trigger transition tables are modelled as VTIs when they drive UPDATEs.

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

          M java/engine/org/apache/derby/impl/sql/compile/FromList.java
          M java/engine/org/apache/derby/impl/sql/compile/FromBaseTable.java

          The binding of column references now involves poking the table name into the reference. Later on, this makes it possible to associate bound columns with their base tables when code-generating the temporary rows which are buffered up for UPDATE actions.

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

          M java/engine/org/apache/derby/impl/sql/compile/DMLModStatementNode.java

          Up until now, it has not been necessary to optimize the dummy SELECT which is cooked up while compiling INSERT/DELETE actions. However, we need to pre-process that SELECT for UPDATE actions. That is because an important re-mapping of column references happens during pre-processing. That re-mapping is needed in order to correctly code-generate CHECK constraints.

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

          M java/engine/org/apache/derby/impl/sql/execute/DMLVTIResultSet.java
          M java/engine/org/apache/derby/impl/sql/execute/InsertResultSet.java
          M java/engine/org/apache/derby/impl/sql/execute/DMLWriteResultSet.java
          M java/engine/org/apache/derby/impl/sql/execute/DeleteResultSet.java

          The ResultDescription field, common across all of the leaf DML classes, has been pulled up into their common superclass. This makes it possible to pull some normalization logic up into the superclass for sharing between the INSERT and UPDATE actions of MERGE statements.

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

          M java/engine/org/apache/derby/impl/sql/execute/UpdateResultSet.java

          Normalize the rows of UPDATE actions in order to enforce storability restrictions.

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

          M java/engine/org/apache/derby/impl/store/access/heap/HeapRowLocation.java

          The RowLocations returned by the driving left join now implement RefDataValue. This eliminates a ClassCastException at run time.

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

          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java

          New tests for the new functionality.

          Show
          Rick Hillegas added a comment - Attaching derby-3155-08-ah-updateAction.diff. This patch adds basic support for the UPDATE action of MERGE statements. I am running tests now. This patch includes tests to stress simple examples of the following scenarios: 1) Target tables with identity, generated, and default columns. 2) Target tables with before/after row/statement UPDATE triggers. 3) Target tables with primary, foreign, and check constraints. 4) Source tables which are the OLD and NEW transition tables of triggers. 5) MERGE statements combining all three kinds of actions: INSERT, UPDATE, and DELETE. When setting an identity column to the literal DEFAULT value, the UPDATE action of a MERGE statement behaves in the same incorrect way as an ordinary UPDATE statement. This behavior should be corrected when we fix DERBY-6414 . Touches the following files: --------------------- M java/engine/org/apache/derby/impl/sql/compile/MergeNode.java M java/engine/org/apache/derby/impl/sql/compile/MatchingClauseNode.java Bulk of the compile-time changes needed to support UPDATE actions. --------------------- M java/engine/org/apache/derby/impl/sql/compile/UpdateNode.java Changes to distinguish MERGE actions from UPDATEs driven by VTIs. It seems that trigger transition tables are modelled as VTIs when they drive UPDATEs. --------------------- M java/engine/org/apache/derby/impl/sql/compile/FromList.java M java/engine/org/apache/derby/impl/sql/compile/FromBaseTable.java The binding of column references now involves poking the table name into the reference. Later on, this makes it possible to associate bound columns with their base tables when code-generating the temporary rows which are buffered up for UPDATE actions. --------------------- M java/engine/org/apache/derby/impl/sql/compile/DMLModStatementNode.java Up until now, it has not been necessary to optimize the dummy SELECT which is cooked up while compiling INSERT/DELETE actions. However, we need to pre-process that SELECT for UPDATE actions. That is because an important re-mapping of column references happens during pre-processing. That re-mapping is needed in order to correctly code-generate CHECK constraints. --------------------- M java/engine/org/apache/derby/impl/sql/execute/DMLVTIResultSet.java M java/engine/org/apache/derby/impl/sql/execute/InsertResultSet.java M java/engine/org/apache/derby/impl/sql/execute/DMLWriteResultSet.java M java/engine/org/apache/derby/impl/sql/execute/DeleteResultSet.java The ResultDescription field, common across all of the leaf DML classes, has been pulled up into their common superclass. This makes it possible to pull some normalization logic up into the superclass for sharing between the INSERT and UPDATE actions of MERGE statements. --------------------- M java/engine/org/apache/derby/impl/sql/execute/UpdateResultSet.java Normalize the rows of UPDATE actions in order to enforce storability restrictions. --------------------- M java/engine/org/apache/derby/impl/store/access/heap/HeapRowLocation.java The RowLocations returned by the driving left join now implement RefDataValue. This eliminates a ClassCastException at run time. --------------------- M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java New tests for the new functionality.
          Rick Hillegas made changes -
          Attachment derby-3155-08-ah-updateAction.diff [ 12615364 ]
          Hide
          Rick Hillegas added a comment -

          Tests passed cleanly for me on derby-3155-08-ah-updateAction.diff

          Show
          Rick Hillegas added a comment - Tests passed cleanly for me on derby-3155-08-ah-updateAction.diff
          Hide
          ASF subversion and git services added a comment -

          Commit 1545343 from Rick Hillegas in branch 'code/trunk'
          [ https://svn.apache.org/r1545343 ]

          DERBY-3155: Add support for UPDATE action of MERGE statement: derby-3155-08-ah-updateAction.diff.

          Show
          ASF subversion and git services added a comment - Commit 1545343 from Rick Hillegas in branch 'code/trunk' [ https://svn.apache.org/r1545343 ] DERBY-3155 : Add support for UPDATE action of MERGE statement: derby-3155-08-ah-updateAction.diff.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-09-aa-correlationNames.diff. This patch adds better support for correlation names in MERGE statements. I am running tests now.

          Correlation names on target tables were confusing the bind phase. The fix is to replace the correlation names with the actual base table name in all ColumnReferences in all expressions in the MERGE statement. This replacement is done before any of those expressions are bound.

          Touches the following files:

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

          M java/engine/org/apache/derby/impl/sql/compile/MergeNode.java
          M java/engine/org/apache/derby/impl/sql/compile/MatchingClauseNode.java

          The fix.

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

          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java

          Corresponding test cases.

          Show
          Rick Hillegas added a comment - Attaching derby-3155-09-aa-correlationNames.diff. This patch adds better support for correlation names in MERGE statements. I am running tests now. Correlation names on target tables were confusing the bind phase. The fix is to replace the correlation names with the actual base table name in all ColumnReferences in all expressions in the MERGE statement. This replacement is done before any of those expressions are bound. Touches the following files: --------------- M java/engine/org/apache/derby/impl/sql/compile/MergeNode.java M java/engine/org/apache/derby/impl/sql/compile/MatchingClauseNode.java The fix. --------------- M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java Corresponding test cases.
          Rick Hillegas made changes -
          Attachment derby-3155-09-aa-correlationNames.diff [ 12616829 ]
          Hide
          Rick Hillegas added a comment -

          Tests passed cleanly for me on derby-3155-09-aa-correlationNames.diff.

          Show
          Rick Hillegas added a comment - Tests passed cleanly for me on derby-3155-09-aa-correlationNames.diff.
          Hide
          ASF subversion and git services added a comment -

          Commit 1547585 from Rick Hillegas in branch 'code/trunk'
          [ https://svn.apache.org/r1547585 ]

          DERBY-3155: Improve handling of correlation names for the target tables in MERGE statements; commit derby-3155-09-aa-correlationNames.diff.

          Show
          ASF subversion and git services added a comment - Commit 1547585 from Rick Hillegas in branch 'code/trunk' [ https://svn.apache.org/r1547585 ] DERBY-3155 : Improve handling of correlation names for the target tables in MERGE statements; commit derby-3155-09-aa-correlationNames.diff.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-10-aa-correlationNames.diff. This patch improves the handling of correlation names for the source tables of MERGE statements. I am running tests now.

          This patch makes changes to the handling of source table references similar to those made for target table references in the previous patch.

          Touches the following files:

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

          M java/engine/org/apache/derby/impl/sql/compile/MergeNode.java
          M java/engine/org/apache/derby/impl/sql/compile/MatchingClauseNode.java
          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java

          Show
          Rick Hillegas added a comment - Attaching derby-3155-10-aa-correlationNames.diff. This patch improves the handling of correlation names for the source tables of MERGE statements. I am running tests now. This patch makes changes to the handling of source table references similar to those made for target table references in the previous patch. Touches the following files: ----------------- M java/engine/org/apache/derby/impl/sql/compile/MergeNode.java M java/engine/org/apache/derby/impl/sql/compile/MatchingClauseNode.java M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java
          Rick Hillegas made changes -
          Attachment derby-3155-10-aa-correlationNames.diff [ 12617203 ]
          Hide
          ASF subversion and git services added a comment -

          Commit 1548298 from Rick Hillegas in branch 'code/trunk'
          [ https://svn.apache.org/r1548298 ]

          DERBY-3155: Improve support for correlation names for the source tables of MERGE statements; tests passed cleanly for me on derby-3155-10-aa-correlationNames.diff.

          Show
          ASF subversion and git services added a comment - Commit 1548298 from Rick Hillegas in branch 'code/trunk' [ https://svn.apache.org/r1548298 ] DERBY-3155 : Improve support for correlation names for the source tables of MERGE statements; tests passed cleanly for me on derby-3155-10-aa-correlationNames.diff.
          Rick Hillegas made changes -
          Link This issue is related to DERBY-6428 [ DERBY-6428 ]
          Rick Hillegas made changes -
          Link This issue is related to DERBY-6429 [ DERBY-6429 ]
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-11-ab-beforeTriggersCantFireMerge.diff. This patch adds tests to verify that BEFORE triggers can't fire MERGE statements, just as they can't fire INSERT/UPDATE/DELETE statements.

          Touches the following file:

          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java

          Show
          Rick Hillegas added a comment - Attaching derby-3155-11-ab-beforeTriggersCantFireMerge.diff. This patch adds tests to verify that BEFORE triggers can't fire MERGE statements, just as they can't fire INSERT/UPDATE/DELETE statements. Touches the following file: M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java
          Rick Hillegas made changes -
          Hide
          ASF subversion and git services added a comment -

          Commit 1549948 from Rick Hillegas in branch 'code/trunk'
          [ https://svn.apache.org/r1549948 ]

          DERBY-3155: Add tests to verify that BEFORE triggers can't fire MERGE statements, just as they can't fire INSERT/UPDATE/DELETE statements; commit derby-3155-11-ab-beforeTriggersCantFireMerge.diff.

          Show
          ASF subversion and git services added a comment - Commit 1549948 from Rick Hillegas in branch 'code/trunk' [ https://svn.apache.org/r1549948 ] DERBY-3155 : Add tests to verify that BEFORE triggers can't fire MERGE statements, just as they can't fire INSERT/UPDATE/DELETE statements; commit derby-3155-11-ab-beforeTriggersCantFireMerge.diff.
          Hide
          Knut Anders Hatlen added a comment -

          Hi Rick,

          I got a NullPointerException from a MERGE statement on trunk:

          ij version 10.11
          ij> connect 'jdbc:derby:memory:db;create=true';
          ij> create table t1 (x int, y int);
          0 rows inserted/updated/deleted
          ij> create table t2 (x int);
          0 rows inserted/updated/deleted
          ij> merge into t1 using t2 on t1.x = t2.x when matched then update set y = -y when not matched then insert values (t2.x, 42);
          ERROR XJ001: Java exception: ': java.lang.NullPointerException'.
          java.lang.NullPointerException
          	at org.apache.derby.impl.sql.compile.MatchingClauseNode.bindInsertValues(MatchingClauseNode.java:783)
          	at org.apache.derby.impl.sql.compile.MatchingClauseNode.bindInsert(MatchingClauseNode.java:738)
          	at org.apache.derby.impl.sql.compile.MatchingClauseNode.bind(MatchingClauseNode.java:224)
          	at org.apache.derby.impl.sql.compile.MergeNode.bindStatement(MergeNode.java:258)
          	at org.apache.derby.impl.sql.GenericStatement.prepMinion(GenericStatement.java:402)
          	at org.apache.derby.impl.sql.GenericStatement.prepare(GenericStatement.java:99)
          	at org.apache.derby.impl.sql.conn.GenericLanguageConnectionContext.prepareInternalStatement(GenericLanguageConnectionContext.java:1116)
          	at org.apache.derby.impl.jdbc.EmbedStatement.execute(EmbedStatement.java:682)
          	at org.apache.derby.impl.jdbc.EmbedStatement.execute(EmbedStatement.java:631)
          	at org.apache.derby.impl.tools.ij.ij.executeImmediate(ij.java:367)
          	at org.apache.derby.impl.tools.ij.utilMain.doCatch(utilMain.java:527)
          	at org.apache.derby.impl.tools.ij.utilMain.runScriptGuts(utilMain.java:369)
          	at org.apache.derby.impl.tools.ij.utilMain.go(utilMain.java:245)
          	at org.apache.derby.impl.tools.ij.Main.go(Main.java:229)
          	at org.apache.derby.impl.tools.ij.Main.mainCore(Main.java:184)
          	at org.apache.derby.impl.tools.ij.Main.main(Main.java:75)
          	at org.apache.derby.tools.ij.main(ij.java:59)
          
          Show
          Knut Anders Hatlen added a comment - Hi Rick, I got a NullPointerException from a MERGE statement on trunk: ij version 10.11 ij> connect 'jdbc:derby:memory:db;create=true'; ij> create table t1 (x int, y int); 0 rows inserted/updated/deleted ij> create table t2 (x int); 0 rows inserted/updated/deleted ij> merge into t1 using t2 on t1.x = t2.x when matched then update set y = -y when not matched then insert values (t2.x, 42); ERROR XJ001: Java exception: ': java.lang.NullPointerException'. java.lang.NullPointerException at org.apache.derby.impl.sql.compile.MatchingClauseNode.bindInsertValues(MatchingClauseNode.java:783) at org.apache.derby.impl.sql.compile.MatchingClauseNode.bindInsert(MatchingClauseNode.java:738) at org.apache.derby.impl.sql.compile.MatchingClauseNode.bind(MatchingClauseNode.java:224) at org.apache.derby.impl.sql.compile.MergeNode.bindStatement(MergeNode.java:258) at org.apache.derby.impl.sql.GenericStatement.prepMinion(GenericStatement.java:402) at org.apache.derby.impl.sql.GenericStatement.prepare(GenericStatement.java:99) at org.apache.derby.impl.sql.conn.GenericLanguageConnectionContext.prepareInternalStatement(GenericLanguageConnectionContext.java:1116) at org.apache.derby.impl.jdbc.EmbedStatement.execute(EmbedStatement.java:682) at org.apache.derby.impl.jdbc.EmbedStatement.execute(EmbedStatement.java:631) at org.apache.derby.impl.tools.ij.ij.executeImmediate(ij.java:367) at org.apache.derby.impl.tools.ij.utilMain.doCatch(utilMain.java:527) at org.apache.derby.impl.tools.ij.utilMain.runScriptGuts(utilMain.java:369) at org.apache.derby.impl.tools.ij.utilMain.go(utilMain.java:245) at org.apache.derby.impl.tools.ij.Main.go(Main.java:229) at org.apache.derby.impl.tools.ij.Main.mainCore(Main.java:184) at org.apache.derby.impl.tools.ij.Main.main(Main.java:75) at org.apache.derby.tools.ij.main(ij.java:59)
          Hide
          Knut Anders Hatlen added a comment -

          The NPE goes away if I specify the target column names in the mergeInsert clause, like this:

          ij(CONNECTION4)> merge into t1 using t2 on t1.x = t2.x when matched then update set y = -y when not matched then insert (x, y) values (t2.x, 42);
          0 rows inserted/updated/deleted
          WARNING 02000: No row was found for FETCH, UPDATE or DELETE; or the result of a query is an empty table.
          

          According to the functional spec, the target column names are optional in the mergeInsert clause.

          Show
          Knut Anders Hatlen added a comment - The NPE goes away if I specify the target column names in the mergeInsert clause, like this: ij(CONNECTION4)> merge into t1 using t2 on t1.x = t2.x when matched then update set y = -y when not matched then insert (x, y) values (t2.x, 42); 0 rows inserted/updated/deleted WARNING 02000: No row was found for FETCH, UPDATE or DELETE; or the result of a query is an empty table. According to the functional spec, the target column names are optional in the mergeInsert clause.
          Hide
          Rick Hillegas added a comment -

          Thanks for kicking the tires, Knut. I'll take a look at this when I pop off my permissions-related interrupt stack. Looks like I need to add a dummy insert column list if the user omits one. Thanks!

          Show
          Rick Hillegas added a comment - Thanks for kicking the tires, Knut. I'll take a look at this when I pop off my permissions-related interrupt stack. Looks like I need to add a dummy insert column list if the user omits one. Thanks!
          Hide
          Dyre Tjeldvoll added a comment - - edited

          Hi Rick,
          it seems that if you have a targetCorrelationName that's different from targetTable you get an error if you're referring to targetCorrelationName in a mergeWhenClause. Using it in searchCondition is ok:

          Exception in thread "main" java.sql.SQLSyntaxErrorException: Table name 'M' should be the same as 'META'.
          at org.apache.derby.impl.jdbc.SQLExceptionFactory.getSQLException(SQLExceptionFactory.java:93)
          at org.apache.derby.impl.jdbc.Util.generateCsSQLException(Util.java:288)
          at org.apache.derby.impl.jdbc.TransactionResourceImpl.wrapInSQLException(TransactionResourceImpl.java:424)
          at org.apache.derby.impl.jdbc.TransactionResourceImpl.handleException(TransactionResourceImpl.java:353)
          at org.apache.derby.impl.jdbc.EmbedConnection.handleException(EmbedConnection.java:2396)
          at org.apache.derby.impl.jdbc.ConnectionChild.handleException(ConnectionChild.java:82)
          at org.apache.derby.impl.jdbc.EmbedStatement.execute(EmbedStatement.java:691)
          at org.apache.derby.impl.jdbc.EmbedStatement.executeLargeUpdate(EmbedStatement.java:181)
          at org.apache.derby.impl.jdbc.EmbedStatement.executeUpdate(EmbedStatement.java:170)
          at regexp.Driver.main(Driver.java:133)
          Caused by: java.sql.SQLException: Table name 'M' should be the same as 'META'.
          at org.apache.derby.impl.jdbc.SQLExceptionFactory.wrapArgsForTransportAcrossDRDA(SQLExceptionFactory.java:138)
          at org.apache.derby.impl.jdbc.SQLExceptionFactory.getSQLException(SQLExceptionFactory.java:73)
          ... 9 more
          Caused by: ERROR 42X55: Table name 'M' should be the same as 'META'.
          at org.apache.derby.iapi.error.StandardException.newException(StandardException.java:265)
          at org.apache.derby.iapi.error.StandardException.newException(StandardException.java:260)
          at org.apache.derby.impl.sql.compile.ResultColumn.setColumnDescriptor(ResultColumn.java:915)
          at org.apache.derby.impl.sql.compile.ResultColumn.bindResultColumnByName(ResultColumn.java:834)
          at org.apache.derby.impl.sql.compile.ResultColumnList.bindResultColumnsByName(ResultColumnList.java:890)
          at org.apache.derby.impl.sql.compile.InsertNode.bindStatement(InsertNode.java:282)
          at org.apache.derby.impl.sql.compile.MatchingClauseNode.bindInsert(MatchingClauseNode.java:770)
          at org.apache.derby.impl.sql.compile.MatchingClauseNode.bind(MatchingClauseNode.java:224)
          at org.apache.derby.impl.sql.compile.MergeNode.bindStatement(MergeNode.java:258)
          at org.apache.derby.impl.sql.GenericStatement.prepMinion(GenericStatement.java:402)
          at org.apache.derby.impl.sql.GenericStatement.prepare(GenericStatement.java:99)
          at org.apache.derby.impl.sql.conn.GenericLanguageConnectionContext.prepareInternalStatement(GenericLanguageConnectionContext.java:1116)
          at org.apache.derby.impl.jdbc.EmbedStatement.execute(EmbedStatement.java:682)
          ... 3 more

          Here is a script for this problem:

          connect 'jdbc:derby:memory:db;create=true';
          
          create table t1( a int, b int );
          create table t2( c int, d int );
          
          insert into t1 values ( 1, 100 ), ( 2, 200 );
          insert into t2 values ( 1, 100 ), (2, 200), ( 3, 300 );
          
          merge into t1 x
          using t2 y on x.a = y.a
          when matched and x.b > 100 then update set x.b = y.d;
          
          Show
          Dyre Tjeldvoll added a comment - - edited Hi Rick, it seems that if you have a targetCorrelationName that's different from targetTable you get an error if you're referring to targetCorrelationName in a mergeWhenClause . Using it in searchCondition is ok: Exception in thread "main" java.sql.SQLSyntaxErrorException: Table name 'M' should be the same as 'META'. at org.apache.derby.impl.jdbc.SQLExceptionFactory.getSQLException(SQLExceptionFactory.java:93) at org.apache.derby.impl.jdbc.Util.generateCsSQLException(Util.java:288) at org.apache.derby.impl.jdbc.TransactionResourceImpl.wrapInSQLException(TransactionResourceImpl.java:424) at org.apache.derby.impl.jdbc.TransactionResourceImpl.handleException(TransactionResourceImpl.java:353) at org.apache.derby.impl.jdbc.EmbedConnection.handleException(EmbedConnection.java:2396) at org.apache.derby.impl.jdbc.ConnectionChild.handleException(ConnectionChild.java:82) at org.apache.derby.impl.jdbc.EmbedStatement.execute(EmbedStatement.java:691) at org.apache.derby.impl.jdbc.EmbedStatement.executeLargeUpdate(EmbedStatement.java:181) at org.apache.derby.impl.jdbc.EmbedStatement.executeUpdate(EmbedStatement.java:170) at regexp.Driver.main(Driver.java:133) Caused by: java.sql.SQLException: Table name 'M' should be the same as 'META'. at org.apache.derby.impl.jdbc.SQLExceptionFactory.wrapArgsForTransportAcrossDRDA(SQLExceptionFactory.java:138) at org.apache.derby.impl.jdbc.SQLExceptionFactory.getSQLException(SQLExceptionFactory.java:73) ... 9 more Caused by: ERROR 42X55: Table name 'M' should be the same as 'META'. at org.apache.derby.iapi.error.StandardException.newException(StandardException.java:265) at org.apache.derby.iapi.error.StandardException.newException(StandardException.java:260) at org.apache.derby.impl.sql.compile.ResultColumn.setColumnDescriptor(ResultColumn.java:915) at org.apache.derby.impl.sql.compile.ResultColumn.bindResultColumnByName(ResultColumn.java:834) at org.apache.derby.impl.sql.compile.ResultColumnList.bindResultColumnsByName(ResultColumnList.java:890) at org.apache.derby.impl.sql.compile.InsertNode.bindStatement(InsertNode.java:282) at org.apache.derby.impl.sql.compile.MatchingClauseNode.bindInsert(MatchingClauseNode.java:770) at org.apache.derby.impl.sql.compile.MatchingClauseNode.bind(MatchingClauseNode.java:224) at org.apache.derby.impl.sql.compile.MergeNode.bindStatement(MergeNode.java:258) at org.apache.derby.impl.sql.GenericStatement.prepMinion(GenericStatement.java:402) at org.apache.derby.impl.sql.GenericStatement.prepare(GenericStatement.java:99) at org.apache.derby.impl.sql.conn.GenericLanguageConnectionContext.prepareInternalStatement(GenericLanguageConnectionContext.java:1116) at org.apache.derby.impl.jdbc.EmbedStatement.execute(EmbedStatement.java:682) ... 3 more Here is a script for this problem: connect 'jdbc:derby:memory:db;create=true'; create table t1( a int, b int ); create table t2( c int, d int ); insert into t1 values ( 1, 100 ), ( 2, 200 ); insert into t2 values ( 1, 100 ), (2, 200), ( 3, 300 ); merge into t1 x using t2 y on x.a = y.a when matched and x.b > 100 then update set x.b = y.d;
          Hide
          Knut Anders Hatlen added a comment -

          If I use a system table as source table, I get an error message saying that the source must be a table, which it is:

          ij> create table t1(x int);
          0 rows inserted/updated/deleted
          ij> merge into t1 using sysibm.sysdummy1 on true when not matched then insert (x) values (1);
          ERROR 42XAL: The source table of a MERGE statement must be a base table, view, or table function.
          ij> merge into t1 using sysibm.sysdummy1 s on true when not matched then insert (x) values (1);
          ERROR 42XAL: The source table of a MERGE statement must be a base table, view, or table function.
          ij> merge into t1 using sys.systables on true when not matched then insert (x) values (1);
          ERROR 42XAL: The source table of a MERGE statement must be a base table, view, or table function.
          
          Show
          Knut Anders Hatlen added a comment - If I use a system table as source table, I get an error message saying that the source must be a table, which it is: ij> create table t1(x int); 0 rows inserted/updated/deleted ij> merge into t1 using sysibm.sysdummy1 on true when not matched then insert (x) values (1); ERROR 42XAL: The source table of a MERGE statement must be a base table, view, or table function. ij> merge into t1 using sysibm.sysdummy1 s on true when not matched then insert (x) values (1); ERROR 42XAL: The source table of a MERGE statement must be a base table, view, or table function. ij> merge into t1 using sys.systables on true when not matched then insert (x) values (1); ERROR 42XAL: The source table of a MERGE statement must be a base table, view, or table function.
          Hide
          Knut Anders Hatlen added a comment -

          Actually, the message says "base table". Maybe system tables don't count as base tables?

          Show
          Knut Anders Hatlen added a comment - Actually, the message says "base table". Maybe system tables don't count as base tables?
          Hide
          Knut Anders Hatlen added a comment -

          I've discovered one more problem:

          ij> create view singlerow(x) as values 1;
          0 rows inserted/updated/deleted
          ij> create table t1(x int);
          0 rows inserted/updated/deleted
          ij> create table t2(y int);
          0 rows inserted/updated/deleted
          ij> create trigger tr after insert on t1 referencing new as new for each row merge into t2 using singlerow on t2.y = new.x when not matched then insert (y) values (new.x);
          0 rows inserted/updated/deleted
          ij> insert into t1 values 1,2,3,4,5,4,3,2,1,1,1,2,3,100;
          ERROR XJ001: Java exception: 'ASSERT FAILED ProjectRestrictResultSet already open: org.apache.derby.shared.common.sanity.AssertFailure'. (errorCode = 0)
          
          Show
          Knut Anders Hatlen added a comment - I've discovered one more problem: ij> create view singlerow(x) as values 1; 0 rows inserted/updated/deleted ij> create table t1(x int); 0 rows inserted/updated/deleted ij> create table t2(y int); 0 rows inserted/updated/deleted ij> create trigger tr after insert on t1 referencing new as new for each row merge into t2 using singlerow on t2.y = new.x when not matched then insert (y) values (new.x); 0 rows inserted/updated/deleted ij> insert into t1 values 1,2,3,4,5,4,3,2,1,1,1,2,3,100; ERROR XJ001: Java exception: 'ASSERT FAILED ProjectRestrictResultSet already open: org.apache.derby.shared.common.sanity.AssertFailure'. (errorCode = 0)
          Hide
          Rick Hillegas added a comment -

          Thanks, for kicking the tires, Knut and Dyre. Please keep finding problems. As I said, I'll take a look at these after I come up for air. Thanks!

          Show
          Rick Hillegas added a comment - Thanks, for kicking the tires, Knut and Dyre. Please keep finding problems. As I said, I'll take a look at these after I come up for air. Thanks!
          Hide
          Knut Anders Hatlen added a comment -

          Here's another NPE:

          ij> create table t1(x int);
          0 rows inserted/updated/deleted
          ij> create table t2(x int);
          0 rows inserted/updated/deleted
          ij> merge into t1 using t2 on t1.x=t2.x when matched then update set x = (select count(*) from t2);
          ERROR XJ001: Java exception: ': java.lang.NullPointerException'. (errorCode = 0)
          java.lang.NullPointerException
          	at org.apache.derby.impl.sql.compile.SelectNode.getFinalCostEstimate(SelectNode.java:2224)
          	at org.apache.derby.impl.sql.compile.SubqueryNode.generateExpression(SubqueryNode.java:1936)
          	at org.apache.derby.impl.sql.compile.ResultColumn.generateExpression(ResultColumn.java:1056)
          	at org.apache.derby.impl.sql.compile.ResultColumnList.generateEvaluatedRow(ResultColumnList.java:1452)
          	at org.apache.derby.impl.sql.compile.MatchingClauseNode.generateInsertUpdateRow(MatchingClauseNode.java:1255)
          	at org.apache.derby.impl.sql.compile.MatchingClauseNode.generate(MatchingClauseNode.java:1146)
          	at org.apache.derby.impl.sql.compile.MergeNode.generate(MergeNode.java:770)
          	at org.apache.derby.impl.sql.compile.StatementNode.generate(StatementNode.java:317)
          	at org.apache.derby.impl.sql.GenericStatement.prepMinion(GenericStatement.java:547)
          	at org.apache.derby.impl.sql.GenericStatement.prepare(GenericStatement.java:99)
          	at org.apache.derby.impl.sql.conn.GenericLanguageConnectionContext.prepareInternalStatement(GenericLanguageConnectionContext.java:1116)
          	at org.apache.derby.impl.jdbc.EmbedStatement.execute(EmbedStatement.java:682)
          	at org.apache.derby.impl.jdbc.EmbedStatement.execute(EmbedStatement.java:631)
          	at org.apache.derby.impl.tools.ij.ij.executeImmediate(ij.java:367)
          	at org.apache.derby.impl.tools.ij.utilMain.doCatch(utilMain.java:527)
          	at org.apache.derby.impl.tools.ij.utilMain.runScriptGuts(utilMain.java:369)
          	at org.apache.derby.impl.tools.ij.utilMain.go(utilMain.java:245)
          	at org.apache.derby.impl.tools.ij.Main.go(Main.java:229)
          	at org.apache.derby.impl.tools.ij.Main.mainCore(Main.java:184)
          	at org.apache.derby.impl.tools.ij.Main.main(Main.java:75)
          	at org.apache.derby.tools.ij.main(ij.java:59)
          
          Show
          Knut Anders Hatlen added a comment - Here's another NPE: ij> create table t1(x int); 0 rows inserted/updated/deleted ij> create table t2(x int); 0 rows inserted/updated/deleted ij> merge into t1 using t2 on t1.x=t2.x when matched then update set x = (select count(*) from t2); ERROR XJ001: Java exception: ': java.lang.NullPointerException'. (errorCode = 0) java.lang.NullPointerException at org.apache.derby.impl.sql.compile.SelectNode.getFinalCostEstimate(SelectNode.java:2224) at org.apache.derby.impl.sql.compile.SubqueryNode.generateExpression(SubqueryNode.java:1936) at org.apache.derby.impl.sql.compile.ResultColumn.generateExpression(ResultColumn.java:1056) at org.apache.derby.impl.sql.compile.ResultColumnList.generateEvaluatedRow(ResultColumnList.java:1452) at org.apache.derby.impl.sql.compile.MatchingClauseNode.generateInsertUpdateRow(MatchingClauseNode.java:1255) at org.apache.derby.impl.sql.compile.MatchingClauseNode.generate(MatchingClauseNode.java:1146) at org.apache.derby.impl.sql.compile.MergeNode.generate(MergeNode.java:770) at org.apache.derby.impl.sql.compile.StatementNode.generate(StatementNode.java:317) at org.apache.derby.impl.sql.GenericStatement.prepMinion(GenericStatement.java:547) at org.apache.derby.impl.sql.GenericStatement.prepare(GenericStatement.java:99) at org.apache.derby.impl.sql.conn.GenericLanguageConnectionContext.prepareInternalStatement(GenericLanguageConnectionContext.java:1116) at org.apache.derby.impl.jdbc.EmbedStatement.execute(EmbedStatement.java:682) at org.apache.derby.impl.jdbc.EmbedStatement.execute(EmbedStatement.java:631) at org.apache.derby.impl.tools.ij.ij.executeImmediate(ij.java:367) at org.apache.derby.impl.tools.ij.utilMain.doCatch(utilMain.java:527) at org.apache.derby.impl.tools.ij.utilMain.runScriptGuts(utilMain.java:369) at org.apache.derby.impl.tools.ij.utilMain.go(utilMain.java:245) at org.apache.derby.impl.tools.ij.Main.go(Main.java:229) at org.apache.derby.impl.tools.ij.Main.mainCore(Main.java:184) at org.apache.derby.impl.tools.ij.Main.main(Main.java:75) at org.apache.derby.tools.ij.main(ij.java:59)
          Hide
          Dyre Tjeldvoll added a comment -

          I tried modifying Knut's singlerow view idea to use an ordinary table with a single row in it:

          String q = "MERGE INTO META USING SINGLEROW "+
          "ON META.i = 42 "+
          "WHEN MATCHED THEN UPDATE SET META.c = 'U' "+
          "WHEN NOT MATCHED THEN INSERT (META.i, META.c) VALUES( 42, 'I' )";

          That triggers the following exception

          Caused by: ERROR XSAI2: The conglomerate (0) requested does not exist.
          at org.apache.derby.iapi.error.StandardException.newException(StandardException.java:265)
          at org.apache.derby.iapi.error.StandardException.newException(StandardException.java:260)
          at org.apache.derby.impl.store.access.heap.HeapConglomerateFactory.readConglomerate(HeapConglomerateFactory.java:254)
          at org.apache.derby.impl.store.access.CacheableConglomerate.setIdentity(CacheableConglomerate.java:102)
          at org.apache.derby.impl.services.cache.ConcurrentCache.find(ConcurrentCache.java:295)
          at org.apache.derby.impl.store.access.RAMAccessManager.conglomCacheFind(RAMAccessManager.java:466)
          at org.apache.derby.impl.store.access.RAMTransaction.findConglomerate(RAMTransaction.java:404)
          at org.apache.derby.impl.store.access.RAMTransaction.findExistingConglomerate(RAMTransaction.java:383)
          at org.apache.derby.impl.store.access.RAMTransaction.openConglomerate(RAMTransaction.java:1289)
          at org.apache.derby.impl.sql.compile.ResultColumnList.newRowLocationTemplate(ResultColumnList.java:1641)
          at org.apache.derby.impl.sql.compile.ResultColumnList.buildRowTemplate(ResultColumnList.java:1563)
          at org.apache.derby.impl.sql.compile.IndexToBaseRowNode.generate(IndexToBaseRowNode.java:238)
          at org.apache.derby.impl.sql.compile.ProjectRestrictNode.generateMinion(ProjectRestrictNode.java:1348)
          at org.apache.derby.impl.sql.compile.ProjectRestrictNode.generate(ProjectRestrictNode.java:1301)
          at org.apache.derby.impl.sql.compile.JoinNode.getJoinArguments(JoinNode.java:1648)
          at org.apache.derby.impl.sql.compile.JoinNode.generateCore(JoinNode.java:1624)
          at org.apache.derby.impl.sql.compile.JoinNode.generateCore(JoinNode.java:1561)
          at org.apache.derby.impl.sql.compile.HalfOuterJoinNode.generate(HalfOuterJoinNode.java:786)
          at org.apache.derby.impl.sql.compile.ProjectRestrictNode.generateMinion(ProjectRestrictNode.java:1348)
          at org.apache.derby.impl.sql.compile.ProjectRestrictNode.generate(ProjectRestrictNode.java:1301)
          at org.apache.derby.impl.sql.compile.ProjectRestrictNode.generateMinion(ProjectRestrictNode.java:1445)
          at org.apache.derby.impl.sql.compile.ProjectRestrictNode.generate(ProjectRestrictNode.java:1301)
          at org.apache.derby.impl.sql.compile.ScrollInsensitiveResultSetNode.generate(ScrollInsensitiveResultSetNode.java:86)
          at org.apache.derby.impl.sql.compile.CursorNode.generate(CursorNode.java:628)
          at org.apache.derby.impl.sql.compile.MergeNode.generate(MergeNode.java:759)
          at org.apache.derby.impl.sql.compile.StatementNode.generate(StatementNode.java:317)
          at org.apache.derby.impl.sql.GenericStatement.prepMinion(GenericStatement.java:547)
          at org.apache.derby.impl.sql.GenericStatement.prepare(GenericStatement.java:99)
          at org.apache.derby.impl.sql.conn.GenericLanguageConnectionContext.prepareInternalStatement(GenericLanguageConnectionContext.java:1116)
          at org.apache.derby.impl.jdbc.EmbedStatement.execute(EmbedStatement.java:682)

          Show
          Dyre Tjeldvoll added a comment - I tried modifying Knut's singlerow view idea to use an ordinary table with a single row in it: String q = "MERGE INTO META USING SINGLEROW "+ "ON META.i = 42 "+ "WHEN MATCHED THEN UPDATE SET META.c = 'U' "+ "WHEN NOT MATCHED THEN INSERT (META.i, META.c) VALUES( 42, 'I' )"; That triggers the following exception Caused by: ERROR XSAI2: The conglomerate (0) requested does not exist. at org.apache.derby.iapi.error.StandardException.newException(StandardException.java:265) at org.apache.derby.iapi.error.StandardException.newException(StandardException.java:260) at org.apache.derby.impl.store.access.heap.HeapConglomerateFactory.readConglomerate(HeapConglomerateFactory.java:254) at org.apache.derby.impl.store.access.CacheableConglomerate.setIdentity(CacheableConglomerate.java:102) at org.apache.derby.impl.services.cache.ConcurrentCache.find(ConcurrentCache.java:295) at org.apache.derby.impl.store.access.RAMAccessManager.conglomCacheFind(RAMAccessManager.java:466) at org.apache.derby.impl.store.access.RAMTransaction.findConglomerate(RAMTransaction.java:404) at org.apache.derby.impl.store.access.RAMTransaction.findExistingConglomerate(RAMTransaction.java:383) at org.apache.derby.impl.store.access.RAMTransaction.openConglomerate(RAMTransaction.java:1289) at org.apache.derby.impl.sql.compile.ResultColumnList.newRowLocationTemplate(ResultColumnList.java:1641) at org.apache.derby.impl.sql.compile.ResultColumnList.buildRowTemplate(ResultColumnList.java:1563) at org.apache.derby.impl.sql.compile.IndexToBaseRowNode.generate(IndexToBaseRowNode.java:238) at org.apache.derby.impl.sql.compile.ProjectRestrictNode.generateMinion(ProjectRestrictNode.java:1348) at org.apache.derby.impl.sql.compile.ProjectRestrictNode.generate(ProjectRestrictNode.java:1301) at org.apache.derby.impl.sql.compile.JoinNode.getJoinArguments(JoinNode.java:1648) at org.apache.derby.impl.sql.compile.JoinNode.generateCore(JoinNode.java:1624) at org.apache.derby.impl.sql.compile.JoinNode.generateCore(JoinNode.java:1561) at org.apache.derby.impl.sql.compile.HalfOuterJoinNode.generate(HalfOuterJoinNode.java:786) at org.apache.derby.impl.sql.compile.ProjectRestrictNode.generateMinion(ProjectRestrictNode.java:1348) at org.apache.derby.impl.sql.compile.ProjectRestrictNode.generate(ProjectRestrictNode.java:1301) at org.apache.derby.impl.sql.compile.ProjectRestrictNode.generateMinion(ProjectRestrictNode.java:1445) at org.apache.derby.impl.sql.compile.ProjectRestrictNode.generate(ProjectRestrictNode.java:1301) at org.apache.derby.impl.sql.compile.ScrollInsensitiveResultSetNode.generate(ScrollInsensitiveResultSetNode.java:86) at org.apache.derby.impl.sql.compile.CursorNode.generate(CursorNode.java:628) at org.apache.derby.impl.sql.compile.MergeNode.generate(MergeNode.java:759) at org.apache.derby.impl.sql.compile.StatementNode.generate(StatementNode.java:317) at org.apache.derby.impl.sql.GenericStatement.prepMinion(GenericStatement.java:547) at org.apache.derby.impl.sql.GenericStatement.prepare(GenericStatement.java:99) at org.apache.derby.impl.sql.conn.GenericLanguageConnectionContext.prepareInternalStatement(GenericLanguageConnectionContext.java:1116) at org.apache.derby.impl.jdbc.EmbedStatement.execute(EmbedStatement.java:682)
          Hide
          Knut Anders Hatlen added a comment -

          Here's another error:

          ij version 10.11
          ij> connect 'jdbc:derby:memory:db;create=true';
          ij> create table t1(x int, y varchar(100));
          0 rows inserted/updated/deleted
          ij> create table t2(x int);
          0 rows inserted/updated/deleted
          ij> insert into t2 values 1, 1, 2;
          3 rows inserted/updated/deleted
          ij> insert into t1 values (1, null), (2, '');
          2 rows inserted/updated/deleted
          ij> merge into t1 using t2 on exists (select * from t2) when matched then update set y = y || 'x';
          ERROR XSDA7: Restore of a serializable or SQLData object of class , attempted to read more data than was originally stored
          ERROR XJ001: Java exception: ': java.io.EOFException'.
          
          Show
          Knut Anders Hatlen added a comment - Here's another error: ij version 10.11 ij> connect 'jdbc:derby:memory:db;create=true'; ij> create table t1(x int, y varchar(100)); 0 rows inserted/updated/deleted ij> create table t2(x int); 0 rows inserted/updated/deleted ij> insert into t2 values 1, 1, 2; 3 rows inserted/updated/deleted ij> insert into t1 values (1, null), (2, ''); 2 rows inserted/updated/deleted ij> merge into t1 using t2 on exists (select * from t2) when matched then update set y = y || 'x'; ERROR XSDA7: Restore of a serializable or SQLData object of class , attempted to read more data than was originally stored ERROR XJ001: Java exception: ': java.io.EOFException'.
          Hide
          Knut Anders Hatlen added a comment -

          I'm able to reproduce the "conglomerate does not exist" error that Dyre mentioned:

          ij> create table t1(x int primary key);
          0 rows inserted/updated/deleted
          ij> create table t2(x int);
          0 rows inserted/updated/deleted
          ij> merge into t1 using t2 on t1.x = 42 when not matched then insert (x) values (42);
          ERROR XSAI2: The conglomerate (0) requested does not exist.
          
          Show
          Knut Anders Hatlen added a comment - I'm able to reproduce the "conglomerate does not exist" error that Dyre mentioned: ij> create table t1(x int primary key); 0 rows inserted/updated/deleted ij> create table t2(x int); 0 rows inserted/updated/deleted ij> merge into t1 using t2 on t1.x = 42 when not matched then insert (x) values (42); ERROR XSAI2: The conglomerate (0) requested does not exist.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-12-aa-canOmitInsertColumnList.diff. This patch fixes the following problem Knut found: you should be allowed to omit the INSERT column list in a WHEN NOT MATCHED clause if the number of values supplied in the VALUES subclause equals the number of columns in the table.

          Touches the following files:

          M java/engine/org/apache/derby/impl/sql/compile/MatchingClauseNode.java
          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java

          Show
          Rick Hillegas added a comment - Attaching derby-3155-12-aa-canOmitInsertColumnList.diff. This patch fixes the following problem Knut found: you should be allowed to omit the INSERT column list in a WHEN NOT MATCHED clause if the number of values supplied in the VALUES subclause equals the number of columns in the table. Touches the following files: M java/engine/org/apache/derby/impl/sql/compile/MatchingClauseNode.java M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java
          Rick Hillegas made changes -
          Hide
          ASF subversion and git services added a comment -

          Commit 1558871 from Rick Hillegas in branch 'code/trunk'
          [ https://svn.apache.org/r1558871 ]

          DERBY-3155: Make the INSERT column list optional in the WHEN NOT MATCHED clause of a MERGE statement provided that the number of values in the VALUES subclause equals the number of columns in the target table; commit derby-3155-12-aa-canOmitInsertColumnList.diff.

          Show
          ASF subversion and git services added a comment - Commit 1558871 from Rick Hillegas in branch 'code/trunk' [ https://svn.apache.org/r1558871 ] DERBY-3155 : Make the INSERT column list optional in the WHEN NOT MATCHED clause of a MERGE statement provided that the number of values in the VALUES subclause equals the number of columns in the target table; commit derby-3155-12-aa-canOmitInsertColumnList.diff.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-13-aa-allowSystemAndTempTables.diff. This patch addresses another problem Knut found: you couldn't use system tables as source tables.

          A couple small changes make the following operations possible:

          1) Using system tables as source tables

          2) Using temp tables as source tables and target tables

          The following is (and should be) illegal:

          3) Using system tables as target tables

          The following is still illegal but should be allowed. The fix, however, will be move involved:

          4) Using synonyms as source and target tables.

          Before addressing item 4, I would like to address other problems found by Knut and Dyre which I consider more important.

          Touches the following files:

          M java/engine/org/apache/derby/impl/sql/compile/MergeNode.java
          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java

          Show
          Rick Hillegas added a comment - Attaching derby-3155-13-aa-allowSystemAndTempTables.diff. This patch addresses another problem Knut found: you couldn't use system tables as source tables. A couple small changes make the following operations possible: 1) Using system tables as source tables 2) Using temp tables as source tables and target tables The following is (and should be) illegal: 3) Using system tables as target tables The following is still illegal but should be allowed. The fix, however, will be move involved: 4) Using synonyms as source and target tables. Before addressing item 4, I would like to address other problems found by Knut and Dyre which I consider more important. Touches the following files: M java/engine/org/apache/derby/impl/sql/compile/MergeNode.java M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java
          Rick Hillegas made changes -
          Hide
          ASF subversion and git services added a comment -

          Commit 1558912 from Rick Hillegas in branch 'code/trunk'
          [ https://svn.apache.org/r1558912 ]

          DERBY-3155: Allow system and temp tables in MERGE statements; commit derby-3155-13-aa-allowSystemAndTempTables.diff.

          Show
          ASF subversion and git services added a comment - Commit 1558912 from Rick Hillegas in branch 'code/trunk' [ https://svn.apache.org/r1558912 ] DERBY-3155 : Allow system and temp tables in MERGE statements; commit derby-3155-13-aa-allowSystemAndTempTables.diff.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-14-aa-replaceCorrelationNamesOnLeftSideOfSETclauses.diff. This patch fixes the following problem which Knut found: correlation names on the left side of SET clauses weren't being replaced with their table names.

          The problem was that I was relying on the Visitor logic in ResultColumn to visit the left side of SET clauses as well as the right side. But apparently it doesn't.

          Touches the following files:

          M java/engine/org/apache/derby/impl/sql/compile/MatchingClauseNode.java
          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java

          Show
          Rick Hillegas added a comment - Attaching derby-3155-14-aa-replaceCorrelationNamesOnLeftSideOfSETclauses.diff. This patch fixes the following problem which Knut found: correlation names on the left side of SET clauses weren't being replaced with their table names. The problem was that I was relying on the Visitor logic in ResultColumn to visit the left side of SET clauses as well as the right side. But apparently it doesn't. Touches the following files: M java/engine/org/apache/derby/impl/sql/compile/MatchingClauseNode.java M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java
          Rick Hillegas made changes -
          Hide
          ASF subversion and git services added a comment -

          Commit 1559183 from Rick Hillegas in branch 'code/trunk'
          [ https://svn.apache.org/r1559183 ]

          DERBY-3155: Replace correlation names on the left side of the SET clauses in MERGE statements: commit derby-3155-14-aa-replaceCorrelationNamesOnLeftSideOfSETclauses.diff.

          Show
          ASF subversion and git services added a comment - Commit 1559183 from Rick Hillegas in branch 'code/trunk' [ https://svn.apache.org/r1559183 ] DERBY-3155 : Replace correlation names on the left side of the SET clauses in MERGE statements: commit derby-3155-14-aa-replaceCorrelationNamesOnLeftSideOfSETclauses.diff.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-15-aa-replumbMergeResultSetCleanup.diff. This patch fixes the following problem found by Knut: an assertion was raised under a row-scope trigger which fired a MERGE statement.

          I re-arranged the cleanup logic in MergeResultSet to more closely resemble the cleanup logic in InsertResultSet.

          Touches the following files:

          M java/engine/org/apache/derby/impl/sql/execute/MergeResultSet.java
          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java

          Show
          Rick Hillegas added a comment - Attaching derby-3155-15-aa-replumbMergeResultSetCleanup.diff. This patch fixes the following problem found by Knut: an assertion was raised under a row-scope trigger which fired a MERGE statement. I re-arranged the cleanup logic in MergeResultSet to more closely resemble the cleanup logic in InsertResultSet. Touches the following files: M java/engine/org/apache/derby/impl/sql/execute/MergeResultSet.java M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java
          Rick Hillegas made changes -
          Hide
          ASF subversion and git services added a comment -

          Commit 1559218 from Rick Hillegas in branch 'code/trunk'
          [ https://svn.apache.org/r1559218 ]

          DERBY-3155: Make cleanup logic in MergeResultSet more closely resemble the cleanup logic in InsertResultSet; commit derby-3155-15-aa-replumbMergeResultSetCleanup.diff.

          Show
          ASF subversion and git services added a comment - Commit 1559218 from Rick Hillegas in branch 'code/trunk' [ https://svn.apache.org/r1559218 ] DERBY-3155 : Make cleanup logic in MergeResultSet more closely resemble the cleanup logic in InsertResultSet; commit derby-3155-15-aa-replumbMergeResultSetCleanup.diff.
          Hide
          Knut Anders Hatlen added a comment -

          Here's another one to ponder on:

          ij version 10.11
          ij> connect 'jdbc:derby:memory:db;create=true';
          ij> create table t(x int, y int);
          0 rows inserted/updated/deleted
          ij> create table tv(x int, y int);
          0 rows inserted/updated/deleted
          ij> create view v as select * from tv;
          0 rows inserted/updated/deleted
          ij> merge into t using v on t.x=v.x when matched then update set t.y=v.y;
          ERROR XJ001: Java exception: 'ASSERT FAILED currentOfNode is null: org.apache.derby.shared.common.sanity.AssertFailure'.
          
          Show
          Knut Anders Hatlen added a comment - Here's another one to ponder on: ij version 10.11 ij> connect 'jdbc:derby:memory:db;create=true'; ij> create table t(x int, y int); 0 rows inserted/updated/deleted ij> create table tv(x int, y int); 0 rows inserted/updated/deleted ij> create view v as select * from tv; 0 rows inserted/updated/deleted ij> merge into t using v on t.x=v.x when matched then update set t.y=v.y; ERROR XJ001: Java exception: 'ASSERT FAILED currentOfNode is null: org.apache.derby.shared.common.sanity.AssertFailure'.
          Hide
          Knut Anders Hatlen added a comment -

          This may be intentional, but I thought I'd raise the issue so that we can clarify whether or not it's supposed to be supported. Merge with self doesn't seem to be accepted:

          ij> create table t(x int, y int);
          0 rows inserted/updated/deleted
          ij> merge into t a using t b on a.x=b.x when matched then update set a.y=b.y;
          ERROR 42X09: The table or alias name 'T' is used more than once in the FROM list.
          
          Show
          Knut Anders Hatlen added a comment - This may be intentional, but I thought I'd raise the issue so that we can clarify whether or not it's supposed to be supported. Merge with self doesn't seem to be accepted: ij> create table t(x int, y int); 0 rows inserted/updated/deleted ij> merge into t a using t b on a.x=b.x when matched then update set a.y=b.y; ERROR 42X09: The table or alias name 'T' is used more than once in the FROM list.
          Hide
          Rick Hillegas added a comment -

          Thanks for continuing to test-drive the MERGE statement. Yes, that last one is supposed to be forbidden, though I can see that the error message could be improved. The same table can't be used as both a source and target table. That's part of what's intended by the following bullet item in the functional spec under the heading "New MERGE Statement Syntax". This item could probably stand some clarification:

          "correlation names - The source table name (or its correlation name) may not be the same as the target table name (or its correlation name)."

          Thanks,
          -Rick

          Show
          Rick Hillegas added a comment - Thanks for continuing to test-drive the MERGE statement. Yes, that last one is supposed to be forbidden, though I can see that the error message could be improved. The same table can't be used as both a source and target table. That's part of what's intended by the following bullet item in the functional spec under the heading "New MERGE Statement Syntax". This item could probably stand some clarification: "correlation names - The source table name (or its correlation name) may not be the same as the target table name (or its correlation name)." Thanks, -Rick
          Hide
          Knut Anders Hatlen added a comment -

          Thanks for that clarification. My original understanding of that bullet was that the exposed name of the source table had to be different from the exposed name of the target table.

          The assert failure that I posted just before the question about self merge, was something I originally came across while trying to work around the limitation. For example, this would allow you to do a self merge, but currently fails just like the repro above:

          ij> create table t(x int, y int);
          0 rows inserted/updated/deleted
          ij> create view v as select * from t;
          0 rows inserted/updated/deleted
          ij> merge into t using v on t.x=v.x when matched then update set t.y=v.y;
          ERROR XJ001: Java exception: 'ASSERT FAILED currentOfNode is null: org.apache.derby.shared.common.sanity.AssertFailure'.
          
          Show
          Knut Anders Hatlen added a comment - Thanks for that clarification. My original understanding of that bullet was that the exposed name of the source table had to be different from the exposed name of the target table. The assert failure that I posted just before the question about self merge, was something I originally came across while trying to work around the limitation. For example, this would allow you to do a self merge, but currently fails just like the repro above: ij> create table t(x int, y int); 0 rows inserted/updated/deleted ij> create view v as select * from t; 0 rows inserted/updated/deleted ij> merge into t using v on t.x=v.x when matched then update set t.y=v.y; ERROR XJ001: Java exception: 'ASSERT FAILED currentOfNode is null: org.apache.derby.shared.common.sanity.AssertFailure'.
          Hide
          Rick Hillegas added a comment -

          This patch addresses the following problem case:

          ij> create table t1(x int primary key);
          0 rows inserted/updated/deleted
          ij> create table t2(x int);
          0 rows inserted/updated/deleted
          ij> merge into t1 using t2 on t1.x = 42 when not matched then insert (x) values (42);
          ERROR XSAI2: The conglomerate (0) requested does not exist.
          

          This was Knut's simplification of a problem case which Dyre identified. Knut's query is fixed and Dyre's original query is converted into a later problem which Knut recorded:

          ERROR XJ001: Java exception: 'ASSERT FAILED currentOfNode is null: org.apache.derby.shared.common.sanity.AssertFailure'.

          At a high level, this is what's going on with the corrected failure:

          1) The MERGE statement cooks up a LEFT JOIN to drive its execution phase.

          2) This is a LEFT JOIN which ordinary users can't create. That's because the row location from the target table is added to the SELECT list of the LEFT JOIN.

          3) The added row location column causes the LEFT JOIN to violate some assumptions made by the compiler.

          Touches the following files:

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

          M java/engine/org/apache/derby/impl/sql/compile/FromBaseTable.java

          Adjusted a case where a FromBaseTable is cloning its result columns without propagating its heap conglomerate id.

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

          M java/engine/org/apache/derby/impl/sql/compile/ResultColumnList.java

          Fixed the construction of the referenced bit map. The old logic did not account for the fact that a row location column can be one of the returned columns.

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

          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java

          Added a test case for this problem query.

          Show
          Rick Hillegas added a comment - This patch addresses the following problem case: ij> create table t1(x int primary key); 0 rows inserted/updated/deleted ij> create table t2(x int); 0 rows inserted/updated/deleted ij> merge into t1 using t2 on t1.x = 42 when not matched then insert (x) values (42); ERROR XSAI2: The conglomerate (0) requested does not exist. This was Knut's simplification of a problem case which Dyre identified. Knut's query is fixed and Dyre's original query is converted into a later problem which Knut recorded: ERROR XJ001: Java exception: 'ASSERT FAILED currentOfNode is null: org.apache.derby.shared.common.sanity.AssertFailure'. At a high level, this is what's going on with the corrected failure: 1) The MERGE statement cooks up a LEFT JOIN to drive its execution phase. 2) This is a LEFT JOIN which ordinary users can't create. That's because the row location from the target table is added to the SELECT list of the LEFT JOIN. 3) The added row location column causes the LEFT JOIN to violate some assumptions made by the compiler. Touches the following files: -------------------- M java/engine/org/apache/derby/impl/sql/compile/FromBaseTable.java Adjusted a case where a FromBaseTable is cloning its result columns without propagating its heap conglomerate id. -------------------- M java/engine/org/apache/derby/impl/sql/compile/ResultColumnList.java Fixed the construction of the referenced bit map. The old logic did not account for the fact that a row location column can be one of the returned columns. -------------------- M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java Added a test case for this problem query.
          Rick Hillegas made changes -
          Hide
          Rick Hillegas added a comment -

          Here is a variation on the query Knut found which raises a de-serialization error when trying to concatenate a null with a string. This variation eliminates the subquery in the ON clause:

          connect 'jdbc:derby:memory:db;create=true';

          create table t1(x int, y varchar(100));
          create table t2(x int);
          insert into t2 values 1, 1, 2;
          insert into t1 values (1, null), (2, '');

          – this is ok
          select y || 'x'
          from t2 left join t1 on true;

          – this fails
          merge into t1
          using t2 on true
          when matched then update set y = y || 'x';

          delete from t1 where y is null;

          – without the nulls, this succeeds
          merge into t1
          using t2 on true
          when matched then update set y = y || 'x';

          Show
          Rick Hillegas added a comment - Here is a variation on the query Knut found which raises a de-serialization error when trying to concatenate a null with a string. This variation eliminates the subquery in the ON clause: connect 'jdbc:derby:memory:db;create=true'; create table t1(x int, y varchar(100)); create table t2(x int); insert into t2 values 1, 1, 2; insert into t1 values (1, null), (2, ''); – this is ok select y || 'x' from t2 left join t1 on true; – this fails merge into t1 using t2 on true when matched then update set y = y || 'x'; delete from t1 where y is null; – without the nulls, this succeeds merge into t1 using t2 on true when matched then update set y = y || 'x';
          Hide
          ASF subversion and git services added a comment -

          Commit 1560134 from Rick Hillegas in branch 'code/trunk'
          [ https://svn.apache.org/r1560134 ]

          DERBY-3155: Allow for row location columns in the result column list of base tables; tests passed cleanly for me on derby-3155-16-aa-treatCurrentRowLocationNodeLikeBaseColumnNode.diff.

          Show
          ASF subversion and git services added a comment - Commit 1560134 from Rick Hillegas in branch 'code/trunk' [ https://svn.apache.org/r1560134 ] DERBY-3155 : Allow for row location columns in the result column list of base tables; tests passed cleanly for me on derby-3155-16-aa-treatCurrentRowLocationNodeLikeBaseColumnNode.diff.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-17-aa-serializingRowLocations.diff. This patch fixes a deserialization problem found by Knut.

          The following script now runs cleanly:

          connect 'jdbc:derby:memory:db;create=true';
          
          create table t1_030(x int, y varchar(100));
          create table t2_030(x int);
          insert into t2_030 values 1, 1, 2;
          insert into t1_030 values (1, null), (2, '');
          
          -- this fails
          merge into t1_030
          using t2_030 on true
          when matched then update set y = y || 'x';
          
          select * from t1_030 order by x, y;
          
          merge into t1_030
          using t2_030 on exists (select * from t2_030)
          when matched then update set y = y || 'x';
          
          select * from t1_030 order by x, y;
          

          Touches the following files:

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

          M java/engine/org/apache/derby/impl/sql/execute/MergeResultSet.java

          Convert HeapRowLocations to SQLRefs. This causes us to store SQLRefs in the temporary tables rather than HeapRowLocations. That makes the contents of the temp tables conform to their metadata and fixes the deserialization problem.

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

          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java

          Add a test case.

          Show
          Rick Hillegas added a comment - Attaching derby-3155-17-aa-serializingRowLocations.diff. This patch fixes a deserialization problem found by Knut. The following script now runs cleanly: connect 'jdbc:derby:memory:db;create=true'; create table t1_030(x int, y varchar(100)); create table t2_030(x int); insert into t2_030 values 1, 1, 2; insert into t1_030 values (1, null), (2, ''); -- this fails merge into t1_030 using t2_030 on true when matched then update set y = y || 'x'; select * from t1_030 order by x, y; merge into t1_030 using t2_030 on exists (select * from t2_030) when matched then update set y = y || 'x'; select * from t1_030 order by x, y; Touches the following files: ------------- M java/engine/org/apache/derby/impl/sql/execute/MergeResultSet.java Convert HeapRowLocations to SQLRefs. This causes us to store SQLRefs in the temporary tables rather than HeapRowLocations. That makes the contents of the temp tables conform to their metadata and fixes the deserialization problem. ------------- M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java Add a test case.
          Rick Hillegas made changes -
          Hide
          ASF subversion and git services added a comment -

          Commit 1560452 from Rick Hillegas in branch 'code/trunk'
          [ https://svn.apache.org/r1560452 ]

          DERBY-3155: Make MERGE statement serialize SQLRefs rather than HeapRowLocations; commit derby-3155-17-aa-serializingRowLocations.diff.

          Show
          ASF subversion and git services added a comment - Commit 1560452 from Rick Hillegas in branch 'code/trunk' [ https://svn.apache.org/r1560452 ] DERBY-3155 : Make MERGE statement serialize SQLRefs rather than HeapRowLocations; commit derby-3155-17-aa-serializingRowLocations.diff.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-18-aa-basicView.diff. This patch fixes a problem Knut found when using a simple view as the source table of a MERGE statement.

          The problem arose because views can be turned into FromSubqueries before an UPDATE action is bound. This causes the UpdateNode's bind logic to take an unexpected path. The fix was to make UpdateNode.bindStatement() not fall into that path.

          Touches the following files:

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

          M java/engine/org/apache/derby/impl/sql/compile/UpdateNode.java

          The fix.

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

          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java

          A regression test.

          Show
          Rick Hillegas added a comment - Attaching derby-3155-18-aa-basicView.diff. This patch fixes a problem Knut found when using a simple view as the source table of a MERGE statement. The problem arose because views can be turned into FromSubqueries before an UPDATE action is bound. This causes the UpdateNode's bind logic to take an unexpected path. The fix was to make UpdateNode.bindStatement() not fall into that path. Touches the following files: ----------------- M java/engine/org/apache/derby/impl/sql/compile/UpdateNode.java The fix. ----------------- M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java A regression test.
          Rick Hillegas made changes -
          Attachment derby-3155-18-aa-basicView.diff [ 12624427 ]
          Hide
          ASF subversion and git services added a comment -

          Commit 1560507 from Rick Hillegas in branch 'code/trunk'
          [ https://svn.apache.org/r1560507 ]

          DERBY-3155: Basic support for views as the source tables of MERGE statements; commit derby-3155-18-aa-basicView.diff.

          Show
          ASF subversion and git services added a comment - Commit 1560507 from Rick Hillegas in branch 'code/trunk' [ https://svn.apache.org/r1560507 ] DERBY-3155 : Basic support for views as the source tables of MERGE statements; commit derby-3155-18-aa-basicView.diff.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-19-aa-forbidSubqueriesInMatchedClauses.diff. This patch forbids subqueries in the WHEN [ NOT ] MATCHED clauses of MERGE statements. I am running tests now.

          The SQL Standard allows subqueries in the WHEN [ NOT ] MATCHED clauses of MERGE statements. However, these subqueries break code-generation. That's because we don't completely optimize the dummy INSERT/UPDATE/DELETE statements which we cook up in order to compile the WHEN [ NOT ] MATCHED clauses. If we improve the MERGE statement so that the dummy statements are completely optimized, then we can consider allowing subqueries in these clauses.

          I will record this limitation in the functional spec and I will create an enhancement request for lifting this limitation later on.

          Touches the following files:

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

          M java/engine/org/apache/derby/impl/sql/compile/MatchingClauseNode.java

          The code to forbid these subqueries.

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

          M java/engine/org/apache/derby/loc/messages.xml
          M java/shared/org/apache/derby/shared/common/reference/SQLState.java

          A new error message for this limitation.

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

          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java

          New test cases.

          Show
          Rick Hillegas added a comment - Attaching derby-3155-19-aa-forbidSubqueriesInMatchedClauses.diff. This patch forbids subqueries in the WHEN [ NOT ] MATCHED clauses of MERGE statements. I am running tests now. The SQL Standard allows subqueries in the WHEN [ NOT ] MATCHED clauses of MERGE statements. However, these subqueries break code-generation. That's because we don't completely optimize the dummy INSERT/UPDATE/DELETE statements which we cook up in order to compile the WHEN [ NOT ] MATCHED clauses. If we improve the MERGE statement so that the dummy statements are completely optimized, then we can consider allowing subqueries in these clauses. I will record this limitation in the functional spec and I will create an enhancement request for lifting this limitation later on. Touches the following files: -------------- M java/engine/org/apache/derby/impl/sql/compile/MatchingClauseNode.java The code to forbid these subqueries. -------------- M java/engine/org/apache/derby/loc/messages.xml M java/shared/org/apache/derby/shared/common/reference/SQLState.java A new error message for this limitation. -------------- M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java New test cases.
          Rick Hillegas made changes -
          Rick Hillegas made changes -
          Link This issue is related to DERBY-6460 [ DERBY-6460 ]
          Hide
          Rick Hillegas added a comment -

          Tests passed cleanly for me on derby-3155-19-aa-forbidSubqueriesInMatchedClauses.diff except for the recently introduced instabilities in org.apache.derbyTesting.functionTests.tests.lang.SelectivityTest.

          Show
          Rick Hillegas added a comment - Tests passed cleanly for me on derby-3155-19-aa-forbidSubqueriesInMatchedClauses.diff except for the recently introduced instabilities in org.apache.derbyTesting.functionTests.tests.lang.SelectivityTest.
          Hide
          ASF subversion and git services added a comment -

          Commit 1561671 from Rick Hillegas in branch 'code/trunk'
          [ https://svn.apache.org/r1561671 ]

          DERBY-3155: Forbid subqueries in the WHEN [ NOT ] MATCHED clauses of MERGE statements; commit derby-3155-19-aa-forbidSubqueriesInMatchedClauses.diff.

          Show
          ASF subversion and git services added a comment - Commit 1561671 from Rick Hillegas in branch 'code/trunk' [ https://svn.apache.org/r1561671 ] DERBY-3155 : Forbid subqueries in the WHEN [ NOT ] MATCHED clauses of MERGE statements; commit derby-3155-19-aa-forbidSubqueriesInMatchedClauses.diff.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-20-aa-reworkColumnMatching.diff. This patch substantially reworks how column references in WHEN [ NOT ] MATCHED clauses are matched to the correct values in the rows produced by the left join which drives the MERGE statement's execution. Tests passed cleanly for me.

          Consider the following MERGE statement:

              merge into t1_027 x
              using t2_027 y on x.a = y.c
              when matched and x.b > 100 then update set x.b = y.d
              when matched and x.b <= 100 then delete
              when not matched and y.d > 3000 then insert values ( y.c, y.d );
          

          At execution time, column references in all of the following expressions must be mapped to the rows coming back from the driving left join:

              x.b > 100
              x.b <= 100
              y.d > 3000
              set x.b = y.d
              values ( y.c, y.d )
          

          Before this patch, that mapping was accomplished through FromTable.getMatchingColumn(). In order to smooth over differences in the column matching for SELECTs and UPDATEs, correlation names were replaced with fully qualified table names in column references. But that was crude and caused many simple use cases to fail.

          The new approach is to do the following:

          1) Before compiling the INSERT/UPDATE/DELETE actions, all of the column references in the WHEN [ NOT ] MATCHED clauses are marked with whether they come from the source or the target table.

          2) The columns in the SELECT list of the driving left join are similarly marked.

          3) At code generation time, these markers are used to match the WHEN [ NOT ] MATCHED clauses to the SELECT list.

          I think that the new approach will handle more use cases.

          Touches the following files:

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

          M java/engine/org/apache/derby/impl/sql/compile/MergeNode.java
          M java/engine/org/apache/derby/impl/sql/compile/MatchingClauseNode.java
          M java/engine/org/apache/derby/impl/sql/compile/ColumnReference.java

          New machinery for linking ColumnReferences to the source or target table of a MERGE statement.

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

          M java/engine/org/apache/derby/impl/sql/compile/FromTable.java
          M java/engine/org/apache/derby/impl/sql/compile/ResultColumn.java

          Special processing for these linked ColumnReferences to that we don't blur the distinction between correlation names and fully qualified names.

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

          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java

          A new test which succeeds with the new scheme but failed with the old scheme.

          Show
          Rick Hillegas added a comment - Attaching derby-3155-20-aa-reworkColumnMatching.diff. This patch substantially reworks how column references in WHEN [ NOT ] MATCHED clauses are matched to the correct values in the rows produced by the left join which drives the MERGE statement's execution. Tests passed cleanly for me. Consider the following MERGE statement: merge into t1_027 x using t2_027 y on x.a = y.c when matched and x.b > 100 then update set x.b = y.d when matched and x.b <= 100 then delete when not matched and y.d > 3000 then insert values ( y.c, y.d ); At execution time, column references in all of the following expressions must be mapped to the rows coming back from the driving left join: x.b > 100 x.b <= 100 y.d > 3000 set x.b = y.d values ( y.c, y.d ) Before this patch, that mapping was accomplished through FromTable.getMatchingColumn(). In order to smooth over differences in the column matching for SELECTs and UPDATEs, correlation names were replaced with fully qualified table names in column references. But that was crude and caused many simple use cases to fail. The new approach is to do the following: 1) Before compiling the INSERT/UPDATE/DELETE actions, all of the column references in the WHEN [ NOT ] MATCHED clauses are marked with whether they come from the source or the target table. 2) The columns in the SELECT list of the driving left join are similarly marked. 3) At code generation time, these markers are used to match the WHEN [ NOT ] MATCHED clauses to the SELECT list. I think that the new approach will handle more use cases. Touches the following files: ----------------- M java/engine/org/apache/derby/impl/sql/compile/MergeNode.java M java/engine/org/apache/derby/impl/sql/compile/MatchingClauseNode.java M java/engine/org/apache/derby/impl/sql/compile/ColumnReference.java New machinery for linking ColumnReferences to the source or target table of a MERGE statement. ----------------- M java/engine/org/apache/derby/impl/sql/compile/FromTable.java M java/engine/org/apache/derby/impl/sql/compile/ResultColumn.java Special processing for these linked ColumnReferences to that we don't blur the distinction between correlation names and fully qualified names. ----------------- M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java A new test which succeeds with the new scheme but failed with the old scheme.
          Rick Hillegas made changes -
          Hide
          ASF subversion and git services added a comment -

          Commit 1564874 from Rick Hillegas in branch 'code/trunk'
          [ https://svn.apache.org/r1564874 ]

          DERBY-3155: Revamp how columns referenced by WHEN [ NOT ] MATCHED clauses are linked to the columns coming back from the driving left join of a MERGE statement; commit derby-3155-20-aa-reworkColumnMatching.diff.

          Show
          ASF subversion and git services added a comment - Commit 1564874 from Rick Hillegas in branch 'code/trunk' [ https://svn.apache.org/r1564874 ] DERBY-3155 : Revamp how columns referenced by WHEN [ NOT ] MATCHED clauses are linked to the columns coming back from the driving left join of a MERGE statement; commit derby-3155-20-aa-reworkColumnMatching.diff.
          Hide
          Knut Anders Hatlen added a comment -

          The handling of parameters seems to have some issues:

          ij> create table t1(x int);
          0 rows inserted/updated/deleted
          ij> create table t2(x int);
          0 rows inserted/updated/deleted
          ij> prepare ps as 'merge into t1 using t2 on ? when not matched then insert values (1)';
          ij> execute ps;
          0 rows inserted/updated/deleted
          WARNING 02000: No row was found for FETCH, UPDATE or DELETE; or the result of a query is an empty table.
          ij> execute ps using 'values true';
          IJ WARNING: Autocommit may close using result set
          ERROR 07009: No input parameters.
          

          I'd expect the first execution to fail because the parameter was not bound, but it succeeded. In the second execution, a parameter value was provided, but that one did fail saying there were no parameters.

          Show
          Knut Anders Hatlen added a comment - The handling of parameters seems to have some issues: ij> create table t1(x int); 0 rows inserted/updated/deleted ij> create table t2(x int); 0 rows inserted/updated/deleted ij> prepare ps as 'merge into t1 using t2 on ? when not matched then insert values (1)'; ij> execute ps; 0 rows inserted/updated/deleted WARNING 02000: No row was found for FETCH, UPDATE or DELETE; or the result of a query is an empty table. ij> execute ps using 'values true'; IJ WARNING: Autocommit may close using result set ERROR 07009: No input parameters. I'd expect the first execution to fail because the parameter was not bound, but it succeeded. In the second execution, a parameter value was provided, but that one did fail saying there were no parameters.
          Hide
          Knut Anders Hatlen added a comment -

          The syntax description in the functional specification says that the source table and the target table can have a correlation name. It doesn't seem to allow column lists to go with the correlation names (since it says correlationName, not correlationClause). The actually implemented syntax seems to accept column lists, though, and the column names can be used in the ON clause:

          ij> create table t(x int);
          0 rows inserted/updated/deleted
          ij> merge into t t1(a) using t t2(b) on a=b when not matched then insert values (1);
          0 rows inserted/updated/deleted
          WARNING 02000: No row was found for FETCH, UPDATE or DELETE; or the result of a query is an empty table.
          

          However, the specified column name cannot be used in the mergeInsert clause:

          ij> merge into t t1(a) using t t2(b) on a=b when not matched then insert values (b);
          ERROR 42X04: Column 'B' is either not in any table in the FROM list or appears within a join specification and is outside the scope of the join specification or appears in a HAVING clause and is not in the GROUP BY list. If this is a CREATE or ALTER TABLE  statement then 'B' is not a column in the target table.
          

          If I read the SQL standard correctly, it doesn't allow a column list for the target table. It does seem to allow a column list for the source table, though.

          Show
          Knut Anders Hatlen added a comment - The syntax description in the functional specification says that the source table and the target table can have a correlation name. It doesn't seem to allow column lists to go with the correlation names (since it says correlationName, not correlationClause). The actually implemented syntax seems to accept column lists, though, and the column names can be used in the ON clause: ij> create table t(x int); 0 rows inserted/updated/deleted ij> merge into t t1(a) using t t2(b) on a=b when not matched then insert values (1); 0 rows inserted/updated/deleted WARNING 02000: No row was found for FETCH, UPDATE or DELETE; or the result of a query is an empty table. However, the specified column name cannot be used in the mergeInsert clause: ij> merge into t t1(a) using t t2(b) on a=b when not matched then insert values (b); ERROR 42X04: Column 'B' is either not in any table in the FROM list or appears within a join specification and is outside the scope of the join specification or appears in a HAVING clause and is not in the GROUP BY list. If this is a CREATE or ALTER TABLE statement then 'B' is not a column in the target table. If I read the SQL standard correctly, it doesn't allow a column list for the target table. It does seem to allow a column list for the source table, though.
          Hide
          Knut Anders Hatlen added a comment - - edited

          Whoops...

          ij> create view sr(i) as values 1;
          0 rows inserted/updated/deleted
          ij> create table t(x int, y int, z int);
          0 rows inserted/updated/deleted
          ij> create unique index idx on t(x, y);
          0 rows inserted/updated/deleted
          ij> prepare ps as 'merge into t using sr on (x = ? and y = ?) when matched then update set y = ?';
          ERROR XSAI2: The conglomerate (0) requested does not exist.
          

          Also fails with constants instead of parameters:

          ij> merge into t using sr on (x = 1 and y = 2) when matched then update set y = 3;
          ERROR XSAI2: The conglomerate (0) requested does not exist.
          

          This looks similar to the problem fixed by the 16-aa patch.

          Show
          Knut Anders Hatlen added a comment - - edited Whoops... ij> create view sr(i) as values 1; 0 rows inserted/updated/deleted ij> create table t(x int, y int, z int); 0 rows inserted/updated/deleted ij> create unique index idx on t(x, y); 0 rows inserted/updated/deleted ij> prepare ps as 'merge into t using sr on (x = ? and y = ?) when matched then update set y = ?'; ERROR XSAI2: The conglomerate (0) requested does not exist. Also fails with constants instead of parameters: ij> merge into t using sr on (x = 1 and y = 2) when matched then update set y = 3; ERROR XSAI2: The conglomerate (0) requested does not exist. This looks similar to the problem fixed by the 16-aa patch.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-21-ac-cleanupAndForbidSynonyms.diff. This patch cleans up some compile-time processing for MERGE nodes to that it is easier to convince oneself that the binding of one action does not interfere with the binding of another action. In addition, this patch forbids the use of synonyms in MERGE statements I am running tests now.

          Synonyms are not a SQL Standard feature. I spent some time trying to fix some tough problem cases involving synonyms with correlation names. But I found myself going in circles. I am not able to resolve aliases on aliases. If someone wants to add synonym support to MERGE statements, they are welcome to try their hand at it.

          Touches the following files:

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

          M java/engine/org/apache/derby/loc/messages.xml
          M java/shared/org/apache/derby/shared/common/reference/SQLState.java

          New message for forbidding synonyms in MERGE statements.

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

          M java/engine/org/apache/derby/impl/sql/compile/MergeNode.java
          M java/engine/org/apache/derby/impl/sql/compile/TableName.java
          M java/engine/org/apache/derby/impl/sql/compile/MatchingClauseNode.java
          M java/engine/org/apache/derby/impl/sql/compile/ColumnReference.java

          Cleanup and changes to forbid synonyms.

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

          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java

          Test to verify that synonyms are forbidden.

          Show
          Rick Hillegas added a comment - Attaching derby-3155-21-ac-cleanupAndForbidSynonyms.diff. This patch cleans up some compile-time processing for MERGE nodes to that it is easier to convince oneself that the binding of one action does not interfere with the binding of another action. In addition, this patch forbids the use of synonyms in MERGE statements I am running tests now. Synonyms are not a SQL Standard feature. I spent some time trying to fix some tough problem cases involving synonyms with correlation names. But I found myself going in circles. I am not able to resolve aliases on aliases. If someone wants to add synonym support to MERGE statements, they are welcome to try their hand at it. Touches the following files: --------------- M java/engine/org/apache/derby/loc/messages.xml M java/shared/org/apache/derby/shared/common/reference/SQLState.java New message for forbidding synonyms in MERGE statements. --------------- M java/engine/org/apache/derby/impl/sql/compile/MergeNode.java M java/engine/org/apache/derby/impl/sql/compile/TableName.java M java/engine/org/apache/derby/impl/sql/compile/MatchingClauseNode.java M java/engine/org/apache/derby/impl/sql/compile/ColumnReference.java Cleanup and changes to forbid synonyms. --------------- M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java Test to verify that synonyms are forbidden.
          Rick Hillegas made changes -
          Hide
          ASF subversion and git services added a comment -

          Commit 1565830 from Rick Hillegas in branch 'code/trunk'
          [ https://svn.apache.org/r1565830 ]

          DERBY-3155: Forbid synonyms in MERGE statement; tests passed cleanly for me on derby-3155-21-ac-cleanupAndForbidSynonyms.diff.

          Show
          ASF subversion and git services added a comment - Commit 1565830 from Rick Hillegas in branch 'code/trunk' [ https://svn.apache.org/r1565830 ] DERBY-3155 : Forbid synonyms in MERGE statement; tests passed cleanly for me on derby-3155-21-ac-cleanupAndForbidSynonyms.diff.
          Hide
          Dag H. Wanvik added a comment -

          Maybe you could file an improvement JIRA for allowing synonyms (and possible add the issues you saw trying to implement it), so we could have that as a starting point if someone wants to do it?

          Show
          Dag H. Wanvik added a comment - Maybe you could file an improvement JIRA for allowing synonyms (and possible add the issues you saw trying to implement it), so we could have that as a starting point if someone wants to do it?
          Rick Hillegas made changes -
          Link This issue is related to DERBY-6474 [ DERBY-6474 ]
          Hide
          Rick Hillegas added a comment -

          Good idea, Dag. I've logged DERBY-6474 in case someone wants to add support for synonyms.

          Show
          Rick Hillegas added a comment - Good idea, Dag. I've logged DERBY-6474 in case someone wants to add support for synonyms.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-22-ad-testIdentifiersOnLeftSideOfSetClauses.diff. This patch adds a test to track the fact that identifiers can be included or omitted on the left side of SET clauses with and without using aliases.

          Touches the following file:

          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java

          Show
          Rick Hillegas added a comment - Attaching derby-3155-22-ad-testIdentifiersOnLeftSideOfSetClauses.diff. This patch adds a test to track the fact that identifiers can be included or omitted on the left side of SET clauses with and without using aliases. Touches the following file: M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java
          Rick Hillegas made changes -
          Hide
          ASF subversion and git services added a comment -

          Commit 1566625 from Rick Hillegas in branch 'code/trunk'
          [ https://svn.apache.org/r1566625 ]

          DERBY-3155: Add test to verify use of identifiers on the left side of SET clauses in MERGE statements; commit derby-3155-22-ad-testIdentifiersOnLeftSideOfSetClauses.diff.

          Show
          ASF subversion and git services added a comment - Commit 1566625 from Rick Hillegas in branch 'code/trunk' [ https://svn.apache.org/r1566625 ] DERBY-3155 : Add test to verify use of identifiers on the left side of SET clauses in MERGE statements; commit derby-3155-22-ad-testIdentifiersOnLeftSideOfSetClauses.diff.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-23-aa-forbidDerivedColumnLists.diff. This patch forbids the use of derived column lists in MERGE statements.

          In a continuing attempt to contain the name resolution problems with MERGE statements, this patch forbids queries like the following:

          merge into t1_036 r( x )
          using t2_036 on r.x = t2_036.a
          when matched then delete;
          
          merge into t1_036
          using t2_036 r( x ) on t1_036.a = r.x
          when matched then delete;
          

          Knut identified some issues with derived column lists in a 2014-02-07 comment. I will log an issue for allowing derived column lists in MERGE statements later on.

          Touches the following files:

          M java/engine/org/apache/derby/impl/sql/compile/MergeNode.java
          M java/engine/org/apache/derby/loc/messages.xml
          M java/shared/org/apache/derby/shared/common/reference/SQLState.java
          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java

          Show
          Rick Hillegas added a comment - Attaching derby-3155-23-aa-forbidDerivedColumnLists.diff. This patch forbids the use of derived column lists in MERGE statements. In a continuing attempt to contain the name resolution problems with MERGE statements, this patch forbids queries like the following: merge into t1_036 r( x ) using t2_036 on r.x = t2_036.a when matched then delete; merge into t1_036 using t2_036 r( x ) on t1_036.a = r.x when matched then delete; Knut identified some issues with derived column lists in a 2014-02-07 comment. I will log an issue for allowing derived column lists in MERGE statements later on. Touches the following files: M java/engine/org/apache/derby/impl/sql/compile/MergeNode.java M java/engine/org/apache/derby/loc/messages.xml M java/shared/org/apache/derby/shared/common/reference/SQLState.java M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java
          Rick Hillegas made changes -
          Hide
          ASF subversion and git services added a comment -

          Commit 1566649 from Rick Hillegas in branch 'code/trunk'
          [ https://svn.apache.org/r1566649 ]

          DERBY-3155: Forbid derived column lists in MERGE statements; commit derby-3155-23-aa-forbidDerivedColumnLists.diff.

          Show
          ASF subversion and git services added a comment - Commit 1566649 from Rick Hillegas in branch 'code/trunk' [ https://svn.apache.org/r1566649 ] DERBY-3155 : Forbid derived column lists in MERGE statements; commit derby-3155-23-aa-forbidDerivedColumnLists.diff.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-24-aa-supportParameters.diff. This patch adds support for ? parameters in MERGE statements.

          This patch fixes a problem Knut found on 2014-02-07: The state of ? parameters was not being checked at runtime. The problem was that every statement which supports ? parameters needs its own call to setUpAndLinkParameters() in its grammar production. I don't know why that call isn't factored higher in the grammar. I added a call to setUpAndLinkParameters() to the MERGE statement's grammar production.

          Touches the following files:

          M java/engine/org/apache/derby/impl/sql/compile/sqlgrammar.jj
          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java

          Show
          Rick Hillegas added a comment - Attaching derby-3155-24-aa-supportParameters.diff. This patch adds support for ? parameters in MERGE statements. This patch fixes a problem Knut found on 2014-02-07: The state of ? parameters was not being checked at runtime. The problem was that every statement which supports ? parameters needs its own call to setUpAndLinkParameters() in its grammar production. I don't know why that call isn't factored higher in the grammar. I added a call to setUpAndLinkParameters() to the MERGE statement's grammar production. Touches the following files: M java/engine/org/apache/derby/impl/sql/compile/sqlgrammar.jj M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java
          Rick Hillegas made changes -
          Attachment derby-3155-24-aa-supportParameters.diff [ 12628000 ]
          Hide
          ASF subversion and git services added a comment -

          Commit 1566673 from Rick Hillegas in branch 'code/trunk'
          [ https://svn.apache.org/r1566673 ]

          DERBY-3155: Add support for ? parameters to MERGE statements; commit derby-3155-24-aa-supportParameters.diff.

          Show
          ASF subversion and git services added a comment - Commit 1566673 from Rick Hillegas in branch 'code/trunk' [ https://svn.apache.org/r1566673 ] DERBY-3155 : Add support for ? parameters to MERGE statements; commit derby-3155-24-aa-supportParameters.diff.
          Rick Hillegas made changes -
          Link This issue is related to DERBY-6476 [ DERBY-6476 ]
          Hide
          Knut Anders Hatlen added a comment -

          Thanks for adding support for ? parameters, Rick. It doesn't look like the parameter values are always seen by the MERGE statement, though:

          ij> create table t1(x int);
          0 rows inserted/updated/deleted
          ij> create table t2(x int);
          0 rows inserted/updated/deleted
          ij> insert into t2 values 1;
          1 row inserted/updated/deleted
          ij> execute 'merge into t1 using t2 on t1.x=t2.x when not matched then insert values (?)' using 'values 1';
          IJ WARNING: Autocommit may close using result set
          1 row inserted/updated/deleted
          ij> select * from t1;
          X          
          -----------
          NULL       
          
          1 row selected
          

          I had expected the value 1 rather than NULL to be inserted into T1 here.

          Show
          Knut Anders Hatlen added a comment - Thanks for adding support for ? parameters, Rick. It doesn't look like the parameter values are always seen by the MERGE statement, though: ij> create table t1(x int); 0 rows inserted/updated/deleted ij> create table t2(x int); 0 rows inserted/updated/deleted ij> insert into t2 values 1; 1 row inserted/updated/deleted ij> execute 'merge into t1 using t2 on t1.x=t2.x when not matched then insert values (?)' using 'values 1'; IJ WARNING: Autocommit may close using result set 1 row inserted/updated/deleted ij> select * from t1; X ----------- NULL 1 row selected I had expected the value 1 rather than NULL to be inserted into T1 here.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-25-aa-parametersAsInsertValues.diff. This patch fixes a bug discovered by Knut: ? parameters were not being substituted when used as values in INSERT lists.

          The problem was that the INSERT values were being transmogrified when we compiled the dummy SELECT which drives the INSERT action. The fix was to clone the INSERT values so that the SELECT has its own copy.

          I added a simple test showing correct use of ? values in all search conditions, as INSERT values, and as UPDATE values. The following statement runs correctly in the test case:

            merge into t1_038
            using t2_038 on t2_038.x = t1_038.x * ?
            when not matched and t2_038.y = ? then insert values (  t2_038.x, t2_038.y, t2_038.z * ? )
            when matched and t2_038.y = ? then delete
            when matched and t2_038.y = ? then update set z = t2_038.z * ?
          

          Touches the following files:

          M java/engine/org/apache/derby/impl/sql/compile/MatchingClauseNode.java
          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java

          Show
          Rick Hillegas added a comment - Attaching derby-3155-25-aa-parametersAsInsertValues.diff. This patch fixes a bug discovered by Knut: ? parameters were not being substituted when used as values in INSERT lists. The problem was that the INSERT values were being transmogrified when we compiled the dummy SELECT which drives the INSERT action. The fix was to clone the INSERT values so that the SELECT has its own copy. I added a simple test showing correct use of ? values in all search conditions, as INSERT values, and as UPDATE values. The following statement runs correctly in the test case: merge into t1_038 using t2_038 on t2_038.x = t1_038.x * ? when not matched and t2_038.y = ? then insert values ( t2_038.x, t2_038.y, t2_038.z * ? ) when matched and t2_038.y = ? then delete when matched and t2_038.y = ? then update set z = t2_038.z * ? Touches the following files: M java/engine/org/apache/derby/impl/sql/compile/MatchingClauseNode.java M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java
          Rick Hillegas made changes -
          Hide
          ASF subversion and git services added a comment -

          Commit 1567368 from Rick Hillegas in branch 'code/trunk'
          [ https://svn.apache.org/r1567368 ]

          DERBY-3155: Add support for ? parameters as INSERT values in MERGE statements; commit derby-3155-25-aa-parametersAsInsertValues.diff.

          Show
          ASF subversion and git services added a comment - Commit 1567368 from Rick Hillegas in branch 'code/trunk' [ https://svn.apache.org/r1567368 ] DERBY-3155 : Add support for ? parameters as INSERT values in MERGE statements; commit derby-3155-25-aa-parametersAsInsertValues.diff.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-26-aa-copyRowLocationForIndexScans.diff. This patch fixes a compilation problem with a MERGE statement involving a view. Regression tests ran cleanly for me on this patch.

          The problem query, found by Knut, was this:

          create view sr_039( i ) as values 1;
          create table t1_039( x int, y int, z int );
          create unique index idx on t1_039( x, y );
          insert into t1_039 values ( 1, 100, 1000 ), ( 2, 200, 2000 );
          
          merge into t1_039
          using sr_039 on ( x = 1 )
          when matched then delete;
          

          The fix involves two changes:

          1) There was a place in FromBaseTable where a ResultColumnList was being cloned but conglomerate information wasn't being propagated from the original list. That just looks like an existing bug to me. However, I don't now why we haven't tripped over it before.

          2) Once I cleaned that up, I stumbled on another bug: Index probes into base tables weren't equipped to hand back the RowLocations needed by the MERGE statement's driving left join.

          After fixing these bugs, Knut's original test case works. But I'm not declaring victory yet. I have tried a variation on that original test case and tripped across yet another bug. I think that the fix for that bug deserves its own patch so that we can clearly reconstruct which changes correspond to which problems.

          Here is the new test case which I'm going to tackle next. It fails during compilation because a result set number hasn't been assigned to the column reference in the matching refinement of the WHEN NOT MATCHED clause:

          create view sr_040( i ) as values 1;
          create table t1_040( x int, y int, z int );
          create unique index idx on t1_040( x, y );
          
          insert into t1_040 values
          ( 1, 100, 1000 ), ( 1, 101, 1000 ), ( 1, 102, 1000 ), ( 1, 103, 1000 ), ( 2, 200, 2000 );
          
          merge into t1_040
          using sr_040 on ( x = 1 )
          when matched and y = 101 then delete
          when matched and y = 102 then update set z = -1000
          when not matched and i = 1 then insert values ( -1, 0, 0 );
          

          Touches the following files:

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

          M java/engine/org/apache/derby/impl/sql/compile/FromBaseTable.java

          The fix for 1).

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

          M java/engine/org/apache/derby/iapi/sql/execute/ResultSetFactory.java
          M java/engine/org/apache/derby/impl/sql/compile/IndexToBaseRowNode.java
          M java/engine/org/apache/derby/impl/sql/execute/GenericResultSetFactory.java
          M java/engine/org/apache/derby/impl/sql/execute/IndexRowToBaseRowResultSet.java

          The fix for 2).

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

          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java

          A new test case to track this improvement.

          Show
          Rick Hillegas added a comment - Attaching derby-3155-26-aa-copyRowLocationForIndexScans.diff. This patch fixes a compilation problem with a MERGE statement involving a view. Regression tests ran cleanly for me on this patch. The problem query, found by Knut, was this: create view sr_039( i ) as values 1; create table t1_039( x int, y int, z int ); create unique index idx on t1_039( x, y ); insert into t1_039 values ( 1, 100, 1000 ), ( 2, 200, 2000 ); merge into t1_039 using sr_039 on ( x = 1 ) when matched then delete; The fix involves two changes: 1) There was a place in FromBaseTable where a ResultColumnList was being cloned but conglomerate information wasn't being propagated from the original list. That just looks like an existing bug to me. However, I don't now why we haven't tripped over it before. 2) Once I cleaned that up, I stumbled on another bug: Index probes into base tables weren't equipped to hand back the RowLocations needed by the MERGE statement's driving left join. After fixing these bugs, Knut's original test case works. But I'm not declaring victory yet. I have tried a variation on that original test case and tripped across yet another bug. I think that the fix for that bug deserves its own patch so that we can clearly reconstruct which changes correspond to which problems. Here is the new test case which I'm going to tackle next. It fails during compilation because a result set number hasn't been assigned to the column reference in the matching refinement of the WHEN NOT MATCHED clause: create view sr_040( i ) as values 1; create table t1_040( x int, y int, z int ); create unique index idx on t1_040( x, y ); insert into t1_040 values ( 1, 100, 1000 ), ( 1, 101, 1000 ), ( 1, 102, 1000 ), ( 1, 103, 1000 ), ( 2, 200, 2000 ); merge into t1_040 using sr_040 on ( x = 1 ) when matched and y = 101 then delete when matched and y = 102 then update set z = -1000 when not matched and i = 1 then insert values ( -1, 0, 0 ); Touches the following files: -------------------- M java/engine/org/apache/derby/impl/sql/compile/FromBaseTable.java The fix for 1). -------------------- M java/engine/org/apache/derby/iapi/sql/execute/ResultSetFactory.java M java/engine/org/apache/derby/impl/sql/compile/IndexToBaseRowNode.java M java/engine/org/apache/derby/impl/sql/execute/GenericResultSetFactory.java M java/engine/org/apache/derby/impl/sql/execute/IndexRowToBaseRowResultSet.java The fix for 2). -------------------- M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java A new test case to track this improvement.
          Rick Hillegas made changes -
          Hide
          ASF subversion and git services added a comment -

          Commit 1569396 from Rick Hillegas in branch 'code/trunk'
          [ https://svn.apache.org/r1569396 ]

          DERBY-3155: Propagate RowLocations out of index probes as necessary for the driving left join of a MERGE statement; commits derby-3155-26-aa-copyRowLocationForIndexScans.diff.

          Show
          ASF subversion and git services added a comment - Commit 1569396 from Rick Hillegas in branch 'code/trunk' [ https://svn.apache.org/r1569396 ] DERBY-3155 : Propagate RowLocations out of index probes as necessary for the driving left join of a MERGE statement; commits derby-3155-26-aa-copyRowLocationForIndexScans.diff.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-27-aa-adjustMatchingRefinements.diff. This patch fixes the compilation problem with a matching refinement clause that referenced a column in a source table which was a view wrapping a values clause.

          I think that the view was flattened but this information was not communicated to the matching refinement clause. The solution was to point the column references in the matching refinement clauses at the row coming out of the driving left join.

          This fixes the following problem case:

          create view sr_040( i ) as values ( 1 );
          create table t1_040( x int, y int, z int );
          create unique index idx on t1_040( x, y );
          
          insert into t1_040 values
          ( 1, 100, 1000 ), ( 1, 101, 1000 ), ( 1, 102, 1000 ), ( 1, 103, 1000 ), ( 2, 200, 2000 );
          
          merge into t1_040
          using sr_040 on ( x = 1 )
          when matched and y = 101 then delete
          when matched and y = 102 then update set z = -1000
          when not matched and i > 1 then insert values ( -1, i, 0 );
          
          select * from t1_040 order by x, y, z;
          
          merge into t1_040
          using sr_040 on ( x = 3 )
          when matched and y = 103 then delete
          when matched and y = 102 then update set z = -10000
          when not matched and i = 1 then insert values ( -1, i, 0 );
          
          select * from t1_040 order by x, y, z;
          

          But we're still not out of the woods with the original problem case which Knut found. Adding another row to the values view causes us to fail on a missing row:

          create view sr_040( i ) as values ( 1 ), ( 3 );
          create table t1_040( x int, y int, z int );
          create unique index idx on t1_040( x, y );
          
          insert into t1_040 values
          ( 1, 100, 1000 ), ( 1, 101, 1000 ), ( 1, 102, 1000 ), ( 1, 103, 1000 ), ( 2, 200, 2000 );
          
          merge into t1_040
          using sr_040 on ( x = 1 )
          when matched and y = 101 then delete
          when matched and y = 102 then update set z = -1000
          when not matched and i > 1 then insert values ( -1, i, 0 );
          

          That, again, looks like a different bug which deserves its own separate patch.

          Touches the following files:

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

          M java/engine/org/apache/derby/impl/sql/compile/MergeNode.java
          M java/engine/org/apache/derby/impl/sql/compile/MatchingClauseNode.java

          The fix.

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

          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java

          The test case.

          Show
          Rick Hillegas added a comment - Attaching derby-3155-27-aa-adjustMatchingRefinements.diff. This patch fixes the compilation problem with a matching refinement clause that referenced a column in a source table which was a view wrapping a values clause. I think that the view was flattened but this information was not communicated to the matching refinement clause. The solution was to point the column references in the matching refinement clauses at the row coming out of the driving left join. This fixes the following problem case: create view sr_040( i ) as values ( 1 ); create table t1_040( x int, y int, z int ); create unique index idx on t1_040( x, y ); insert into t1_040 values ( 1, 100, 1000 ), ( 1, 101, 1000 ), ( 1, 102, 1000 ), ( 1, 103, 1000 ), ( 2, 200, 2000 ); merge into t1_040 using sr_040 on ( x = 1 ) when matched and y = 101 then delete when matched and y = 102 then update set z = -1000 when not matched and i > 1 then insert values ( -1, i, 0 ); select * from t1_040 order by x, y, z; merge into t1_040 using sr_040 on ( x = 3 ) when matched and y = 103 then delete when matched and y = 102 then update set z = -10000 when not matched and i = 1 then insert values ( -1, i, 0 ); select * from t1_040 order by x, y, z; But we're still not out of the woods with the original problem case which Knut found. Adding another row to the values view causes us to fail on a missing row: create view sr_040( i ) as values ( 1 ), ( 3 ); create table t1_040( x int, y int, z int ); create unique index idx on t1_040( x, y ); insert into t1_040 values ( 1, 100, 1000 ), ( 1, 101, 1000 ), ( 1, 102, 1000 ), ( 1, 103, 1000 ), ( 2, 200, 2000 ); merge into t1_040 using sr_040 on ( x = 1 ) when matched and y = 101 then delete when matched and y = 102 then update set z = -1000 when not matched and i > 1 then insert values ( -1, i, 0 ); That, again, looks like a different bug which deserves its own separate patch. Touches the following files: --------------- M java/engine/org/apache/derby/impl/sql/compile/MergeNode.java M java/engine/org/apache/derby/impl/sql/compile/MatchingClauseNode.java The fix. --------------- M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java The test case.
          Rick Hillegas made changes -
          Hide
          ASF subversion and git services added a comment -

          Commit 1569521 from Rick Hillegas in branch 'code/trunk'
          [ https://svn.apache.org/r1569521 ]

          DERBY-3155: Point matching refinement clauses into the row returned by the driving left join of the MERGE statement; commit derby-3155-27-aa-adjustMatchingRefinements.diff.

          Show
          ASF subversion and git services added a comment - Commit 1569521 from Rick Hillegas in branch 'code/trunk' [ https://svn.apache.org/r1569521 ] DERBY-3155 : Point matching refinement clauses into the row returned by the driving left join of the MERGE statement; commit derby-3155-27-aa-adjustMatchingRefinements.diff.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-28-aa-cardinalityViolations.diff. This patch addresses the last issue I discussed above: an attempt to delete the same row twice. Tests ran cleanly against this patch.

          The bug arises because I had not implemented the following required behavior of the MERGE statement: The database should raise a cardinality exception if an attempt is made to delete/update a target row more than once. See the 2011 SQL Standard, part 2, section 14.12 (merge statement), general rule 6.

          The fix is to maintain a BackingStoreHashtable of row locations from touched target rows and to raise an error if a row is touched twice.

          I will also note this behavior in the functional spec.

          Affects the following files:

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

          M java/engine/org/apache/derby/iapi/types/SQLRef.java

          This class needed to override hashCode() so that SQLRefs could be used as hash keys.

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

          M java/engine/org/apache/derby/impl/sql/execute/MergeResultSet.java

          Maintain a BackingStoreHashtable of row locations of subject rows and raise a cardinality exception if the same row qualifies twice.

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

          M java/engine/org/apache/derby/loc/messages.xml
          M java/shared/org/apache/derby/shared/common/reference/SQLState.java

          New cardinality violation message.

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

          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java

          Test cases.

          Show
          Rick Hillegas added a comment - Attaching derby-3155-28-aa-cardinalityViolations.diff. This patch addresses the last issue I discussed above: an attempt to delete the same row twice. Tests ran cleanly against this patch. The bug arises because I had not implemented the following required behavior of the MERGE statement: The database should raise a cardinality exception if an attempt is made to delete/update a target row more than once. See the 2011 SQL Standard, part 2, section 14.12 (merge statement), general rule 6. The fix is to maintain a BackingStoreHashtable of row locations from touched target rows and to raise an error if a row is touched twice. I will also note this behavior in the functional spec. Affects the following files: -------------- M java/engine/org/apache/derby/iapi/types/SQLRef.java This class needed to override hashCode() so that SQLRefs could be used as hash keys. -------------- M java/engine/org/apache/derby/impl/sql/execute/MergeResultSet.java Maintain a BackingStoreHashtable of row locations of subject rows and raise a cardinality exception if the same row qualifies twice. -------------- M java/engine/org/apache/derby/loc/messages.xml M java/shared/org/apache/derby/shared/common/reference/SQLState.java New cardinality violation message. -------------- M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java Test cases.
          Rick Hillegas made changes -
          Hide
          ASF subversion and git services added a comment -

          Commit 1570230 from Rick Hillegas in branch 'code/trunk'
          [ https://svn.apache.org/r1570230 ]

          DERBY-3155: Prevent a MERGE statement from altering the same target row twice; commit derby-3155-28-aa-cardinalityViolations.diff.

          Show
          ASF subversion and git services added a comment - Commit 1570230 from Rick Hillegas in branch 'code/trunk' [ https://svn.apache.org/r1570230 ] DERBY-3155 : Prevent a MERGE statement from altering the same target row twice; commit derby-3155-28-aa-cardinalityViolations.diff.
          Hide
          Rick Hillegas added a comment -

          Attaching a fourth rev of the functional spec. This incorporates several changes which have accumulated since August.

          Show
          Rick Hillegas added a comment - Attaching a fourth rev of the functional spec. This incorporates several changes which have accumulated since August.
          Rick Hillegas made changes -
          Attachment MergeStatement.html [ 12630075 ]
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-29-aa-missingSchema.diff. This patch removes some obsolete code which was causing us to see a missing schema error for the following case:

          connect 'jdbc:derby:memory:db;user=test_dbo;create=true';
          
          call syscs_util.syscs_create_user( 'TEST_DBO', 'test_dbopassword' );
          call syscs_util.syscs_create_user( 'RUTH', 'ruthpassword' );
          
          connect 'jdbc:derby:memory:db;shutdown=true';
          
          connect 'jdbc:derby:memory:db;user=test_dbo;password=test_dbopassword' as dbo;
          
          create table deleteTable_042
          (
              publicSelectColumn int
          );
          
          create table selectTable_042
          (
              selectColumn int
          );
          
          grant select on  selectTable_042 to public;
          grant select on  deleteTable_042 to public;
          grant delete on deleteTable_042 to ruth;
          
          connect 'jdbc:derby:memory:db;user=ruth;password=ruthpassword' as ruth;
          
          merge into test_dbo.deleteTable_042
          using test_dbo.selectTable_042
          on publicSelectColumn = selectColumn
          when matched then delete;
          

          Touches the following files:

          M java/engine/org/apache/derby/impl/sql/compile/MergeNode.java
          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java

          Show
          Rick Hillegas added a comment - Attaching derby-3155-29-aa-missingSchema.diff. This patch removes some obsolete code which was causing us to see a missing schema error for the following case: connect 'jdbc:derby:memory:db;user=test_dbo;create=true'; call syscs_util.syscs_create_user( 'TEST_DBO', 'test_dbopassword' ); call syscs_util.syscs_create_user( 'RUTH', 'ruthpassword' ); connect 'jdbc:derby:memory:db;shutdown=true'; connect 'jdbc:derby:memory:db;user=test_dbo;password=test_dbopassword' as dbo; create table deleteTable_042 ( publicSelectColumn int ); create table selectTable_042 ( selectColumn int ); grant select on selectTable_042 to public; grant select on deleteTable_042 to public; grant delete on deleteTable_042 to ruth; connect 'jdbc:derby:memory:db;user=ruth;password=ruthpassword' as ruth; merge into test_dbo.deleteTable_042 using test_dbo.selectTable_042 on publicSelectColumn = selectColumn when matched then delete; Touches the following files: M java/engine/org/apache/derby/impl/sql/compile/MergeNode.java M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java
          Rick Hillegas made changes -
          Attachment derby-3155-29-aa-missingSchema.diff [ 12630147 ]
          Hide
          ASF subversion and git services added a comment -

          Commit 1570352 from Rick Hillegas in branch 'code/trunk'
          [ https://svn.apache.org/r1570352 ]

          DERBY-3155: Remove some obsolete code which was causing a cross-schema MERGE statement to raise a missing schema error: commit derby-3155-29-aa-missingSchema.diff.

          Show
          ASF subversion and git services added a comment - Commit 1570352 from Rick Hillegas in branch 'code/trunk' [ https://svn.apache.org/r1570352 ] DERBY-3155 : Remove some obsolete code which was causing a cross-schema MERGE statement to raise a missing schema error: commit derby-3155-29-aa-missingSchema.diff.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-30-ab-moreCorrelationNames.diff. This patch fixes more column resolution problems. I am running tests now.

          Working on this patch has helped me understand a defect (or at least a brittleness) in my approach to implementing the UPDATE action of the MERGE statement. The dummy UPDATE statement I create is one which can not arise via the supported Derby grammar. That is because this UPDATE statement is driven by a SELECT having more than one table. An ordinary Derby UPDATE can be driven by a multi-table SELECT but only if the SELECT is hidden behind a WHERE CURRENT OF clause.

          The existing logic takes advantage of an assumption that the driving SELECT only has columns from the table being UPDATEd. In particular, there is some UpdateNode logic (around line 1530) which clears out table identifiers from the UPDATE statement's column list. It appears that that logic was added as part of the work on DERBY-1043. But without those table identifiers, I can't distinguish between columns coming from the source vs. the target table of a MERGE statement.

          My original solution to this problem was to skip the logic which clears out table identifiers when I am binding a MERGE statement. That worked for a long time until I started trying to compile UPDATE actions which supplement their column lists with extra columns needed to evaluate constraints, triggers, and generation expressions. I re-instated id-clearing logic in order to make those statements compile.

          This revived the ambiguity when the source and target tables had columns with the same name. My solution is to double-down on associating all columns with the source vs. target tables before binding the INSERT/UPDATE/DELETE actions.

          It's possible that I will continue to be buried under a pile of column resolution problems. I may need to fall back and re-implement the UPDATE action so that it uses some dummy column list which could be concocted via SQL and then add some substitution logic to map between the dummy column list and the expressions in the SET clauses.

          However, for the moment I'm doubling down on the current implementation.

          Touches the following files:

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

          M java/engine/org/apache/derby/iapi/sql/compile/TagFilter.java
          M java/engine/org/apache/derby/impl/sql/compile/UpdateNode.java

          Columns added to satisfy constraints, triggers, and generation clauses are assocated with the TARGET table. The table-id-clearing logic is re-instated.

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

          M java/engine/org/apache/derby/impl/sql/compile/MergeNode.java
          M java/engine/org/apache/derby/impl/sql/compile/MatchingClauseNode.java
          M java/engine/org/apache/derby/impl/sql/compile/DMLModStatementNode.java
          M java/engine/org/apache/derby/impl/sql/compile/FromBaseTable.java
          M java/engine/org/apache/derby/impl/sql/compile/FromTable.java

          More logic to associate columns with the source or target table.

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

          M java/engine/org/apache/derby/impl/sql/compile/FromList.java

          Use the pre-computed association of columns with source/target tables to resolve column references.

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

          M java/engine/org/apache/derby/impl/sql/compile/ColumnReference.java

          Minor tweak to improve encapsulation and tracing of this class.

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

          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java

          Additional tests.

          Show
          Rick Hillegas added a comment - Attaching derby-3155-30-ab-moreCorrelationNames.diff. This patch fixes more column resolution problems. I am running tests now. Working on this patch has helped me understand a defect (or at least a brittleness) in my approach to implementing the UPDATE action of the MERGE statement. The dummy UPDATE statement I create is one which can not arise via the supported Derby grammar. That is because this UPDATE statement is driven by a SELECT having more than one table. An ordinary Derby UPDATE can be driven by a multi-table SELECT but only if the SELECT is hidden behind a WHERE CURRENT OF clause. The existing logic takes advantage of an assumption that the driving SELECT only has columns from the table being UPDATEd. In particular, there is some UpdateNode logic (around line 1530) which clears out table identifiers from the UPDATE statement's column list. It appears that that logic was added as part of the work on DERBY-1043 . But without those table identifiers, I can't distinguish between columns coming from the source vs. the target table of a MERGE statement. My original solution to this problem was to skip the logic which clears out table identifiers when I am binding a MERGE statement. That worked for a long time until I started trying to compile UPDATE actions which supplement their column lists with extra columns needed to evaluate constraints, triggers, and generation expressions. I re-instated id-clearing logic in order to make those statements compile. This revived the ambiguity when the source and target tables had columns with the same name. My solution is to double-down on associating all columns with the source vs. target tables before binding the INSERT/UPDATE/DELETE actions. It's possible that I will continue to be buried under a pile of column resolution problems. I may need to fall back and re-implement the UPDATE action so that it uses some dummy column list which could be concocted via SQL and then add some substitution logic to map between the dummy column list and the expressions in the SET clauses. However, for the moment I'm doubling down on the current implementation. Touches the following files: ---------------- M java/engine/org/apache/derby/iapi/sql/compile/TagFilter.java M java/engine/org/apache/derby/impl/sql/compile/UpdateNode.java Columns added to satisfy constraints, triggers, and generation clauses are assocated with the TARGET table. The table-id-clearing logic is re-instated. ---------------- M java/engine/org/apache/derby/impl/sql/compile/MergeNode.java M java/engine/org/apache/derby/impl/sql/compile/MatchingClauseNode.java M java/engine/org/apache/derby/impl/sql/compile/DMLModStatementNode.java M java/engine/org/apache/derby/impl/sql/compile/FromBaseTable.java M java/engine/org/apache/derby/impl/sql/compile/FromTable.java More logic to associate columns with the source or target table. ---------------- M java/engine/org/apache/derby/impl/sql/compile/FromList.java Use the pre-computed association of columns with source/target tables to resolve column references. ---------------- M java/engine/org/apache/derby/impl/sql/compile/ColumnReference.java Minor tweak to improve encapsulation and tracing of this class. ---------------- M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java Additional tests.
          Rick Hillegas made changes -
          Hide
          ASF subversion and git services added a comment -

          Commit 1571808 from Rick Hillegas in branch 'code/trunk'
          [ https://svn.apache.org/r1571808 ]

          DERBY-3155: More improvements to column resolution in MERGE statements; tests passed cleanly for me on derby-3155-30-ab-moreCorrelationNames.diff.

          Show
          ASF subversion and git services added a comment - Commit 1571808 from Rick Hillegas in branch 'code/trunk' [ https://svn.apache.org/r1571808 ] DERBY-3155 : More improvements to column resolution in MERGE statements; tests passed cleanly for me on derby-3155-30-ab-moreCorrelationNames.diff.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-31-aa-deletePrivs.diff. This patch corrects privilege management for the DELETE actions of MERGE statements. I am running tests now.

          It's my understanding that the user needs the following permissions in order to run a MERGE statement with a DELETE action. This is what the patch attempts to implement and test:

          1) DELETE permission on the target table.

          2) For the ON clause and the search condition in the WHEN MATCHED clause

          a) SELECT permission on all referenced columns
          b) EXECUTE permission on all referenced routines
          c) USAGE privilege on all referenced types

          Touches the following files:

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

          M java/engine/org/apache/derby/impl/sql/compile/QueryTreeNode.java
          M java/engine/org/apache/derby/impl/sql/compile/UpdateNode.java

          Move some privilege setting (for UDTs) from UpdateNode up to QueryTreeNode for use by all nodes.

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

          M java/engine/org/apache/derby/impl/sql/compile/FromBaseTable.java

          Good code hygiene. Added some braces for a dangling consequence of an if statement.

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

          M java/engine/org/apache/derby/impl/sql/compile/MergeNode.java
          M java/engine/org/apache/derby/impl/sql/compile/MatchingClauseNode.java
          M java/engine/org/apache/derby/impl/sql/compile/DeleteNode.java

          Turn off privilege checking at inappropriate moments.

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

          A java/testing/org/apache/derbyTesting/functionTests/tests/lang/Permission.java
          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/GeneratedColumnsHelper.java
          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java
          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/GrantRevokeDDLTest.java

          Tests.

          Show
          Rick Hillegas added a comment - Attaching derby-3155-31-aa-deletePrivs.diff. This patch corrects privilege management for the DELETE actions of MERGE statements. I am running tests now. It's my understanding that the user needs the following permissions in order to run a MERGE statement with a DELETE action. This is what the patch attempts to implement and test: 1) DELETE permission on the target table. 2) For the ON clause and the search condition in the WHEN MATCHED clause a) SELECT permission on all referenced columns b) EXECUTE permission on all referenced routines c) USAGE privilege on all referenced types Touches the following files: ---------------- M java/engine/org/apache/derby/impl/sql/compile/QueryTreeNode.java M java/engine/org/apache/derby/impl/sql/compile/UpdateNode.java Move some privilege setting (for UDTs) from UpdateNode up to QueryTreeNode for use by all nodes. ---------------- M java/engine/org/apache/derby/impl/sql/compile/FromBaseTable.java Good code hygiene. Added some braces for a dangling consequence of an if statement. ---------------- M java/engine/org/apache/derby/impl/sql/compile/MergeNode.java M java/engine/org/apache/derby/impl/sql/compile/MatchingClauseNode.java M java/engine/org/apache/derby/impl/sql/compile/DeleteNode.java Turn off privilege checking at inappropriate moments. ---------------- A java/testing/org/apache/derbyTesting/functionTests/tests/lang/Permission.java M java/testing/org/apache/derbyTesting/functionTests/tests/lang/GeneratedColumnsHelper.java M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java M java/testing/org/apache/derbyTesting/functionTests/tests/lang/GrantRevokeDDLTest.java Tests.
          Rick Hillegas made changes -
          Attachment derby-3155-31-aa-deletePrivs.diff [ 12631551 ]
          Hide
          ASF subversion and git services added a comment -

          Commit 1572665 from Rick Hillegas in branch 'code/trunk'
          [ https://svn.apache.org/r1572665 ]

          DERBY-3155: Enforce correct privileges for DELETE actions of MERGE statements; tests passed cleanly on derby-3155-31-aa-deletePrivs.diff.

          Show
          ASF subversion and git services added a comment - Commit 1572665 from Rick Hillegas in branch 'code/trunk' [ https://svn.apache.org/r1572665 ] DERBY-3155 : Enforce correct privileges for DELETE actions of MERGE statements; tests passed cleanly on derby-3155-31-aa-deletePrivs.diff.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-32-aa-newTestFunction.diff. This patch adds another test function for use in testing privileges on user-defined types.

          Touches the following file:

          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/UDTTest.java

          Show
          Rick Hillegas added a comment - Attaching derby-3155-32-aa-newTestFunction.diff. This patch adds another test function for use in testing privileges on user-defined types. Touches the following file: M java/testing/org/apache/derbyTesting/functionTests/tests/lang/UDTTest.java
          Rick Hillegas made changes -
          Attachment derby-3155-32-aa-newTestFunction.diff [ 12631734 ]
          Hide
          ASF subversion and git services added a comment -

          Commit 1572949 from Rick Hillegas in branch 'code/trunk'
          [ https://svn.apache.org/r1572949 ]

          DERBY-3155: Add a test function to help in testing of udt privileges; commit derby-3155-32-aa-newTestFunction.diff.

          Show
          ASF subversion and git services added a comment - Commit 1572949 from Rick Hillegas in branch 'code/trunk' [ https://svn.apache.org/r1572949 ] DERBY-3155 : Add a test function to help in testing of udt privileges; commit derby-3155-32-aa-newTestFunction.diff.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-33-ab-insertPrivs.diff. This patch adds privilege checks for the INSERT actions of MERGE statements. I am running tests now.

          Touches the following files:

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

          M java/engine/org/apache/derby/impl/sql/compile/CompilerContextImpl.java

          Adds braces around a dangling consequence.

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

          M java/engine/org/apache/derby/impl/sql/compile/MergeNode.java
          M java/engine/org/apache/derby/impl/sql/compile/MatchingClauseNode.java
          M java/engine/org/apache/derby/impl/sql/compile/InsertNode.java

          Changes to support privilege checks for the INSERT actions of MERGE statements.

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

          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java

          Tests.

          Show
          Rick Hillegas added a comment - Attaching derby-3155-33-ab-insertPrivs.diff. This patch adds privilege checks for the INSERT actions of MERGE statements. I am running tests now. Touches the following files: ------------ M java/engine/org/apache/derby/impl/sql/compile/CompilerContextImpl.java Adds braces around a dangling consequence. ------------ M java/engine/org/apache/derby/impl/sql/compile/MergeNode.java M java/engine/org/apache/derby/impl/sql/compile/MatchingClauseNode.java M java/engine/org/apache/derby/impl/sql/compile/InsertNode.java Changes to support privilege checks for the INSERT actions of MERGE statements. ------------ M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java Tests.
          Rick Hillegas made changes -
          Attachment derby-3155-33-ab-insertPrivs.diff [ 12632509 ]
          Hide
          ASF subversion and git services added a comment -

          Commit 1574131 from Rick Hillegas in branch 'code/trunk'
          [ https://svn.apache.org/r1574131 ]

          DERBY-3155: Add privilege checks for the INSERT actions of MERGE statements; tests passed cleanly on derby-3155-33-ab-insertPrivs.diff.

          Show
          ASF subversion and git services added a comment - Commit 1574131 from Rick Hillegas in branch 'code/trunk' [ https://svn.apache.org/r1574131 ] DERBY-3155 : Add privilege checks for the INSERT actions of MERGE statements; tests passed cleanly on derby-3155-33-ab-insertPrivs.diff.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-34-aa-updatePrivs.diff. This patch adds privilege checking for UPDATE actions of MERGE statements. I am running tests now.

          Touches the following files:

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

          M java/engine/org/apache/derby/impl/sql/compile/MatchingClauseNode.java
          M java/engine/org/apache/derby/impl/sql/compile/UpdateNode.java

          Add privilege checks for UPDATE actions.

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

          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java

          Tests.

          Show
          Rick Hillegas added a comment - Attaching derby-3155-34-aa-updatePrivs.diff. This patch adds privilege checking for UPDATE actions of MERGE statements. I am running tests now. Touches the following files: --------------- M java/engine/org/apache/derby/impl/sql/compile/MatchingClauseNode.java M java/engine/org/apache/derby/impl/sql/compile/UpdateNode.java Add privilege checks for UPDATE actions. --------------- M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java Tests.
          Rick Hillegas made changes -
          Attachment derby-3155-34-aa-updatePrivs.diff [ 12632838 ]
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-34-ab-updatePrivs.diff. The previous rev of the patch raised 3 errors in GrantRevokeDDLTest because of canonized wrong results. This revised patch fixes those test cases.

          Touches the following additional file:

          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/GrantRevokeDDLTest.java

          Show
          Rick Hillegas added a comment - Attaching derby-3155-34-ab-updatePrivs.diff. The previous rev of the patch raised 3 errors in GrantRevokeDDLTest because of canonized wrong results. This revised patch fixes those test cases. Touches the following additional file: M java/testing/org/apache/derbyTesting/functionTests/tests/lang/GrantRevokeDDLTest.java
          Rick Hillegas made changes -
          Attachment derby-3155-34-ab-updatePrivs.diff [ 12632846 ]
          Hide
          ASF subversion and git services added a comment -

          Commit 1574566 from Rick Hillegas in branch 'code/trunk'
          [ https://svn.apache.org/r1574566 ]

          DERBY-3155: Add privilege checks for the UPDATE actions of MERGE statements; commit derby-3155-34-ab-updatePrivs.diff.

          Show
          ASF subversion and git services added a comment - Commit 1574566 from Rick Hillegas in branch 'code/trunk' [ https://svn.apache.org/r1574566 ] DERBY-3155 : Add privilege checks for the UPDATE actions of MERGE statements; commit derby-3155-34-ab-updatePrivs.diff.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-35-aa-allPrivsTest.diff. This patch adds a test for privilege checking on a MERGE statement with INSERT, UPDATE, and DELETE clauses.

          Touches the following file:

          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java

          Show
          Rick Hillegas added a comment - Attaching derby-3155-35-aa-allPrivsTest.diff. This patch adds a test for privilege checking on a MERGE statement with INSERT, UPDATE, and DELETE clauses. Touches the following file: M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java
          Rick Hillegas made changes -
          Attachment derby-3155-35-aa-allPrivsTest.diff [ 12633172 ]
          Hide
          ASF subversion and git services added a comment -

          Commit 1574956 from Rick Hillegas in branch 'code/trunk'
          [ https://svn.apache.org/r1574956 ]

          DERBY-3155: Add a test case for privilege checking on a MERGE statement with all 3 kinds of actions; commit derby-3155-35-aa-allPrivsTest.diff.

          Show
          ASF subversion and git services added a comment - Commit 1574956 from Rick Hillegas in branch 'code/trunk' [ https://svn.apache.org/r1574956 ] DERBY-3155 : Add a test case for privilege checking on a MERGE statement with all 3 kinds of actions; commit derby-3155-35-aa-allPrivsTest.diff.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-36-aa-lockModeComment.diff. This replaces some commented-out code with a comment saying that there is no need to set a global lock mode for the MERGE statement. The compiler determines individual lock modes for the driving left-join and for each INSERT, UPDATE, DELETE action.

          Touches the following file:

          M java/engine/org/apache/derby/impl/sql/compile/MergeNode.java

          Show
          Rick Hillegas added a comment - Attaching derby-3155-36-aa-lockModeComment.diff. This replaces some commented-out code with a comment saying that there is no need to set a global lock mode for the MERGE statement. The compiler determines individual lock modes for the driving left-join and for each INSERT, UPDATE, DELETE action. Touches the following file: M java/engine/org/apache/derby/impl/sql/compile/MergeNode.java
          Rick Hillegas made changes -
          Attachment derby-3155-36-aa-lockModeComment.diff [ 12633214 ]
          Hide
          ASF subversion and git services added a comment -

          Commit 1575026 from Rick Hillegas in branch 'code/trunk'
          [ https://svn.apache.org/r1575026 ]

          DERBY-3155: Add lock mode comment to master MERGE node; commit derby-3155-36-aa-lockModeComment.diff.

          Show
          ASF subversion and git services added a comment - Commit 1575026 from Rick Hillegas in branch 'code/trunk' [ https://svn.apache.org/r1575026 ] DERBY-3155 : Add lock mode comment to master MERGE node; commit derby-3155-36-aa-lockModeComment.diff.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-37-aa-printSubNodes.diff. This patch implements the printSubNodes() debug method for MergeNode and MatchingClauseNode().

          Touches the following files:

          M java/engine/org/apache/derby/impl/sql/compile/MergeNode.java
          M java/engine/org/apache/derby/impl/sql/compile/MatchingClauseNode.java
          M java/engine/org/apache/derby/impl/sql/compile/DMLModStatementNode.java

          Show
          Rick Hillegas added a comment - Attaching derby-3155-37-aa-printSubNodes.diff. This patch implements the printSubNodes() debug method for MergeNode and MatchingClauseNode(). Touches the following files: M java/engine/org/apache/derby/impl/sql/compile/MergeNode.java M java/engine/org/apache/derby/impl/sql/compile/MatchingClauseNode.java M java/engine/org/apache/derby/impl/sql/compile/DMLModStatementNode.java
          Rick Hillegas made changes -
          Attachment derby-3155-37-aa-printSubNodes.diff [ 12633221 ]
          Hide
          ASF subversion and git services added a comment -

          Commit 1575032 from Rick Hillegas in branch 'code/trunk'
          [ https://svn.apache.org/r1575032 ]

          DERBY-3155: Implement printSubNodes() for MergeNode and MatchingClauseNode; commit derby-3155-37-aa-printSubNodes.diff.

          Show
          ASF subversion and git services added a comment - Commit 1575032 from Rick Hillegas in branch 'code/trunk' [ https://svn.apache.org/r1575032 ] DERBY-3155 : Implement printSubNodes() for MergeNode and MatchingClauseNode; commit derby-3155-37-aa-printSubNodes.diff.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-38-aa-datatypes.diff. This patch adds tests for using all datatypes in MERGE statements. I am running tests now.

          These tests uncovered one bug: We were not able to use XML datatypes in MERGE statements. The driving left-join failed to compile because the compiler thought that the XML types were going to be returned to the client. The solution was to make CursorNode not forbid XML types in its select list when it has been cooked up to support a MERGE statement.

          Touches the following files:

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

          M java/engine/org/apache/derby/impl/sql/compile/MergeNode.java
          M java/engine/org/apache/derby/impl/sql/compile/CursorNode.java
          M java/engine/org/apache/derby/impl/sql/compile/sqlgrammar.jj

          Allow XML types in the select lists of the driving left-joins which support MERGE statements.

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

          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java
          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/IntArray.java

          Tests for MERGE statements which exercise all Derby datatypes.

          Show
          Rick Hillegas added a comment - Attaching derby-3155-38-aa-datatypes.diff. This patch adds tests for using all datatypes in MERGE statements. I am running tests now. These tests uncovered one bug: We were not able to use XML datatypes in MERGE statements. The driving left-join failed to compile because the compiler thought that the XML types were going to be returned to the client. The solution was to make CursorNode not forbid XML types in its select list when it has been cooked up to support a MERGE statement. Touches the following files: --------------- M java/engine/org/apache/derby/impl/sql/compile/MergeNode.java M java/engine/org/apache/derby/impl/sql/compile/CursorNode.java M java/engine/org/apache/derby/impl/sql/compile/sqlgrammar.jj Allow XML types in the select lists of the driving left-joins which support MERGE statements. --------------- M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java M java/testing/org/apache/derbyTesting/functionTests/tests/lang/IntArray.java Tests for MERGE statements which exercise all Derby datatypes.
          Rick Hillegas made changes -
          Attachment derby-3155-38-aa-datatypes.diff [ 12633726 ]
          Hide
          ASF subversion and git services added a comment -

          Commit 1576027 from Rick Hillegas in branch 'code/trunk'
          [ https://svn.apache.org/r1576027 ]

          DERBY-3155: Add datatype tests for MERGE statement and fix bug involving xml types in MERGE statements; tests passed cleanly on derby-3155-38-aa-datatypes.diff.

          Show
          ASF subversion and git services added a comment - Commit 1576027 from Rick Hillegas in branch 'code/trunk' [ https://svn.apache.org/r1576027 ] DERBY-3155 : Add datatype tests for MERGE statement and fix bug involving xml types in MERGE statements; tests passed cleanly on derby-3155-38-aa-datatypes.diff.
          Hide
          Knut Anders Hatlen added a comment -

          The last commit introduced a dependency on a Java 8 method in MergeStatementTest, and that made the Jenkins build fail. See https://builds.apache.org/job/Derby-trunk/2093/console. I think you can use Math.min() instead of Integer.min().

          Show
          Knut Anders Hatlen added a comment - The last commit introduced a dependency on a Java 8 method in MergeStatementTest, and that made the Jenkins build fail. See https://builds.apache.org/job/Derby-trunk/2093/console . I think you can use Math.min() instead of Integer.min().
          Hide
          Rick Hillegas added a comment -

          Thanks for spotting that, Knut. Attaching derby-3155-39-aa-fixBuild.diff. This fixes the build problem. I'm also introducing a new Blob-making method for my next batch of tests.

          I have verified that the build fails on Java 7 without this fix, but works with this fix.

          Touches the following file:

          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java

          Show
          Rick Hillegas added a comment - Thanks for spotting that, Knut. Attaching derby-3155-39-aa-fixBuild.diff. This fixes the build problem. I'm also introducing a new Blob-making method for my next batch of tests. I have verified that the build fails on Java 7 without this fix, but works with this fix. Touches the following file: M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java
          Rick Hillegas made changes -
          Attachment derby-3155-39-aa-fixBuild.diff [ 12633754 ]
          Hide
          ASF subversion and git services added a comment -

          Commit 1576062 from Rick Hillegas in branch 'code/trunk'
          [ https://svn.apache.org/r1576062 ]

          DERBY-3155: Fix build problem introduced by derby-3155-38-aa-datatypes.diff; commit derby-3155-39-aa-fixBuild.diff.

          Show
          ASF subversion and git services added a comment - Commit 1576062 from Rick Hillegas in branch 'code/trunk' [ https://svn.apache.org/r1576062 ] DERBY-3155 : Fix build problem introduced by derby-3155-38-aa-datatypes.diff; commit derby-3155-39-aa-fixBuild.diff.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-40-aa-bigLobs.diff. This patch adds some tests for largish lobs.

          Touches the following file:

          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java

          Show
          Rick Hillegas added a comment - Attaching derby-3155-40-aa-bigLobs.diff. This patch adds some tests for largish lobs. Touches the following file: M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java
          Rick Hillegas made changes -
          Attachment derby-3155-40-aa-bigLobs.diff [ 12633921 ]
          Hide
          ASF subversion and git services added a comment -

          Commit 1576383 from Rick Hillegas in branch 'code/trunk'
          [ https://svn.apache.org/r1576383 ]

          DERBY-3155: Add tests for largish lobs in MERGE statements; commit derby-3155-40-aa-bigLobs.diff.

          Show
          ASF subversion and git services added a comment - Commit 1576383 from Rick Hillegas in branch 'code/trunk' [ https://svn.apache.org/r1576383 ] DERBY-3155 : Add tests for largish lobs in MERGE statements; commit derby-3155-40-aa-bigLobs.diff.
          Hide
          Mike Matrigali added a comment -

          i could not tell from quick scan of last commit what size lobs were being tested. In general I suggest good testing sizes for lobs to be bothe some tests smaller than 32k and some larger than 32k. This is the magic place where their handling changes in store and some language level caching i think. And I think 32k is magic for network server as well. If easy probably good to make it bigger than 96k to get a few pages involved.

          Show
          Mike Matrigali added a comment - i could not tell from quick scan of last commit what size lobs were being tested. In general I suggest good testing sizes for lobs to be bothe some tests smaller than 32k and some larger than 32k. This is the magic place where their handling changes in store and some language level caching i think. And I think 32k is magic for network server as well. If easy probably good to make it bigger than 96k to get a few pages involved.
          Hide
          Rick Hillegas added a comment -

          Thanks for that advice, Mike. The derby-3155-38-aa-datatypes.diff patch tested small lobs (just a handful of bytes) and the derby-3155-40-aa-bigLobs.diff patch tested lobs which were 100K bytes or larger. Thanks.

          Show
          Rick Hillegas added a comment - Thanks for that advice, Mike. The derby-3155-38-aa-datatypes.diff patch tested small lobs (just a handful of bytes) and the derby-3155-40-aa-bigLobs.diff patch tested lobs which were 100K bytes or larger. Thanks.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-41-aa-nullGeneratedColumns.diff. This patch adds a test for MERGE statements which sometimes populate generated columns with nulls and sometimes don't.

          Touches the following file:

          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java

          Show
          Rick Hillegas added a comment - Attaching derby-3155-41-aa-nullGeneratedColumns.diff. This patch adds a test for MERGE statements which sometimes populate generated columns with nulls and sometimes don't. Touches the following file: M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java
          Rick Hillegas made changes -
          Hide
          ASF subversion and git services added a comment -

          Commit 1576710 from Rick Hillegas in branch 'code/trunk'
          [ https://svn.apache.org/r1576710 ]

          DERBY-3155: Add a test for MERGE statements which sometimes put nulls into generated columns and sometimes don't; commit derby-3155-41-aa-nullGeneratedColumns.diff.

          Show
          ASF subversion and git services added a comment - Commit 1576710 from Rick Hillegas in branch 'code/trunk' [ https://svn.apache.org/r1576710 ] DERBY-3155 : Add a test for MERGE statements which sometimes put nulls into generated columns and sometimes don't; commit derby-3155-41-aa-nullGeneratedColumns.diff.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-42-aa-triggersAndGeneratedColumns.diff. This patch adds a test for triggers involving generated columns which are fired by MERGE statements.

          Touches the following file:

          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java

          Show
          Rick Hillegas added a comment - Attaching derby-3155-42-aa-triggersAndGeneratedColumns.diff. This patch adds a test for triggers involving generated columns which are fired by MERGE statements. Touches the following file: M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java
          Rick Hillegas made changes -
          Hide
          ASF subversion and git services added a comment -

          Commit 1576893 from Rick Hillegas in branch 'code/trunk'
          [ https://svn.apache.org/r1576893 ]

          DERBY-3155: Add test for MERGE statement which fires triggers involving generated columns; commit derby-3155-42-aa-triggersAndGeneratedColumns.diff.

          Show
          ASF subversion and git services added a comment - Commit 1576893 from Rick Hillegas in branch 'code/trunk' [ https://svn.apache.org/r1576893 ] DERBY-3155 : Add test for MERGE statement which fires triggers involving generated columns; commit derby-3155-42-aa-triggersAndGeneratedColumns.diff.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-43-aa-eliminateDuplicateColumnRefs.diff. This patch eliminates redundant copies of columns in the select list of the driving left join. MergeStatementTest runs cleanly with this patch.

          I tripped across this redundancy while debugging a problem involving lobs. The redundancy shouldn't have any noticeable effects for non-lob columns. But for lobs this might be a performance issue.

          The fix is to replace an ArrayList with a HashSet when computing the select list of the driving left join.

          Touches the following file:

          M java/engine/org/apache/derby/impl/sql/compile/MergeNode.java

          Show
          Rick Hillegas added a comment - Attaching derby-3155-43-aa-eliminateDuplicateColumnRefs.diff. This patch eliminates redundant copies of columns in the select list of the driving left join. MergeStatementTest runs cleanly with this patch. I tripped across this redundancy while debugging a problem involving lobs. The redundancy shouldn't have any noticeable effects for non-lob columns. But for lobs this might be a performance issue. The fix is to replace an ArrayList with a HashSet when computing the select list of the driving left join. Touches the following file: M java/engine/org/apache/derby/impl/sql/compile/MergeNode.java
          Rick Hillegas made changes -
          Hide
          ASF subversion and git services added a comment -

          Commit 1577566 from Rick Hillegas in branch 'code/trunk'
          [ https://svn.apache.org/r1577566 ]

          DERBY-3155: Eliminate redundant copies of columns in the select list of the driving left join for MERGE statements; commit derby-3155-43-aa-eliminateDuplicateColumnRefs.diff.

          Show
          ASF subversion and git services added a comment - Commit 1577566 from Rick Hillegas in branch 'code/trunk' [ https://svn.apache.org/r1577566 ] DERBY-3155 : Eliminate redundant copies of columns in the select list of the driving left join for MERGE statements; commit derby-3155-43-aa-eliminateDuplicateColumnRefs.diff.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-44-aa-lobsInTriggers.diff. This patch materializes LOBs before handing them to the WHEN [ NOT ] MATCHED clauses.

          This patch fixes a bug which surfaced when MERGE statements fired triggers which referenced LOBs. The LOBs could not be read because the scan underneath them had been closed.

          Right now the MERGE statement has 2 execution phases:

          1) Scan the driving left join and buffer up qualified rows for processing by the WHEN [ NOT ] MATCHED clauses.

          2) Then process each clause in turn, performing an INSERT/UPDATE/DELETE on the rows buffered up for it.

          Between these two steps, we close the driving left join. This means that we can't use references to LOBs inside the driving left join. Instead, we have to do our work on LOB values. This may be an inefficient way to handle LOBs, and we may want to optimize this later on. Perhaps we can leave the driving left join open. However, then I think we will run into the problems we've seen with reading the same LOB reference more than once. I do not want to fix those problems as part of implementing MERGE.

          Touches the following files:

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

          M java/engine/org/apache/derby/impl/sql/execute/MergeResultSet.java

          Materialize LOBs from their references before buffering them up for handling by the WHEN [ NOT ] MATCHED clauses.

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

          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java

          Tests for MERGE-fired triggers which reference LOBs.

          Show
          Rick Hillegas added a comment - Attaching derby-3155-44-aa-lobsInTriggers.diff. This patch materializes LOBs before handing them to the WHEN [ NOT ] MATCHED clauses. This patch fixes a bug which surfaced when MERGE statements fired triggers which referenced LOBs. The LOBs could not be read because the scan underneath them had been closed. Right now the MERGE statement has 2 execution phases: 1) Scan the driving left join and buffer up qualified rows for processing by the WHEN [ NOT ] MATCHED clauses. 2) Then process each clause in turn, performing an INSERT/UPDATE/DELETE on the rows buffered up for it. Between these two steps, we close the driving left join. This means that we can't use references to LOBs inside the driving left join. Instead, we have to do our work on LOB values. This may be an inefficient way to handle LOBs, and we may want to optimize this later on. Perhaps we can leave the driving left join open. However, then I think we will run into the problems we've seen with reading the same LOB reference more than once. I do not want to fix those problems as part of implementing MERGE. Touches the following files: ------------- M java/engine/org/apache/derby/impl/sql/execute/MergeResultSet.java Materialize LOBs from their references before buffering them up for handling by the WHEN [ NOT ] MATCHED clauses. ------------- M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java Tests for MERGE-fired triggers which reference LOBs.
          Rick Hillegas made changes -
          Attachment derby-3155-44-aa-lobsInTriggers.diff [ 12635153 ]
          Hide
          ASF subversion and git services added a comment -

          Commit 1578535 from Rick Hillegas in branch 'code/trunk'
          [ https://svn.apache.org/r1578535 ]

          DERBY-3155: Materialize LOBs before buffering them up for processing by WHEN [ NOT ] MATCHED clauses; commit derby-3155-44-aa-lobsInTriggers.diff.

          Show
          ASF subversion and git services added a comment - Commit 1578535 from Rick Hillegas in branch 'code/trunk' [ https://svn.apache.org/r1578535 ] DERBY-3155 : Materialize LOBs before buffering them up for processing by WHEN [ NOT ] MATCHED clauses; commit derby-3155-44-aa-lobsInTriggers.diff.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-45-aa-serialization.diff. This patch adds tests for the (de)serialization of MERGE statements.

          The patch adjusts the existing tests for triggers whose transition tables drive MERGE statements. Those tests are now run in 2 configurations:

          1) bouncing the database after the trigger definition but before executing the statement which fires the trigger

          2) not bouncing the database

          Touches the following file:

          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java

          Show
          Rick Hillegas added a comment - Attaching derby-3155-45-aa-serialization.diff. This patch adds tests for the (de)serialization of MERGE statements. The patch adjusts the existing tests for triggers whose transition tables drive MERGE statements. Those tests are now run in 2 configurations: 1) bouncing the database after the trigger definition but before executing the statement which fires the trigger 2) not bouncing the database Touches the following file: M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java
          Rick Hillegas made changes -
          Attachment derby-3155-45-aa-serialization.diff [ 12635325 ]
          Hide
          ASF subversion and git services added a comment -

          Commit 1578920 from Rick Hillegas in branch 'code/trunk'
          [ https://svn.apache.org/r1578920 ]

          DERBY-3155: Add tests for the (de)serialization of MERGE statements; commit derby-3155-45-aa-serialization.diff.

          Show
          ASF subversion and git services added a comment - Commit 1578920 from Rick Hillegas in branch 'code/trunk' [ https://svn.apache.org/r1578920 ] DERBY-3155 : Add tests for the (de)serialization of MERGE statements; commit derby-3155-45-aa-serialization.diff.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-46-aa-deferredDeletes.diff. This patch verifies that columns needed by triggers (but not the MERGE statement itself) are buffered up for the WHEN [ NOT ] MATCHED actions.

          Touches the following file:

          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java

          Show
          Rick Hillegas added a comment - Attaching derby-3155-46-aa-deferredDeletes.diff. This patch verifies that columns needed by triggers (but not the MERGE statement itself) are buffered up for the WHEN [ NOT ] MATCHED actions. Touches the following file: M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java
          Rick Hillegas made changes -
          Attachment derby-3155-46-aa-deferredDeletes.diff [ 12635331 ]
          Hide
          ASF subversion and git services added a comment -

          Commit 1578945 from Rick Hillegas in branch 'code/trunk'
          [ https://svn.apache.org/r1578945 ]

          DERBY-3155: Verify that columns needed for triggers are buffered up by MERGE statements, even when the MERGE statements do not mention those columns; commit derby-3155-46-aa-deferredDeletes.diff.

          Show
          ASF subversion and git services added a comment - Commit 1578945 from Rick Hillegas in branch 'code/trunk' [ https://svn.apache.org/r1578945 ] DERBY-3155 : Verify that columns needed for triggers are buffered up by MERGE statements, even when the MERGE statements do not mention those columns; commit derby-3155-46-aa-deferredDeletes.diff.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-47-aa-collations.diff. This patch runs all of the MERGE statement tests with collations turned on and with collations turned off. I will run regression tests.

          For the collation-enabled case, I tried to compose decorators in a way which would turn on both collations and sql authorization. But I couldn't figure out the right order and I gave up fiddling with it. Instead, I added some defensive code in setUp(), which turns on sql authorization if it isn't on. I have added a test case which verifies that collations are enabled when they should be.

          Touches the following file:

          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java

          Show
          Rick Hillegas added a comment - Attaching derby-3155-47-aa-collations.diff. This patch runs all of the MERGE statement tests with collations turned on and with collations turned off. I will run regression tests. For the collation-enabled case, I tried to compose decorators in a way which would turn on both collations and sql authorization. But I couldn't figure out the right order and I gave up fiddling with it. Instead, I added some defensive code in setUp(), which turns on sql authorization if it isn't on. I have added a test case which verifies that collations are enabled when they should be. Touches the following file: M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java
          Rick Hillegas made changes -
          Attachment derby-3155-47-aa-collations.diff [ 12635365 ]
          Hide
          Rick Hillegas added a comment -

          Tests passed cleanly for me on derby-3155-47-aa-collations.diff except for an instance of the Metaspace exhaustion heisenbug which we see on JDK 8 sometimes:

          There was 1 failure:
          1) testCreateTable(org.apache.derbyTesting.functionTests.tests.upgradeTests.BasicSetup)junit.framework.ComparisonFailure: Unexpected SQL state. expected:<[42Y55]> but was:<[XJ001]>
          at org.apache.derbyTesting.junit.BaseJDBCTestCase.assertSQLState(BaseJDBCTestCase.java:876)
          at org.apache.derbyTesting.junit.BaseJDBCTestCase.assertSQLState(BaseJDBCTestCase.java:940)
          at org.apache.derbyTesting.functionTests.tests.upgradeTests.BasicSetup.testCreateTable(BasicSetup.java:211)
          at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
          at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
          at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
          at org.apache.derbyTesting.junit.BaseTestCase.runBare(BaseTestCase.java:118)
          at org.apache.derbyTesting.junit.BaseJDBCTestCase.runBareOverridable(BaseJDBCTestCase.java:440)
          at org.apache.derbyTesting.junit.BaseJDBCTestCase.runBare(BaseJDBCTestCase.java:457)
          at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24)
          at junit.extensions.TestSetup$1.protect(TestSetup.java:21)
          at junit.extensions.TestSetup.run(TestSetup.java:25)
          at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24)
          at junit.extensions.TestSetup$1.protect(TestSetup.java:21)
          at junit.extensions.TestSetup.run(TestSetup.java:25)
          at org.apache.derbyTesting.junit.BaseTestSetup.run(BaseTestSetup.java:57)
          at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24)
          at junit.extensions.TestSetup$1.protect(TestSetup.java:21)
          at junit.extensions.TestSetup.run(TestSetup.java:25)
          at org.apache.derbyTesting.junit.BaseTestSetup.run(BaseTestSetup.java:57)
          at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24)
          at junit.extensions.TestSetup$1.protect(TestSetup.java:21)
          at junit.extensions.TestSetup.run(TestSetup.java:25)
          at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24)
          at junit.extensions.TestSetup$1.protect(TestSetup.java:21)
          at junit.extensions.TestSetup.run(TestSetup.java:25)
          at org.apache.derbyTesting.junit.BaseTestSetup.run(BaseTestSetup.java:57)
          at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24)
          at junit.extensions.TestSetup$1.protect(TestSetup.java:21)
          at junit.extensions.TestSetup.run(TestSetup.java:25)
          at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24)
          at junit.extensions.TestSetup$1.protect(TestSetup.java:21)
          at junit.extensions.TestSetup.run(TestSetup.java:25)
          at org.apache.derbyTesting.junit.BaseTestSetup.run(BaseTestSetup.java:57)
          at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24)
          at junit.extensions.TestSetup$1.protect(TestSetup.java:21)
          at junit.extensions.TestSetup.run(TestSetup.java:25)
          at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24)
          at junit.extensions.TestSetup$1.protect(TestSetup.java:21)
          at junit.extensions.TestSetup.run(TestSetup.java:25)
          at org.apache.derbyTesting.junit.BaseTestSetup.run(BaseTestSetup.java:57)
          at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24)
          at junit.extensions.TestSetup$1.protect(TestSetup.java:21)
          at junit.extensions.TestSetup.run(TestSetup.java:25)
          at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24)
          at junit.extensions.TestSetup$1.protect(TestSetup.java:21)
          at junit.extensions.TestSetup.run(TestSetup.java:25)
          at org.apache.derbyTesting.junit.BaseTestSetup.run(BaseTestSetup.java:57)
          at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24)
          at junit.extensions.TestSetup$1.protect(TestSetup.java:21)
          at junit.extensions.TestSetup.run(TestSetup.java:25)
          at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24)
          at junit.extensions.TestSetup$1.protect(TestSetup.java:21)
          at junit.extensions.TestSetup.run(TestSetup.java:25)
          at org.apache.derbyTesting.junit.BaseTestSetup.run(BaseTestSetup.java:57)
          at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24)
          at junit.extensions.TestSetup$1.protect(TestSetup.java:21)
          at junit.extensions.TestSetup.run(TestSetup.java:25)
          at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24)
          at junit.extensions.TestSetup$1.protect(TestSetup.java:21)
          at junit.extensions.TestSetup.run(TestSetup.java:25)
          at org.apache.derbyTesting.junit.BaseTestSetup.run(BaseTestSetup.java:57)
          at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24)
          at junit.extensions.TestSetup$1.protect(TestSetup.java:21)
          at junit.extensions.TestSetup.run(TestSetup.java:25)
          at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24)
          at junit.extensions.TestSetup$1.protect(TestSetup.java:21)
          at junit.extensions.TestSetup.run(TestSetup.java:25)
          at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24)
          at junit.extensions.TestSetup$1.protect(TestSetup.java:21)
          at junit.extensions.TestSetup.run(TestSetup.java:25)
          Caused by: java.sql.SQLException: Java exception: 'Metaspace: java.lang.OutOfMemoryError'.
          at org.apache.derby.impl.jdbc.SQLExceptionFactory40.getSQLException(Unknown Source)
          at org.apache.derby.impl.jdbc.Util.newEmbedSQLException(Unknown Source)
          at org.apache.derby.impl.jdbc.Util.javaException(Unknown Source)
          at org.apache.derby.impl.jdbc.TransactionResourceImpl.wrapInSQLException(Unknown Source)
          at org.apache.derby.impl.jdbc.TransactionResourceImpl.handleException(Unknown Source)
          at org.apache.derby.impl.jdbc.EmbedConnection.handleException(Unknown Source)
          at org.apache.derby.impl.jdbc.ConnectionChild.handleException(Unknown Source)
          at org.apache.derby.impl.jdbc.EmbedStatement.execute(Unknown Source)
          at org.apache.derby.impl.jdbc.EmbedStatement.executeUpdate(Unknown Source)
          at org.apache.derbyTesting.functionTests.tests.upgradeTests.BasicSetup.testCreateTable(BasicSetup.java:207)
          ... 110 more
          Caused by: java.sql.SQLException: Java exception: 'Metaspace: java.lang.OutOfMemoryError'.
          at org.apache.derby.impl.jdbc.SQLExceptionFactory.getSQLException(Unknown Source)
          at org.apache.derby.impl.jdbc.SQLExceptionFactory40.wrapArgsForTransportAcrossDRDA(Unknown Source)
          ... 120 more
          Caused by: java.lang.OutOfMemoryError: Metaspace
          at java.lang.ClassLoader.defineClass1(Native Method)
          at java.lang.ClassLoader.defineClass(ClassLoader.java:760)
          at java.security.SecureClassLoader.defineClass(SecureClassLoader.java:142)
          at java.net.URLClassLoader.defineClass(URLClassLoader.java:455)
          at java.net.URLClassLoader.access$100(URLClassLoader.java:73)
          at java.net.URLClassLoader$1.run(URLClassLoader.java:367)
          at java.net.URLClassLoader$1.run(URLClassLoader.java:361)
          at java.security.AccessController.doPrivileged(Native Method)
          at java.net.URLClassLoader.findClass(URLClassLoader.java:360)
          at java.lang.ClassLoader.loadClass(ClassLoader.java:424)
          at java.lang.ClassLoader.loadClass(ClassLoader.java:357)
          at org.apache.derby.impl.sql.compile.ParserImpl.getParser(Unknown Source)
          at org.apache.derby.impl.sql.compile.ParserImpl.parseStatement(Unknown Source)
          at org.apache.derby.impl.sql.GenericStatement.prepMinion(Unknown Source)
          at org.apache.derby.impl.sql.GenericStatement.prepare(Unknown Source)
          at org.apache.derby.impl.sql.conn.GenericLanguageConnectionContext.prepareInternalStatement(Unknown Source)
          at org.apache.derby.impl.jdbc.EmbedStatement.execute(Unknown Source)
          at org.apache.derby.impl.jdbc.EmbedStatement.executeUpdate(Unknown Source)
          at org.apache.derbyTesting.functionTests.tests.upgradeTests.BasicSetup.testCreateTable(BasicSetup.java:207)
          at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
          at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
          at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
          at org.apache.derbyTesting.junit.BaseTestCase.runBare(BaseTestCase.java:118)
          at org.apache.derbyTesting.junit.BaseJDBCTestCase.runBareOverridable(BaseJDBCTestCase.java:440)
          at org.apache.derbyTesting.junit.BaseJDBCTestCase.runBare(BaseJDBCTestCase.java:457)

          FAILURES!!!
          Tests run: 13717, Failures: 1, Errors: 0

          Show
          Rick Hillegas added a comment - Tests passed cleanly for me on derby-3155-47-aa-collations.diff except for an instance of the Metaspace exhaustion heisenbug which we see on JDK 8 sometimes: There was 1 failure: 1) testCreateTable(org.apache.derbyTesting.functionTests.tests.upgradeTests.BasicSetup)junit.framework.ComparisonFailure: Unexpected SQL state. expected:< [42Y55] > but was:< [XJ001] > at org.apache.derbyTesting.junit.BaseJDBCTestCase.assertSQLState(BaseJDBCTestCase.java:876) at org.apache.derbyTesting.junit.BaseJDBCTestCase.assertSQLState(BaseJDBCTestCase.java:940) at org.apache.derbyTesting.functionTests.tests.upgradeTests.BasicSetup.testCreateTable(BasicSetup.java:211) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at org.apache.derbyTesting.junit.BaseTestCase.runBare(BaseTestCase.java:118) at org.apache.derbyTesting.junit.BaseJDBCTestCase.runBareOverridable(BaseJDBCTestCase.java:440) at org.apache.derbyTesting.junit.BaseJDBCTestCase.runBare(BaseJDBCTestCase.java:457) at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24) at junit.extensions.TestSetup$1.protect(TestSetup.java:21) at junit.extensions.TestSetup.run(TestSetup.java:25) at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24) at junit.extensions.TestSetup$1.protect(TestSetup.java:21) at junit.extensions.TestSetup.run(TestSetup.java:25) at org.apache.derbyTesting.junit.BaseTestSetup.run(BaseTestSetup.java:57) at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24) at junit.extensions.TestSetup$1.protect(TestSetup.java:21) at junit.extensions.TestSetup.run(TestSetup.java:25) at org.apache.derbyTesting.junit.BaseTestSetup.run(BaseTestSetup.java:57) at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24) at junit.extensions.TestSetup$1.protect(TestSetup.java:21) at junit.extensions.TestSetup.run(TestSetup.java:25) at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24) at junit.extensions.TestSetup$1.protect(TestSetup.java:21) at junit.extensions.TestSetup.run(TestSetup.java:25) at org.apache.derbyTesting.junit.BaseTestSetup.run(BaseTestSetup.java:57) at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24) at junit.extensions.TestSetup$1.protect(TestSetup.java:21) at junit.extensions.TestSetup.run(TestSetup.java:25) at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24) at junit.extensions.TestSetup$1.protect(TestSetup.java:21) at junit.extensions.TestSetup.run(TestSetup.java:25) at org.apache.derbyTesting.junit.BaseTestSetup.run(BaseTestSetup.java:57) at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24) at junit.extensions.TestSetup$1.protect(TestSetup.java:21) at junit.extensions.TestSetup.run(TestSetup.java:25) at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24) at junit.extensions.TestSetup$1.protect(TestSetup.java:21) at junit.extensions.TestSetup.run(TestSetup.java:25) at org.apache.derbyTesting.junit.BaseTestSetup.run(BaseTestSetup.java:57) at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24) at junit.extensions.TestSetup$1.protect(TestSetup.java:21) at junit.extensions.TestSetup.run(TestSetup.java:25) at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24) at junit.extensions.TestSetup$1.protect(TestSetup.java:21) at junit.extensions.TestSetup.run(TestSetup.java:25) at org.apache.derbyTesting.junit.BaseTestSetup.run(BaseTestSetup.java:57) at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24) at junit.extensions.TestSetup$1.protect(TestSetup.java:21) at junit.extensions.TestSetup.run(TestSetup.java:25) at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24) at junit.extensions.TestSetup$1.protect(TestSetup.java:21) at junit.extensions.TestSetup.run(TestSetup.java:25) at org.apache.derbyTesting.junit.BaseTestSetup.run(BaseTestSetup.java:57) at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24) at junit.extensions.TestSetup$1.protect(TestSetup.java:21) at junit.extensions.TestSetup.run(TestSetup.java:25) at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24) at junit.extensions.TestSetup$1.protect(TestSetup.java:21) at junit.extensions.TestSetup.run(TestSetup.java:25) at org.apache.derbyTesting.junit.BaseTestSetup.run(BaseTestSetup.java:57) at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24) at junit.extensions.TestSetup$1.protect(TestSetup.java:21) at junit.extensions.TestSetup.run(TestSetup.java:25) at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24) at junit.extensions.TestSetup$1.protect(TestSetup.java:21) at junit.extensions.TestSetup.run(TestSetup.java:25) at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24) at junit.extensions.TestSetup$1.protect(TestSetup.java:21) at junit.extensions.TestSetup.run(TestSetup.java:25) Caused by: java.sql.SQLException: Java exception: 'Metaspace: java.lang.OutOfMemoryError'. at org.apache.derby.impl.jdbc.SQLExceptionFactory40.getSQLException(Unknown Source) at org.apache.derby.impl.jdbc.Util.newEmbedSQLException(Unknown Source) at org.apache.derby.impl.jdbc.Util.javaException(Unknown Source) at org.apache.derby.impl.jdbc.TransactionResourceImpl.wrapInSQLException(Unknown Source) at org.apache.derby.impl.jdbc.TransactionResourceImpl.handleException(Unknown Source) at org.apache.derby.impl.jdbc.EmbedConnection.handleException(Unknown Source) at org.apache.derby.impl.jdbc.ConnectionChild.handleException(Unknown Source) at org.apache.derby.impl.jdbc.EmbedStatement.execute(Unknown Source) at org.apache.derby.impl.jdbc.EmbedStatement.executeUpdate(Unknown Source) at org.apache.derbyTesting.functionTests.tests.upgradeTests.BasicSetup.testCreateTable(BasicSetup.java:207) ... 110 more Caused by: java.sql.SQLException: Java exception: 'Metaspace: java.lang.OutOfMemoryError'. at org.apache.derby.impl.jdbc.SQLExceptionFactory.getSQLException(Unknown Source) at org.apache.derby.impl.jdbc.SQLExceptionFactory40.wrapArgsForTransportAcrossDRDA(Unknown Source) ... 120 more Caused by: java.lang.OutOfMemoryError: Metaspace at java.lang.ClassLoader.defineClass1(Native Method) at java.lang.ClassLoader.defineClass(ClassLoader.java:760) at java.security.SecureClassLoader.defineClass(SecureClassLoader.java:142) at java.net.URLClassLoader.defineClass(URLClassLoader.java:455) at java.net.URLClassLoader.access$100(URLClassLoader.java:73) at java.net.URLClassLoader$1.run(URLClassLoader.java:367) at java.net.URLClassLoader$1.run(URLClassLoader.java:361) at java.security.AccessController.doPrivileged(Native Method) at java.net.URLClassLoader.findClass(URLClassLoader.java:360) at java.lang.ClassLoader.loadClass(ClassLoader.java:424) at java.lang.ClassLoader.loadClass(ClassLoader.java:357) at org.apache.derby.impl.sql.compile.ParserImpl.getParser(Unknown Source) at org.apache.derby.impl.sql.compile.ParserImpl.parseStatement(Unknown Source) at org.apache.derby.impl.sql.GenericStatement.prepMinion(Unknown Source) at org.apache.derby.impl.sql.GenericStatement.prepare(Unknown Source) at org.apache.derby.impl.sql.conn.GenericLanguageConnectionContext.prepareInternalStatement(Unknown Source) at org.apache.derby.impl.jdbc.EmbedStatement.execute(Unknown Source) at org.apache.derby.impl.jdbc.EmbedStatement.executeUpdate(Unknown Source) at org.apache.derbyTesting.functionTests.tests.upgradeTests.BasicSetup.testCreateTable(BasicSetup.java:207) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at org.apache.derbyTesting.junit.BaseTestCase.runBare(BaseTestCase.java:118) at org.apache.derbyTesting.junit.BaseJDBCTestCase.runBareOverridable(BaseJDBCTestCase.java:440) at org.apache.derbyTesting.junit.BaseJDBCTestCase.runBare(BaseJDBCTestCase.java:457) FAILURES!!! Tests run: 13717, Failures: 1, Errors: 0
          Hide
          ASF subversion and git services added a comment -

          Commit 1579040 from Rick Hillegas in branch 'code/trunk'
          [ https://svn.apache.org/r1579040 ]

          DERBY-3155: Run all MERGE tests with and without collations; commit derby-3155-47-aa-collations.diff.

          Show
          ASF subversion and git services added a comment - Commit 1579040 from Rick Hillegas in branch 'code/trunk' [ https://svn.apache.org/r1579040 ] DERBY-3155 : Run all MERGE tests with and without collations; commit derby-3155-47-aa-collations.diff.
          Rick Hillegas made changes -
          Link This issue is related to DERBY-6522 [ DERBY-6522 ]
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-48-aa-indexScan.diff. This patch adds a test to verify that MERGE works when an index scan is chosen for the target table and the row location has to come out of the index row.

          Touches the following file:

          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java

          Show
          Rick Hillegas added a comment - Attaching derby-3155-48-aa-indexScan.diff. This patch adds a test to verify that MERGE works when an index scan is chosen for the target table and the row location has to come out of the index row. Touches the following file: M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java
          Rick Hillegas made changes -
          Attachment derby-3155-48-aa-indexScan.diff [ 12635818 ]
          Hide
          ASF subversion and git services added a comment -

          Commit 1579685 from Rick Hillegas in branch 'code/trunk'
          [ https://svn.apache.org/r1579685 ]

          DERBY-3155: Add test for MERGE statement which reads the target table via an index; commit derby-3155-48-aa-indexScan.diff.

          Show
          ASF subversion and git services added a comment - Commit 1579685 from Rick Hillegas in branch 'code/trunk' [ https://svn.apache.org/r1579685 ] DERBY-3155 : Add test for MERGE statement which reads the target table via an index; commit derby-3155-48-aa-indexScan.diff.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-49-aa-cleanup1.diff. This is a minor cleanup of the bind-time and execute-time code for the MERGE statement. The goal is to make this code more readable.

          1) Renames some variables and methods in order to clarify their usage.

          2) Groups closely related methods together under banner comments.

          3) Removes some dead code.

          Touches the following files:

          M java/engine/org/apache/derby/impl/sql/compile/MergeNode.java
          M java/engine/org/apache/derby/impl/sql/compile/MatchingClauseNode.java
          M java/engine/org/apache/derby/impl/sql/execute/MatchingClauseConstantAction.java

          Show
          Rick Hillegas added a comment - Attaching derby-3155-49-aa-cleanup1.diff. This is a minor cleanup of the bind-time and execute-time code for the MERGE statement. The goal is to make this code more readable. 1) Renames some variables and methods in order to clarify their usage. 2) Groups closely related methods together under banner comments. 3) Removes some dead code. Touches the following files: M java/engine/org/apache/derby/impl/sql/compile/MergeNode.java M java/engine/org/apache/derby/impl/sql/compile/MatchingClauseNode.java M java/engine/org/apache/derby/impl/sql/execute/MatchingClauseConstantAction.java
          Rick Hillegas made changes -
          Attachment derby-3155-49-aa-cleanup1.diff [ 12635853 ]
          Hide
          ASF subversion and git services added a comment -

          Commit 1579725 from Rick Hillegas in branch 'code/trunk'
          [ https://svn.apache.org/r1579725 ]

          DERBY-3155: Cleanup MERGE statement in order to make the code more readable; commit derby-3155-49-aa-cleanup1.diff.

          Show
          ASF subversion and git services added a comment - Commit 1579725 from Rick Hillegas in branch 'code/trunk' [ https://svn.apache.org/r1579725 ] DERBY-3155 : Cleanup MERGE statement in order to make the code more readable; commit derby-3155-49-aa-cleanup1.diff.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-50-aa-revampDeleteThenRows.diff. This patch removes some special handling of DELETE actions.

          DELETE was the first action I coded. The rows which are passed to DELETE actions are simpler than the rows which are passed to INSERT and UPDATE actions. That is because a DELETE action just needs columns from the target table. But INSERT may need complicated expressions built out of source row columns and UPDATE may need even more complicated expressions built out of columns from both the target and source tables.

          When I coded the DELETE action, I just built some simple machinery to copy columns from the driving left join. But I had to bite the bullet when I coded INSERT and UPDATE actions. Then I had to add code to generate the complex expressions. The copying (for DELETEs) is now redundant since it is a special case of the more complicated expression evaluation. I have removed the special copying done for DELETEs. Now DELETE actions use the same expression evaluator logic as INSERT and UPDATE.

          This simplifies and clarifies the code.

          Touches the following files:

          M java/engine/org/apache/derby/impl/sql/compile/MergeNode.java
          M java/engine/org/apache/derby/impl/sql/compile/MatchingClauseNode.java
          M java/engine/org/apache/derby/impl/sql/execute/MatchingClauseConstantAction.java
          M java/engine/org/apache/derby/impl/sql/execute/GenericConstantActionFactory.java

          Show
          Rick Hillegas added a comment - Attaching derby-3155-50-aa-revampDeleteThenRows.diff. This patch removes some special handling of DELETE actions. DELETE was the first action I coded. The rows which are passed to DELETE actions are simpler than the rows which are passed to INSERT and UPDATE actions. That is because a DELETE action just needs columns from the target table. But INSERT may need complicated expressions built out of source row columns and UPDATE may need even more complicated expressions built out of columns from both the target and source tables. When I coded the DELETE action, I just built some simple machinery to copy columns from the driving left join. But I had to bite the bullet when I coded INSERT and UPDATE actions. Then I had to add code to generate the complex expressions. The copying (for DELETEs) is now redundant since it is a special case of the more complicated expression evaluation. I have removed the special copying done for DELETEs. Now DELETE actions use the same expression evaluator logic as INSERT and UPDATE. This simplifies and clarifies the code. Touches the following files: M java/engine/org/apache/derby/impl/sql/compile/MergeNode.java M java/engine/org/apache/derby/impl/sql/compile/MatchingClauseNode.java M java/engine/org/apache/derby/impl/sql/execute/MatchingClauseConstantAction.java M java/engine/org/apache/derby/impl/sql/execute/GenericConstantActionFactory.java
          Rick Hillegas made changes -
          Hide
          ASF subversion and git services added a comment -

          Commit 1579937 from Rick Hillegas in branch 'code/trunk'
          [ https://svn.apache.org/r1579937 ]

          DERBY-3155: Simplify processing of then rows for the DELETE actions of MERGE statements; commit derby-3155-50-aa-revampDeleteThenRows.diff.

          Show
          ASF subversion and git services added a comment - Commit 1579937 from Rick Hillegas in branch 'code/trunk' [ https://svn.apache.org/r1579937 ] DERBY-3155 : Simplify processing of then rows for the DELETE actions of MERGE statements; commit derby-3155-50-aa-revampDeleteThenRows.diff.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-51-aa-cleanup2.diff. This patch adds more comments to the compiler code for the MERGE statement.

          At this point, I would welcome a desk-check by anyone who has spare cycles. Your questions and comments may discover actual bugs, suggest parts of the code which need more testing, and generally help me improve the readability of this code. The major classes to read are:

          o org.apache.derby.impl.sql.compile.MergeNode
          o org.apache.derby.impl.sql.compile.MatchingClauseNode
          o org.apache.derby.impl.sql.execute.MergeResultSet
          o org.apache.derby.impl.sql.execute.MatchingClauseConstantAction

          In reading the code, I would suggest the following approach:

          1) First read the header comment in MergeNode. That header comment lays out the approach to implementing MERGE.

          2) Then read the execution code: MergeResultSet and MatchingClauseConstantAction.

          3) Then read MergeNode itself.

          4) Finish up with MatchingClauseNode.

          The following principles guided the design:

          i) Keep the execution code as simple as possible.

          ii) Boost run-time performance by avoiding unnecessary expression evaluation.

          iii) Re-use the existing INSERT/UPDATE/DELETE machinery wherever possible.

          At this point, I think that the buggiest, most complex, and most brittle code is in the compiler, concentrated in the following areas:

          A) Name resolution. The MergeNode header comment explains why this is so tricky.

          B) The compilation of expressions for INSERT and UPDATE values. This is exceedingly complex in order to handle all sorts of special cases arising from identity, generated, and default columns as well as columns involved in constraints. Debugging this was difficult. However, I have not seen any problems in this area for a long time.

          Thanks!

          Touches the following files:

          M java/engine/org/apache/derby/impl/sql/compile/MergeNode.java
          M java/engine/org/apache/derby/impl/sql/compile/MatchingClauseNode.java

          Show
          Rick Hillegas added a comment - Attaching derby-3155-51-aa-cleanup2.diff. This patch adds more comments to the compiler code for the MERGE statement. At this point, I would welcome a desk-check by anyone who has spare cycles. Your questions and comments may discover actual bugs, suggest parts of the code which need more testing, and generally help me improve the readability of this code. The major classes to read are: o org.apache.derby.impl.sql.compile.MergeNode o org.apache.derby.impl.sql.compile.MatchingClauseNode o org.apache.derby.impl.sql.execute.MergeResultSet o org.apache.derby.impl.sql.execute.MatchingClauseConstantAction In reading the code, I would suggest the following approach: 1) First read the header comment in MergeNode. That header comment lays out the approach to implementing MERGE. 2) Then read the execution code: MergeResultSet and MatchingClauseConstantAction. 3) Then read MergeNode itself. 4) Finish up with MatchingClauseNode. The following principles guided the design: i) Keep the execution code as simple as possible. ii) Boost run-time performance by avoiding unnecessary expression evaluation. iii) Re-use the existing INSERT/UPDATE/DELETE machinery wherever possible. At this point, I think that the buggiest, most complex, and most brittle code is in the compiler, concentrated in the following areas: A) Name resolution. The MergeNode header comment explains why this is so tricky. B) The compilation of expressions for INSERT and UPDATE values. This is exceedingly complex in order to handle all sorts of special cases arising from identity, generated, and default columns as well as columns involved in constraints. Debugging this was difficult. However, I have not seen any problems in this area for a long time. Thanks! Touches the following files: M java/engine/org/apache/derby/impl/sql/compile/MergeNode.java M java/engine/org/apache/derby/impl/sql/compile/MatchingClauseNode.java
          Rick Hillegas made changes -
          Attachment derby-3155-51-aa-cleanup2.diff [ 12636030 ]
          Hide
          ASF subversion and git services added a comment -

          Commit 1579950 from Rick Hillegas in branch 'code/trunk'
          [ https://svn.apache.org/r1579950 ]

          DERBY-3155: Improve comments for compilation of MERGE statements; commit derby-3155-51-aa-cleanup2.diff.

          Show
          ASF subversion and git services added a comment - Commit 1579950 from Rick Hillegas in branch 'code/trunk' [ https://svn.apache.org/r1579950 ] DERBY-3155 : Improve comments for compilation of MERGE statements; commit derby-3155-51-aa-cleanup2.diff.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-52-aa-upgrade.diff. This patch prevents you from using the MERGE statement in soft-upgraded databases.

          Triggers invoking MERGE statements would fail on soft-downgrade. So we forbid MERGE in soft-upgraded databases.

          Touches the following files:

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

          M java/engine/org/apache/derby/impl/sql/compile/sqlgrammar.jj

          Forbid MERGE statement unless the database is at level 10.11 or higher.

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

          M java/testing/org/apache/derbyTesting/functionTests/tests/upgradeTests/Changes10_11.java

          Verify that MERGE is not allowed when you soft-upgrade to 10.11.

          Show
          Rick Hillegas added a comment - Attaching derby-3155-52-aa-upgrade.diff. This patch prevents you from using the MERGE statement in soft-upgraded databases. Triggers invoking MERGE statements would fail on soft-downgrade. So we forbid MERGE in soft-upgraded databases. Touches the following files: -------------- M java/engine/org/apache/derby/impl/sql/compile/sqlgrammar.jj Forbid MERGE statement unless the database is at level 10.11 or higher. -------------- M java/testing/org/apache/derbyTesting/functionTests/tests/upgradeTests/Changes10_11.java Verify that MERGE is not allowed when you soft-upgrade to 10.11.
          Rick Hillegas made changes -
          Attachment derby-3155-52-aa-upgrade.diff [ 12636354 ]
          Hide
          ASF subversion and git services added a comment -

          Commit 1580889 from Rick Hillegas in branch 'code/trunk'
          [ https://svn.apache.org/r1580889 ]

          DERBY-3155: Forbid MERGE statements in soft-upgraded databases; commit derby-3155-52-aa-upgrade.diff.

          Show
          ASF subversion and git services added a comment - Commit 1580889 from Rick Hillegas in branch 'code/trunk' [ https://svn.apache.org/r1580889 ] DERBY-3155 : Forbid MERGE statements in soft-upgraded databases; commit derby-3155-52-aa-upgrade.diff.
          Hide
          Rick Hillegas added a comment -

          Attaching 5th draft of the functional spec. This draft notes that MERGE requires hard-upgrading to 10.11.

          Show
          Rick Hillegas added a comment - Attaching 5th draft of the functional spec. This draft notes that MERGE requires hard-upgrading to 10.11.
          Rick Hillegas made changes -
          Attachment MergeStatement.html [ 12636391 ]
          Rick Hillegas made changes -
          Link This issue is related to DERBY-6526 [ DERBY-6526 ]
          Hide
          Knut Anders Hatlen added a comment -

          I came across this, which looks like a bug to me:

          connect 'jdbc:derby:memory:db;create=true';
          
          create table t1(x int);
          create table t2(y int);
          
          create trigger tr after insert on t1 referencing new table as new
          merge into t2 using new on x = y when matched then update set y = x;
          

          Results in:

          ERROR 42X04: Column 'NEW.Y' is either not in any table in the FROM list or appears within a join specification and is outside the scope of the join specification or appears in a HAVING clause and is not in the GROUP BY list. If this is a CREATE or ALTER TABLE  statement then 'NEW.Y' is not a column in the target table.
          

          The message is correct, in the sense that there is no column Y in the NEW transition table. But the trigger action never references column Y in the transition table, so I don't think it should fail.

          It fails the same way even if all the columns are qualified with table names:

          ij> create trigger tr after insert on t1 referencing new table as new
          merge into t2 using new on new.x = t2.y when matched then update set t2.y = new.x;
          ERROR 42X04: Column 'NEW.Y' is either not in any table in the FROM list or appears within a join specification and is outside the scope of the join specification or appears in a HAVING clause and is not in the GROUP BY list. If this is a CREATE or ALTER TABLE  statement then 'NEW.Y' is not a column in the target table.
          

          NEW.X and T2.Y are the only column references, yet it complains about some reference to NEW.Y.

          Show
          Knut Anders Hatlen added a comment - I came across this, which looks like a bug to me: connect 'jdbc:derby:memory:db;create=true'; create table t1(x int); create table t2(y int); create trigger tr after insert on t1 referencing new table as new merge into t2 using new on x = y when matched then update set y = x; Results in: ERROR 42X04: Column 'NEW.Y' is either not in any table in the FROM list or appears within a join specification and is outside the scope of the join specification or appears in a HAVING clause and is not in the GROUP BY list. If this is a CREATE or ALTER TABLE statement then 'NEW.Y' is not a column in the target table. The message is correct, in the sense that there is no column Y in the NEW transition table. But the trigger action never references column Y in the transition table, so I don't think it should fail. It fails the same way even if all the columns are qualified with table names: ij> create trigger tr after insert on t1 referencing new table as new merge into t2 using new on new.x = t2.y when matched then update set t2.y = new.x; ERROR 42X04: Column 'NEW.Y' is either not in any table in the FROM list or appears within a join specification and is outside the scope of the join specification or appears in a HAVING clause and is not in the GROUP BY list. If this is a CREATE or ALTER TABLE statement then 'NEW.Y' is not a column in the target table. NEW.X and T2.Y are the only column references, yet it complains about some reference to NEW.Y.
          Hide
          Rick Hillegas added a comment -

          Thanks for finding this bug, Knut. What distinguishes this case from the case tested in MergeStatementTest is the use of a column from the transition table in the result expression for the column being changed by the MERGE statement. The trigger definition succeeds if I change "set y = x" to "set y = 2 * y":

          connect 'jdbc:derby:memory:db;create=true';
          
          create table t1(x int);
          create table t2(y int);
          
          -- fails
          create trigger tr1 after insert on t1
          referencing new table as new
          merge into t2
          using new on x = y
          when matched then update set y = x;
          
          -- succeeds
          create trigger tr2 after insert on t1
          referencing new table as new
          merge into t2
          using new on x = y
          when matched then update set y = 2 * y;
          
          Show
          Rick Hillegas added a comment - Thanks for finding this bug, Knut. What distinguishes this case from the case tested in MergeStatementTest is the use of a column from the transition table in the result expression for the column being changed by the MERGE statement. The trigger definition succeeds if I change "set y = x" to "set y = 2 * y": connect 'jdbc:derby:memory:db;create=true'; create table t1(x int); create table t2(y int); -- fails create trigger tr1 after insert on t1 referencing new table as new merge into t2 using new on x = y when matched then update set y = x; -- succeeds create trigger tr2 after insert on t1 referencing new table as new merge into t2 using new on x = y when matched then update set y = 2 * y;
          Hide
          Rick Hillegas added a comment -

          Attaching derby-3155-53-aa-transitionSimpleColumn.diff. This patch addresses the problem Knut just found. I am running regression tests now.

          The problem arises because of the special logic which was put into UpdateNode with revision 418933 as part of the work on DERBY-1043. That logic was put into UpdateNode to handle another issue with triggers. That logic is supposed to null out the table name in each column on the left side of a SET clause. It's a creepy thing to do, and that logic has caused a lot of grief for the UPDATE actions of MERGE statements. That logic breaks if what is on the right side of the SET clause is a column from another table. The logic does not break for the existing MergeStatementTest.test_018_updateFromTriggerTransitionTables() test case. That is because, for that test case, what's on the right side of the SET clause isn't just a column, it's an expression. Without this current patch the following trigger definition works:

          create trigger tr2 after insert on t1
          referencing new table as new
          merge into t2
          using new on x = y
          when matched then update set y = 2 * x;
          

          Why does this not break outside a MERGE statement? Because MERGE gives rise to the only situation in which a plain column reference on the right side of a SET clause can be a column from a table other than the one being updated.

          The fix is to ignore the nulling-out of table names if we are compiling an UPDATE action of a MERGE statement. I think this should be ok because the MERGE statement already has substantial logic to correct for the effects of DERBY-1043 and should work regardless of whether the table names are nulled out. But I'm not promising that there are no edge cases on this edge case.

          Touches the following files:

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

          M java/engine/org/apache/derby/impl/sql/compile/UpdateNode.java
          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java

          Show
          Rick Hillegas added a comment - Attaching derby-3155-53-aa-transitionSimpleColumn.diff. This patch addresses the problem Knut just found. I am running regression tests now. The problem arises because of the special logic which was put into UpdateNode with revision 418933 as part of the work on DERBY-1043 . That logic was put into UpdateNode to handle another issue with triggers. That logic is supposed to null out the table name in each column on the left side of a SET clause. It's a creepy thing to do, and that logic has caused a lot of grief for the UPDATE actions of MERGE statements. That logic breaks if what is on the right side of the SET clause is a column from another table. The logic does not break for the existing MergeStatementTest.test_018_updateFromTriggerTransitionTables() test case. That is because, for that test case, what's on the right side of the SET clause isn't just a column, it's an expression. Without this current patch the following trigger definition works: create trigger tr2 after insert on t1 referencing new table as new merge into t2 using new on x = y when matched then update set y = 2 * x; Why does this not break outside a MERGE statement? Because MERGE gives rise to the only situation in which a plain column reference on the right side of a SET clause can be a column from a table other than the one being updated. The fix is to ignore the nulling-out of table names if we are compiling an UPDATE action of a MERGE statement. I think this should be ok because the MERGE statement already has substantial logic to correct for the effects of DERBY-1043 and should work regardless of whether the table names are nulled out. But I'm not promising that there are no edge cases on this edge case. Touches the following files: ------------------ M java/engine/org/apache/derby/impl/sql/compile/UpdateNode.java M java/testing/org/apache/derbyTesting/functionTests/tests/lang/MergeStatementTest.java
          Rick Hillegas made changes -
          Hide
          Rick Hillegas added a comment -

          For some reason, the metaspace exhaustion bug in Java 8 has finally surfaced on my Mac. It has taken me a while to get clean test results. However, I do get clean results on derby-3155-53-aa-transitionSimpleColumn.diff if I add the following switch to the JUnit test run:

          -XX:MaxMetaspaceFreeRatio=100

          Show
          Rick Hillegas added a comment - For some reason, the metaspace exhaustion bug in Java 8 has finally surfaced on my Mac. It has taken me a while to get clean test results. However, I do get clean results on derby-3155-53-aa-transitionSimpleColumn.diff if I add the following switch to the JUnit test run: -XX:MaxMetaspaceFreeRatio=100
          Hide
          ASF subversion and git services added a comment -

          Commit 1587317 from Rick Hillegas in branch 'code/trunk'
          [ https://svn.apache.org/r1587317 ]

          DERBY-3155: Fix bug in MERGE statement fired by a trigger; commit derby-3155-53-aa-transitionSimpleColumn.diff.

          Show
          ASF subversion and git services added a comment - Commit 1587317 from Rick Hillegas in branch 'code/trunk' [ https://svn.apache.org/r1587317 ] DERBY-3155 : Fix bug in MERGE statement fired by a trigger; commit derby-3155-53-aa-transitionSimpleColumn.diff.
          Rick Hillegas made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]
          Rick Hillegas made changes -
          Fix Version/s 10.11.0.0 [ 12324243 ]
          Rick Hillegas made changes -
          Link This issue is related to DERBY-6652 [ DERBY-6652 ]
          Rick Hillegas made changes -
          Link This issue is related to DERBY-6656 [ DERBY-6656 ]

            People

            • Assignee:
              Rick Hillegas
              Reporter:
              Trejkaz
            • Votes:
              8 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development