[jira] [Commented] (METAMODEL-1161) Max rows doesn't work on Oracle without offset and ORDER BY

["ASF GitHub Bot (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/METAMODEL-1161?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16139875#comment-16139875 ] 

ASF GitHub Bot commented on METAMODEL-1161:
-------------------------------------------

GitHub user LosD opened a pull request:

    https://github.com/apache/metamodel/pull/158

    METAMODEL-1161: Max rows without offset and order by

    This fixes Oracle and SQL Server max rows and offset to work properly in all viable situations, and to use post processing otherwise.
    
    Please note that Oracle supports offset without order by, but that it is too inconsistent to use for much.
    
    Also fixes Oracle and (jTDS) SQL Server tests, which had bit-rotted quite a bit. For SQL Server, I had to ignore the timestamp test to avoid changing `JdbcTestTemplates.timestampValueInsertSelect`, which makes assumptions about how database engines rounds, and due to the use of `toString()` more tests how the database reacts than how MetaModel does. If someone wants to fix that particular mess, be my guest, but I'm not touching that with a 10-foot logic probe. :)
    
    // rant begin
    In general, all the integration tests are _very_ prone to bitrot due to over-assuming, e.g. orders and types. Unfortunately, the heavy use of `toString()` makes it much worse. Other problems is assuming certain versions of DB manufacturer's test databases without stressing which actual version was used, and of course that the integration tests are barely ever run. Unfortunately, I don't think we can do much about how often they are run, but at least we should ensure that the tests only tests MetaModel-related things, to keep them immune from breakage due to database changes and internal MetaModel refactorings, i.e.; NO specific count of schemas, tables or rows unless absolutely needed, NO assuming order of schemas, NO assuming existance og non-existance of unrelated schemas, NO assuming order of unordered queries, ABSOLUTELY NO use of toString on overall types or (especially) arrays.
    
    In short: Test that the query worked and that MetaModel-specific things are as expected, nothing more. Creating our own schemas, tables and data is probably also a good move, then at least we can make assumptions on that without going under for every new version (as long as we don't assume the non-presence of other schemas and tables)
    // rant end 
    
    Fixes METAMODEL-1161

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/LosD/metamodel bug/METAMODEL-1161-max-rows-oracle

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/metamodel/pull/158.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #158
    
----
commit 86cd2b305e15128ae2a71718547e72ff0a1d674f
Author: Dennis Du Krøger <lo...@apache.org>
Date:   2017-08-24T08:31:31Z

    METAMODEL-1161: Max rows without offset and order by
    
    This fixes Oracle and SQL Server max rows and offset to work properly
    in all viable situations, and to use post processing otherwise.
    
    Please note that Oracle supports offset without order by, but that it
    is too inconsistent to use for much.
    
    Also fixes Oracle and SQL Server tests.

----


> Max rows doesn't work on Oracle without offset and ORDER BY
> -----------------------------------------------------------
>
>                 Key: METAMODEL-1161
>                 URL: https://issues.apache.org/jira/browse/METAMODEL-1161
>             Project: Apache MetaModel
>          Issue Type: Bug
>    Affects Versions: 4.5.4
>            Reporter: Dennis Du Krøger
>            Priority: Critical
>
> On Oracle, the query rewriting is completely broken for max rows, unless an offset and ORDER BY clause is also specified, and the version is 12.1.
> This seems to stem from the fix for METAMODEL-1132, which made a new OffsetFetchQueryRewriter and then extended OracleQueryRewriter and SQLServerQueryRewriter on top, but is unfortunately missing quite a bit of checks before concluding that `isMaxRowsSupported` should return true: no version check, and no checking that the query actually has the needed clauses set.
> The {{SQLServerQueryRewriter}} is not affected, since it  has code to rewrite the SELECT clause.
> h5. Reproduction
> Query an Oracle datastore with e.g.
> {code}
> context.query().from("tablename").selectAll().maxRows(10).execute()
> {code}
> and print the resulting dataset.
> h5. Expected result
> Only 10 records in the dataset
> h5. Actual result
> All records are in the dataset



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)