Fix order by rendering for queries containing UNION #3429
Conversation
| void sortShouldBeAppendedWithSpacingInCaseOfSetOperator() { | ||
|
|
||
| String source = "SELECT tb FROM Test tb WHERE (tb.type='A') UNION SELECT tb FROM Test tb WHERE (tb.type='B')"; | ||
| String target = createQueryFor(source, Sort.by("Type").ascending()); |
There was a problem hiding this comment.
After this line target has the value
SELECT tb
FROM Test tb
WHERE (tb.type = 'A')
order by tb.Type asc
UNION
SELECT tb
FROM Test tb
WHERE (tb.type = 'B')
order by tb.Type asc`i.e. the order by is applied twice which is illegal, since according to answers here https://stackoverflow.com/questions/4715820/how-to-order-by-with-union-in-sql the later order by applies to the full union and an order by on the individual selects might be even illegal (probably depends on the database)
There was a problem hiding this comment.
since we do not know the target db nor how it handles order by we cannot imply ordering the last is the correct behaviour, nor do we have the means to define which of the selects to decorate. an alternative would be to raise an error and demand wrapping the entire thing as it's done in one of the follow up tests.
There was a problem hiding this comment.
Probably, let's wait and see what kind of reports we get. We can then still react to the feedback we receive.
There was a problem hiding this comment.
reading through IBM an Oracle SQL reference docs @schauder has a good point in the above mentioned query not being compliant (at least not in the form it is currently rendered)
For example, the following is not valid (SQLSTATE 428FJ):
SELECT * FROM T1
ORDER BY C1
UNION
SELECT * FROM T2
ORDER BY C1The following example is valid:
(SELECT * FROM T1
ORDER BY C1)
UNION
(SELECT * FROM T2
ORDER BY C1)
| String source = "SELECT tb FROM Test tb WHERE (tb.type='A') UNION SELECT tb FROM Test tb WHERE (tb.type='B')"; | ||
| String target = createQueryFor(source, Sort.by("Type").ascending()); | ||
|
|
||
| assertThat(target).contains(" UNION SELECT ").doesNotContainPattern(Pattern.compile(".*\\SUNION")); |
There was a problem hiding this comment.
Since we put in an exact SQL statement we should be able to assert on a precise output and not rely on pattern matching and simiar.
| } | ||
|
|
||
| @ParameterizedTest // GH-3427 | ||
| @ValueSource(strings = {"", "res"}) |
There was a problem hiding this comment.
A comment would be nice explaining why we need to run this with different prefixes.
There was a problem hiding this comment.
deemed to be obvious - apparently isn't
| @ValueSource(strings = {"", "res"}) | ||
| void sortShouldBeAppendedToSubSelectWithSetOperatorInSubselect(String alias) { | ||
|
|
||
| String prefix = StringUtils.hasText(alias) ? (alias + ".") : ""; |
There was a problem hiding this comment.
Again I think we should be able to and prefer a simple equals-assertion.
There was a problem hiding this comment.
I'm not a fan of duplication.
|
we should additionally check the count query creation for |
Make sure to append space after order by clause and fix alias detection for wrapped sub select. Also make sure to ignore alias used in subselect so they do not conflict with root ones.
christophstrobl
force-pushed
the
issue/3427
branch
from
d092eb8
to
82b28a4
Compare
Make sure to append space after order by clause and fix alias detection for wrapped sub select.
Also make sure to ignore alias used in subselect so they do not conflict with root ones.
Closes: #3427