Metamodel 1132 sql native paging #137
Conversation
- set for SQLServer 2012+ and Oracle 12+ - added new tests of the clause creation - fix tests of SQL Server and Oracle
- moved the code pieces to better places
|
|
||
| public OracleQueryRewriter(JdbcDataContext dataContext) { | ||
| super(dataContext); | ||
| super(dataContext, 11); |
There was a problem hiding this comment.
Probably want to use a constant here to document the magic number, like FIRST_FETCH_SUPPORTING_VERSION (or maybe something a little less clumsy than what I came up with 😄).
There was a problem hiding this comment.
BTW, I thought it was only supported from version 12c in Oracle?
| @@ -460,4 +460,25 @@ public Object getResultSetValue(ResultSet resultSet, int columnIndex, Column col | |||
| } | |||
| return resultSet.getObject(columnIndex); | |||
| } | |||
|
|
|||
| protected boolean isSupportedDatabase(String databaseProductName, int databaseVersion) { | |||
There was a problem hiding this comment.
Shouldn't this be limited to isSupportedVersion? I mean, hopefully the concrete rewriters know what product they are.
| @Override | ||
| public boolean isMaxRowsSupported() { | ||
| return true; | ||
| super(dataContext, 11); |
|
BTW, not a blocker for me, but you could consider to use a separate class that are used by SQL Server and Oracle rewriters instead of inheritance for this. It's not really a problem now, but if we want to use other features that are shared between some database engines, this design will break down rather quickly. |
| return false; | ||
| } | ||
|
|
||
| private int databaseVersionToInt(String version) { |
There was a problem hiding this comment.
I'd call this method something like getDatabaseMajorVersion() just because it will return the major version element only, not something that represents the 'real' version.
| firstDot = version.indexOf('.'); | ||
| } | ||
| if(firstDot >= 0) { | ||
| return Integer.valueOf(version.substring(0, firstDot)); |
There was a problem hiding this comment.
This could throw an exception if the version from the DB is something like R12.0.
- needed order by for correct execution - removed fetch check for TOP
| firstDot = version.indexOf('.'); | ||
| } | ||
| if(firstDot >= 0) { | ||
| return Integer.valueOf(version.substring(0, firstDot)); |
There was a problem hiding this comment.
Not sure if we want to make this more "defensive" in terms of programming. I can't imagine it really, but if the string argument is just "." then this substring will return empty string and that will throw exception when parsing as integer. It's not really something I think is likely to happen, but just saying :-)
|
LGTM |
|
Shall we pull then? I want to test this using the DH connector. Thanks guys! |
|
Merged :) |
fixes https://issues.apache.org/jira/browse/METAMODEL-1132