-
Notifications
You must be signed in to change notification settings - Fork 3
$expand tries to read much more data, than actually needed #37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main-javax
Are you sure you want to change the base?
Conversation
… so no redundant data loaded from the DB
|
Do you have a more concrete example/scenario to explain, when it happens that too much data is read?! |
|
It happens when data for ManyToOne dependencies being loaded. There is nothing limiting the request so all the data from the dependant table being loaded. |
|
So sorry for the delay... but the changes are breaking the test suite, so we have to inspect what happens.. With the current state the PR will have side effects, we have to manage; means still more work... |
|
I didn't manage to make tests pass even before my changes. |
|
I hope i will find also time for an deeper look. You can run the complete build + test suite via: |
|
In win 10 with oracle's java build 1.8.0_281-b09 and Apache Maven 3.6.3 the command you provided still does not pass against untouched branch |
|
Looks like it was a maven bug can be fixed by adding And I'm actually can't find neither |
These excel-sheets are created by the test classes (testing the excel export), so they are not in the repository. I think your setup is correct (the build + tests are started). That you fail in 'processor' is expected; that is because i telled you that your changes have side effects.... |
|
Are you saying that those errors about *.xlsx files caused by my changes? |
| queryResult.putElementCollectionResults(readElementCollections(elementCollectionMap)); | ||
|
|
||
| if (processExpandOption && !intermediateResult.isEmpty()) { | ||
| String idAttributeName = getQueryResultType().getKeyAttributes(false).get(0).getInternalName(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is the reason for some of the test failures: your using the attribute name to get the result value from the tuple, but in the tuple the result is accessible only be the alias... so you have to do:
String idAttributeName = getLastAffectingNavigationKeyBuilder().getNavigationKeyPaths().get(0).getAlias();
But there are more issues... one should be the fact, that you using only one column as key (see ...get(0)), but there are entities with multiple key columns in the world, so you have to bring ALL key columns into effect for the expand query...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi
What would be the proper way to get all the key fields?
Anyway, I think not handling multi-column keys is still better than it was before :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getLastAffectingNavigationKeyBuilder().getNavigationKeyPaths().get(0).getAlias() gives me something else other than key column name. My JPA entities does not work with this change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems we cannot use tuples in JPA criteria builder of an inClause. Anyway odata cannot express composite primary keys either
Here is the fix