Derby
  1. Derby
  2. DERBY-4230

DatabaseMetaData.getColumns() returns extra column from view with group by and expression in SELECT list

    Details

    • Issue & fix info:
      High Value Fix, Patch Available, Release Note Needed
    • Bug behavior facts:
      Regression

      Description

      DatabaseMetaData.getColumns() returns an extra column for a view with a group by and an expression in the select list. I will attach the reproduction. Run the script create.sql and then the program ViewTest.

      This is a regression in version 10.3, It ran ok on latest on the 10.1 and 10.2 branches.

      The ResultSetMetaData appears to return the correct number of columns when you select from the view, but it would be nice to add a regression test for that too.

      See discussion on derby-dev.

      http://www.nabble.com/extra-column-in-DatabaseMetaData.getColumns()-with-group-by-in-view-td23545576.html

      1. create.sql
        0.3 kB
        Kathey Marsden
      2. DERBY-4230_diff.txt
        4 kB
        Kathey Marsden
      3. derby-4230_diff2.txt
        1 kB
        Kathey Marsden
      4. DERBY-4230_preview_diff.txt
        1.0 kB
        Kathey Marsden
      5. releaseNote.html
        4 kB
        Kathey Marsden
      6. ViewTest.java
        1 kB
        Kathey Marsden

        Issue Links

          Activity

          Kathey Marsden created issue -
          Kathey Marsden made changes -
          Field Original Value New Value
          Attachment ViewTest.java [ 12408178 ]
          Attachment create.sql [ 12408179 ]
          Hide
          Kathey Marsden added a comment -

          One thing to note that the problem seems to occur on view creation, so SYS.SYSCOLUMS is incorrect. This means
          1) If the view was originally created with version 10.2 or lower, the bug won't manifest itself after soft or hard upgrade to 10.3 or higher.
          2) I think when we fix this issue, users who created their views with 10.3 or higher will have to drop and recreate the view to see the fix.

          Show
          Kathey Marsden added a comment - One thing to note that the problem seems to occur on view creation, so SYS.SYSCOLUMS is incorrect. This means 1) If the view was originally created with version 10.2 or lower, the bug won't manifest itself after soft or hard upgrade to 10.3 or higher. 2) I think when we fix this issue, users who created their views with 10.3 or higher will have to drop and recreate the view to see the fix.
          Kathey Marsden made changes -
          Link This issue is related to DERBY-681 [ DERBY-681 ]
          Hide
          Kathey Marsden added a comment -

          This problem was introduced with the fix for DERBY-681 (svn revision 516454)

          Show
          Kathey Marsden added a comment - This problem was introduced with the fix for DERBY-681 (svn revision 516454)
          Kathey Marsden made changes -
          Assignee Kathey Marsden [ kmarsden ]
          Hide
          Kathey Marsden added a comment -

          Here is my initial attempt at a fix for this issue. DERBY-4230_preview_diff.txt. This is not for commit as I still need to add a regression test and run tests. I just thought I would put it out there to make sure I am on the right track.

          The code that defines the columns for insert in to SYS.SYSCOLUMNS was including the generated columns. I changed it so that it no longer includes the generated columns and the test case passes. Are the SYS.SYSCOLUMN entries for views only used for DatabaseMetaData.getColumns() or is there some other use that might need the generated columns to be there?

          Thanks

          Kathey

          Show
          Kathey Marsden added a comment - Here is my initial attempt at a fix for this issue. DERBY-4230 _preview_diff.txt. This is not for commit as I still need to add a regression test and run tests. I just thought I would put it out there to make sure I am on the right track. The code that defines the columns for insert in to SYS.SYSCOLUMNS was including the generated columns. I changed it so that it no longer includes the generated columns and the test case passes. Are the SYS.SYSCOLUMN entries for views only used for DatabaseMetaData.getColumns() or is there some other use that might need the generated columns to be there? Thanks Kathey
          Kathey Marsden made changes -
          Attachment DERBY-4230_preview_diff.txt [ 12408508 ]
          Hide
          Kathey Marsden added a comment -

          Attached is a patch for this issue (DERBY-4230_diff.txt) It is the same as the preview patch, but also has a test added. I ran suites.All and derbyall which passed except for known intermittent issues.

          Please review!

          Show
          Kathey Marsden added a comment - Attached is a patch for this issue ( DERBY-4230 _diff.txt) It is the same as the preview patch, but also has a test added. I ran suites.All and derbyall which passed except for known intermittent issues. Please review!
          Kathey Marsden made changes -
          Attachment DERBY-4230_diff.txt [ 12408545 ]
          Hide
          Kathey Marsden added a comment -

          Marking patch available and release note needed since users will have to drop and recreate their impacted view if it was created with a version of Derby with the bug. I will attach a release note soon.

          Show
          Kathey Marsden added a comment - Marking patch available and release note needed since users will have to drop and recreate their impacted view if it was created with a version of Derby with the bug. I will attach a release note soon.
          Kathey Marsden made changes -
          Derby Info [Regression] [Patch Available, Regression, Release Note Needed]
          Hide
          Knut Anders Hatlen added a comment -

          The changes look reasonable to me. I know other regressions caused by DERBY-681 have been fixed in a similar way (size() -> visibleSize()).

          I wonder if it would be better in genColumnInfos() to use rcl.visibleSize() (or perhaps colInfos.length) as stop condition for the loop and skip the rc.isGenerated test. It's simpler and then this code should automatically work if we later allow ORDER BY in CREATE VIEW.

          Show
          Knut Anders Hatlen added a comment - The changes look reasonable to me. I know other regressions caused by DERBY-681 have been fixed in a similar way (size() -> visibleSize()). I wonder if it would be better in genColumnInfos() to use rcl.visibleSize() (or perhaps colInfos.length) as stop condition for the loop and skip the rc.isGenerated test. It's simpler and then this code should automatically work if we later allow ORDER BY in CREATE VIEW.
          Hide
          Kathey Marsden added a comment -

          Thanks Knut for the review. You said:
          >I wonder if it would be better in genColumnInfos() to use rcl.visibleSize() (or >perhaps colInfos.length) as stop condition for the loop and skip the >rc.isGenerated test.

          Are the generated columns always guaranteed to come at the end? If so I will make this change and post a new patch.

          Show
          Kathey Marsden added a comment - Thanks Knut for the review. You said: >I wonder if it would be better in genColumnInfos() to use rcl.visibleSize() (or >perhaps colInfos.length) as stop condition for the loop and skip the >rc.isGenerated test. Are the generated columns always guaranteed to come at the end? If so I will make this change and post a new patch.
          Hide
          Knut Anders Hatlen added a comment -

          Hmm... I thought all visible columns were guaranteed to come before the non-visible ones, but looking at the comments in the code I can only find this explicitly stated for order by columns and not for group by columns. A quick usage search for visibleSize() indicates that some of the callers have this as an assumption, though.

          Show
          Knut Anders Hatlen added a comment - Hmm... I thought all visible columns were guaranteed to come before the non-visible ones, but looking at the comments in the code I can only find this explicitly stated for order by columns and not for group by columns. A quick usage search for visibleSize() indicates that some of the callers have this as an assumption, though.
          Hide
          Kathey Marsden added a comment -

          Anyone else have opinions on this? I am inclined to leave the change as is if we are not sure.

          Show
          Kathey Marsden added a comment - Anyone else have opinions on this? I am inclined to leave the change as is if we are not sure.
          Hide
          Kathey Marsden added a comment -

          I spoke to Army and he wasn't sure whether the generated columns always come at the end. I think I will commit the original proposed fix, (iterating over all the columns and eliminating the generated ones) to be on the safe side. I will do this later today if I hear no objections.

          Show
          Kathey Marsden added a comment - I spoke to Army and he wasn't sure whether the generated columns always come at the end. I think I will commit the original proposed fix, (iterating over all the columns and eliminating the generated ones) to be on the safe side. I will do this later today if I hear no objections.
          Hide
          Knut Anders Hatlen added a comment -

          Sounds fine. The problem with ORDER BY columns can be addressed when/if we allow ORDER BY in CREATE VIEW.

          Show
          Knut Anders Hatlen added a comment - Sounds fine. The problem with ORDER BY columns can be addressed when/if we allow ORDER BY in CREATE VIEW.
          Hide
          Kathey Marsden added a comment -

          Thanks Knut. Committed revision 777500. Is there an issue for CREATE VIEW with ORDER BY? I couldn't find it with a quick Jira search. I would like to add a comment referencing your comments in this issue regarding ORDER BY.

          I will leave this issue open while I backport to 10.5,10.4, and 10.3.

          Show
          Kathey Marsden added a comment - Thanks Knut. Committed revision 777500. Is there an issue for CREATE VIEW with ORDER BY? I couldn't find it with a quick Jira search. I would like to add a comment referencing your comments in this issue regarding ORDER BY. I will leave this issue open while I backport to 10.5,10.4, and 10.3.
          Hide
          Knut Anders Hatlen added a comment -

          I don't think there is an issue for it. Now that SQL:2008 allows ORDER BY in subqueries, it's more likely that we'll allow it for CREATE VIEW too (there's no issue for ORDER BY in subqueries either, I think).

          By the way, even without changing size() to visibleSize(), isn't the patch already assuming that the generated columns are at the end? If one of the generated columns appear at an index < visibleSize(), the corresponding colInfos[index] entry will be left blank, and when we come to a generated column with index >= visibleSize(), colInfos[index] will throw ArrayIndexOutOfBoundsException.

          Show
          Knut Anders Hatlen added a comment - I don't think there is an issue for it. Now that SQL:2008 allows ORDER BY in subqueries, it's more likely that we'll allow it for CREATE VIEW too (there's no issue for ORDER BY in subqueries either, I think). By the way, even without changing size() to visibleSize(), isn't the patch already assuming that the generated columns are at the end? If one of the generated columns appear at an index < visibleSize(), the corresponding colInfos [index] entry will be left blank, and when we come to a generated column with index >= visibleSize(), colInfos [index] will throw ArrayIndexOutOfBoundsException.
          Hide
          Kathey Marsden added a comment -

          Ah, good point. I guess I should have kept a separate counter for the colInfos index. Should I do that or go with your to original suggestion to keep the assumption and use colInfos.length?

          I want to get this right before I backport but there is some urgency for the user who encountered this issue, so I would like to get this resolved today if possible.

          Thanks

          Kathey

          Show
          Kathey Marsden added a comment - Ah, good point. I guess I should have kept a separate counter for the colInfos index. Should I do that or go with your to original suggestion to keep the assumption and use colInfos.length? I want to get this right before I backport but there is some urgency for the user who encountered this issue, so I would like to get this resolved today if possible. Thanks Kathey
          Hide
          Kathey Marsden added a comment -

          Here is a follow up patch for this issue . derby-4230_diff2.txt. It uses colInfos.length for the loop as Knut orignally suggested, making the assumption that the generated columns come at the end. Added a comment and an assertion if we are wrong in this assumption and hit a generated column in the expected visible range.

          I ran lang.ViewsTest but haven't run the full regression suite yet.

          Show
          Kathey Marsden added a comment - Here is a follow up patch for this issue . derby-4230_diff2.txt. It uses colInfos.length for the loop as Knut orignally suggested, making the assumption that the generated columns come at the end. Added a comment and an assertion if we are wrong in this assumption and hit a generated column in the expected visible range. I ran lang.ViewsTest but haven't run the full regression suite yet.
          Kathey Marsden made changes -
          Attachment derby-4230_diff2.txt [ 12408855 ]
          Hide
          Knut Anders Hatlen added a comment -

          Thanks Kathey. Adding an assert and a comment to make the assumption explicit is a good change. If we want an assert that's more likely to expose that generated grouping columns are in the visible range, RCL.numGeneratedColumns() is probably executed for a larger set of queries than CreateViewNode.

          (nit: trailing whitespace was added on the rcl=getResultColumns() line)

          Show
          Knut Anders Hatlen added a comment - Thanks Kathey. Adding an assert and a comment to make the assumption explicit is a good change. If we want an assert that's more likely to expose that generated grouping columns are in the visible range, RCL.numGeneratedColumns() is probably executed for a larger set of queries than CreateViewNode. (nit: trailing whitespace was added on the rcl=getResultColumns() line)
          Hide
          Kathey Marsden added a comment -

          Thanks Knut for the comment. I put the assertion in numGeneratedColumns() to verify that the generated columns come at the end of the list. I will run tests and see if it finds anything and I think just keep that assertion in there too with my checkin.

          Show
          Kathey Marsden added a comment - Thanks Knut for the comment. I put the assertion in numGeneratedColumns() to verify that the generated columns come at the end of the list. I will run tests and see if it finds anything and I think just keep that assertion in there too with my checkin.
          Hide
          Kathey Marsden added a comment -

          Committed to trunk revision 779319. I will leave open while I backport to 10.5,10.4, and 10.3.

          Show
          Kathey Marsden added a comment - Committed to trunk revision 779319. I will leave open while I backport to 10.5,10.4, and 10.3.
          Kathey Marsden made changes -
          Fix Version/s 10.6.0.0 [ 12313727 ]
          Hide
          Kathey Marsden added a comment -

          Attaching release note.

          Show
          Kathey Marsden added a comment - Attaching release note.
          Kathey Marsden made changes -
          Attachment releaseNote.html [ 12409466 ]
          Kathey Marsden made changes -
          Status Open [ 1 ] Closed [ 6 ]
          Fix Version/s 10.3.3.1 [ 12313143 ]
          Fix Version/s 10.4.2.1 [ 12313401 ]
          Fix Version/s 10.5.1.2 [ 12313870 ]
          Resolution Fixed [ 1 ]
          Dag H. Wanvik made changes -
          Bug behavior facts [High Value Fix] [Regression]
          Dag H. Wanvik made changes -
          Issue & fix info [Release Note Needed, Patch Available] [High Value Fix, Patch Available, Release Note Needed]
          Kathey Marsden made changes -
          Fix Version/s 10.5.2.0 [ 12314116 ]
          Fix Version/s 10.5.1.2 [ 12313870 ]
          Gavin made changes -
          Workflow jira [ 12463509 ] Default workflow, editable Closed status [ 12799480 ]

            People

            • Assignee:
              Kathey Marsden
              Reporter:
              Kathey Marsden
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development