You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@metamodel.apache.org by LosD <gi...@git.apache.org> on 2017/08/24 10:41:45 UTC
[GitHub] metamodel pull request #158: METAMODEL-1161: Max rows without offset and ord...
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.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel issue #158: METAMODEL-1161: Max rows without offset and order by
Posted by LosD <gi...@git.apache.org>.
Github user LosD commented on the issue:
https://github.com/apache/metamodel/pull/158
Whoops, missed the query rewriter tests. Will fix.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel issue #158: METAMODEL-1161: Max rows without offset and order by
Posted by LosD <gi...@git.apache.org>.
Github user LosD commented on the issue:
https://github.com/apache/metamodel/pull/158
Actually, that's one of the worst kind for me (in general, but especially in this context), since they assume order, and often lead to unfocused tests (e.g. testing the whole list instead of that the element you added is present). Unfocused tests fails if you look at the code a bit too hard ☺️
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request #158: METAMODEL-1161: Max rows without offset and ord...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/metamodel/pull/158
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel issue #158: METAMODEL-1161: Max rows without offset and order by
Posted by LosD <gi...@git.apache.org>.
Github user LosD commented on the issue:
https://github.com/apache/metamodel/pull/158
BTW, any chance that we could also add this to a 4.x bugfix release? In DataCleaner we'll soon make a bugfix release, and moving to 5.x for that seems like a quite a change
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel issue #158: METAMODEL-1161: Max rows without offset and order by
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the issue:
https://github.com/apache/metamodel/pull/158
Btw. probably contrary to what you think, I agree with your rant :-) I do think "toString tests" can sometimes have value, but they are being way over-used in the integration tests and certainly assert waaaay too much. I do like "toString tests" for something simple such as asserting the contents of a string list or so, like this:
```
assertEquals("[id, name, foo, bar]", table.getColumnNames().toString());
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel issue #158: METAMODEL-1161: Max rows without offset and order by
Posted by LosD <gi...@git.apache.org>.
Github user LosD commented on the issue:
https://github.com/apache/metamodel/pull/158
Nope, only the version you're maintaining. We have no update plans until next major release
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel issue #158: METAMODEL-1161: Max rows without offset and order by
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the issue:
https://github.com/apache/metamodel/pull/158
Please merge, but also remember to add an entry in CHANGES.md
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel issue #158: METAMODEL-1161: Max rows without offset and order by
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the issue:
https://github.com/apache/metamodel/pull/158
Ok, I think you have committer rights to prepare a 4.6.1 release branch with the fix then.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel issue #158: METAMODEL-1161: Max rows without offset and order by
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the issue:
https://github.com/apache/metamodel/pull/158
Code LGTM. So we can merge.
As for a release, I looked at DataCleaner and haven't you actually upgraded to some 5.0 RC version? Not the latest with the big arrays-to-collections change though.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel issue #158: METAMODEL-1161: Max rows without offset and order by
Posted by tomatophantastico <gi...@git.apache.org>.
Github user tomatophantastico commented on the issue:
https://github.com/apache/metamodel/pull/158
I found those "toString" test to be quite problematic too, even in small tests and i encountered a lot of them during the arrays->collection conversion and i would be happy to see them phased out, even the small ones.
The schema uses a lot of interfaces but tests against the string serialisation, which is meant for human consumption.
`assertTrue(Lists.newArrayList("a","b").equals(table.getColumnNames()))`
is short, but does not depend on the toString() of the implementing List.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel issue #158: METAMODEL-1161: Max rows without offset and order by
Posted by LosD <gi...@git.apache.org>.
Github user LosD commented on the issue:
https://github.com/apache/metamodel/pull/158
Unfocused tests also has the problem that it's often unclear what it intends to test, it makes it harder to understand what the purpose of the code it is testing was, making its value as part of the documentation lower, and harder to conclude if a change was actually breaking stuff, or if the test was just a casualty of something unrelated.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---