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

• Key: DERBY-4230
• Type: Bug
• Status: Closed
• Resolution: Fixed
• Priority: Major
• Assignee: Kathey Marsden
• Reporter: Kathey Marsden
• Component/s: JDBC
• Affects Version/s: 10.3.2.1
• Fix Version/s: 10.3.3.1, 10.4.2.1, 10.5.2.0, 10.6.0.0

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

Comments

[Kathey Marsden]

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]

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

[Kathey Marsden]

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]

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]

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.

[Knut Anders Hatlen]

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.

[Kathey Marsden]

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.

[Knut Anders Hatlen]

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.

[Kathey Marsden]

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

[Kathey Marsden]

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.

[Knut Anders Hatlen]

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

[Kathey Marsden]

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.

[Knut Anders Hatlen]

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.

[Kathey Marsden]

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

[Kathey Marsden]

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.

[Knut Anders Hatlen]

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)

[Kathey Marsden]

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.

[Kathey Marsden]

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

[Kathey Marsden]

Attaching release note.