#2010 unknown ids on window subquery #2011
Conversation
|
All the tests failed, with a cache error? |
beikov
force-pushed
the
iss-2010-UnknownIdsOnWindowSubquery
branch
from
6fb6121 to
8168d48
Compare
|
Thanks for notifying me about this. I updated the GitHub action step versions and rebased your PR. |
|
Thank you, I've looked through the now failing tests, most of these seem to be due to the join on subquery not supported on hibernate pre-5.1, or unsupported in datanucleus/eclipselink. I've disabled the new test on those. The failures on the oracle DB seem to be related to #295 just as all the CTE tests are disabled for oracle, so I've also disabled testing on that. |
|
Looks good to me, can you please squash the commits and use |
kizzywuzzit
force-pushed
the
iss-2010-UnknownIdsOnWindowSubquery
branch
from
302c531 to
baa678e
Compare
|
Squashed and pushed, but isn't that usually done during merge? |
Thanks. It can be done during merge, but then I think that authorship attribution might be messed up. Either way, having the PR follow our standards is the preferred approach. |
|
|
||
| for (Map.Entry<String, Collection<?>> entry : listParameters.entrySet()) { | ||
| baseQuery.setParameter(entry.getKey(), entry.getValue()); | ||
| if (!entry.getValue().isEmpty()) { |
There was a problem hiding this comment.
Is this really necessary? I think this might cause problems if you first execute a query with a list of e.g. 2 elements and then again with an empty list.
There was a problem hiding this comment.
Yes, the reason it's needed is that the ids_# values aren't set during the initalisation like the param_# values are, they're populated later on. We skip the setting here, otherwise it causes ERROR: <AST>:0:0: unexpected end of subtree
| } else if (identifierExpressionsToUse.length > 1) { | ||
| for (Parameter<?> p : baseQuery.getParameters()) { | ||
| if (p.getName().startsWith(skippedParameterPrefix)) { | ||
| parameterManager.registerParameterName(p.getName(), true, null, this); |
There was a problem hiding this comment.
Instead of "dirtying" the parameter manager, I would suggest we create ParameterManager.ParameterImpl instances directly in the else branch below and add these instances to the parameters collection.
There was a problem hiding this comment.
That was one of the things I looked at, the issue was that we need the underlying AbstractCustomQuery to have both the parameter and the value binder. Without that the AbstractCustomQuery.ValuesParameter fails in setValue as it throws throw new IllegalArgumentException("The size of the collection must be lower or equal to the specified size for the VALUES clause."); on line 439
There was a problem hiding this comment.
I'm certainly open to an improved way to handle this parameter registration, but I'm admittedly not sure what the alternative would be.
There was a problem hiding this comment.
@beikov just wanted to follow up if you've had a chance to read above or if you have any suggestions for what I was seeing?
There was a problem hiding this comment.
Hi and sorry it took me so long. If you could spend some time to try and figure out a way to make this work without registering parameters in the parameter manager, that would be really helpful. Otherwise I have to look into this more deeply which I am not sure when I can get to. Unfortunately, I don't have any good suggestions other than what I already wrote. I'm kind of surprised that values parameters play a role here, since there is no values clause in your test case. Maybe you mixed something up there?
There was a problem hiding this comment.
Okay, I'll try to take a look. as for
there is no values clause in your test case. Maybe you mixed something up there?
It's because I've also disabled the inline ID query so when it first fetches the IDs, it's then creating the object query taking in IDs as values intrinsically.
Description
Fix issue where join on window query and sort by left joined column breaks
Related Issue
Fixes #2010
Motivation and Context
See issue above for info