Skip to content
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

[#857] Fix some regressions in JPQLNext parser #862

Merged
merged 2 commits into from
Oct 7, 2019

Conversation

Mobe91
Copy link
Contributor

@Mobe91 Mobe91 commented Oct 2, 2019

Fixes #857

@Mobe91 Mobe91 requested a review from beikov October 2, 2019 16:20
Copy link
Member

@beikov beikov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but you missed a few cases

  • com.blazebit.persistence.view.impl.objectbuilder.mapper.AbstractCorrelationJoinTupleElementMapper#AbstractCorrelationJoinTupleElementMapper
  • com.blazebit.persistence.view.impl.PrefixingQueryGenerator#prefix
  • com.blazebit.persistence.view.impl.objectbuilder.ViewTypeObjectBuilderTemplate#getCorrelationBasisType

Also, please add a test for this

@Mobe91
Copy link
Contributor Author

Mobe91 commented Oct 3, 2019

Not really, the cases that you mention did not change compared to before your parser rework commit so I did not touch them.
I would need to disable expression caching to create a test case that triggers this behavior - ok?

@beikov
Copy link
Member

beikov commented Oct 3, 2019

True but the cases I mentioned should also allow object results.
You shouldn't need to disable a cache to reproduce this. If you do, that would be a bug.

@Mobe91
Copy link
Contributor Author

Mobe91 commented Oct 3, 2019

True but the cases I mentioned should also allow object results.

Ok, I will apply the changes then.

You shouldn't need a cache to reproduce this. If you do, that would be a bug.

Expression caching itself is a bug at the moment. The cache keys need to consider the various flags that influence behavior of parsing and the expression visitor which is currently not the case. If it was implemented that way, those regressions (and possibly the additional cases you mentioned) would have come up with the existing tests I think.

I can fix the expression caching in the course of this issue.

@beikov
Copy link
Member

beikov commented Oct 3, 2019

Please create constants for the bit masks

@beikov
Copy link
Member

beikov commented Oct 4, 2019

@beikov beikov force-pushed the master branch 5 times, most recently from 33a84d7 to fc7db50 Compare October 7, 2019 10:36
@Mobe91 Mobe91 force-pushed the issues/857-fix-parser-regressions branch from 4961369 to 84c82f2 Compare October 7, 2019 10:50
@beikov beikov merged commit 8060684 into Blazebit:master Oct 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parser regression with top level treat expressions
2 participants