Allow subclasses of groovy.sql.Sql to get ResultSet

Details

  • Type: Improvement Improvement
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: 1.6.4, 1.7-beta-1
  • Fix Version/s: 1.6.5, 1.7-beta-2
  • Component/s: SQL processing
  • Labels:
    None
  • Testcase included:
    yes
  • Patch Submitted:
    Yes

Description

In order for applications to perform optimally where multiple, large databases are in use, it can be important to iterate through a result set without using a closure. One use case is a merge of records from multiple databases: the company has several divisions and each has a an order database, each day, the orders for each SKU must be totalled across all divisions.

The desired solution is to submit a query to each of the databases in parallel.

String query = "select SKU, sum(Qty) from OrderItems group by SKU, order by SKU"

The results from the various DBs are then combined using a merge algorithm.

This cannot be accomplished with the current API, since it's impractical to load the results of the queries into List objects as is done by the rows(...) methods.

The attached patch provides a path to implementing the desired solution by adding new protected methods. It also refactors to consolidate logic and remove an exposure to NPE. In addition to new protect methods, a static method to create a List from a ResultSet is made public and the log field is made static.

The patch includes a new test case added to SqlTest.groovy

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Paul King added a comment -

Looks like the attachment is an unchanged existing file rather than a patch?

Show
Paul King added a comment - Looks like the attachment is an unchanged existing file rather than a patch?
Hide
Permalink
John Bito added a comment -

Sorry for wasting time with the botched pasting!

Show
John Bito added a comment - Sorry for wasting time with the botched pasting!
Hide
Permalink
John Bito added a comment -

The attached file really is a patch, now!

Show
John Bito added a comment - The attached file really is a patch, now!
Hide
Permalink
Paul King added a comment -

Thanks. Is there a need for the following method to be public?

public static List<GroovyRowResult> resultSetAsList(String sql, ResultSet rs) throws SQLException
Show
Paul King added a comment - Thanks. Is there a need for the following method to be public? 
public static List<GroovyRowResult> resultSetAsList(String sql, ResultSet rs) throws SQLException
Hide
Permalink
John Bito added a comment -

I was debating how to do the asList. I created the static method because it doesn't need any instance fields or methods. Then it seemed that it could be useful to classes that get a ResultSet from call to a stored proc. Since my current code is a subclass, it can use the protected asList instance method.

Feel free to change the visibility of the static method (or roll it back into the asList member).

I just noticed that I created the patch file before removing the //TODO in PreparedQueryCommand constructor, so if you're thinking of committing the patch, please remove that line.

Thanks!
John

Show
John Bito added a comment - I was debating how to do the asList. I created the static method because it doesn't need any instance fields or methods. Then it seemed that it could be useful to classes that get a ResultSet from call to a stored proc. Since my current code is a subclass, it can use the protected asList instance method. Feel free to change the visibility of the static method (or roll it back into the asList member). I just noticed that I created the patch file before removing the //TODO in PreparedQueryCommand constructor, so if you're thinking of committing the patch, please remove that line. Thanks! John
Hide
Permalink
Paul King added a comment -

Added to trunk - pending merge to 1_6_X branch

Show
Paul King added a comment - Added to trunk - pending merge to 1_6_X branch
Hide
Permalink
John Bito added a comment -

Thanks, Paul!

Please remove the TODO comment on line 2011 of Sql.java if you have a chance.

I still question the Sql(Sql) constructor. I've been unable to come up with a use case outside of testing. Would you consider marking it as deprecated with the idea that it could be moved to package visibility, or does the Groovy POV influence this?

Inquiring minds...

Thanks again!
John

Show
John Bito added a comment - Thanks, Paul! Please remove the TODO comment on line 2011 of Sql.java if you have a chance. I still question the Sql(Sql) constructor. I've been unable to come up with a use case outside of testing. Would you consider marking it as deprecated with the idea that it could be moved to package visibility, or does the Groovy POV influence this? Inquiring minds... Thanks again! John
Hide
Permalink
John Bito added a comment -

Another question: The code coverage seems not to reflect SqlCacheTest: http://build.canoo.com/groovy/artifacts/20090808171013/reports/cobertura/groovy.sql.Sql.html on line 1893 indicates that cacheStatements isn't being set true. Should something be changed to get the code coverage analysis to include those tests?

Show
John Bito added a comment - Another question: The code coverage seems not to reflect SqlCacheTest: http://build.canoo.com/groovy/artifacts/20090808171013/reports/cobertura/groovy.sql.Sql.html on line 1893 indicates that cacheStatements isn't being set true. Should something be changed to get the code coverage analysis to include those tests?
Hide
Permalink
Paul King added a comment -

Merged onto 1.6 branch. To answer question about coverage, I haven't had time to investigate properly yet but I suspect cobertura coverage database file needs flushing on CI server.

Show
Paul King added a comment - Merged onto 1.6 branch. To answer question about coverage, I haven't had time to investigate properly yet but I suspect cobertura coverage database file needs flushing on CI server.

People

  • Assignee:
    Paul King
    Reporter:
    John Bito
Vote (0)
Watch (3)

Dates

  • Created:
    Updated:
    Resolved: