[OPENJPA-2956] Unwrap ID(), honour null precedence and reject set operations in memory - #178
Open
rzo1 wants to merge 1 commit into
Open
[OPENJPA-2956] Unwrap ID(), honour null precedence and reject set operations in memory#178rzo1 wants to merge 1 commit into
rzo1 wants to merge 1 commit into
Conversation
…rations in memory Three separate defects on the in-memory path, all reachable: the executor is selected whenever a candidate collection is supplied, when the store does not support datastore execution, or when dirty instances are queried with FlushBeforeQueries disabled. ID() returned the internal identity wrapper rather than the raw key, so a comparison against the plain key threw a ClassCastException out of Filters.convert for numeric ids, never matched for an @EmbeddedId, and matched only by accident for a String id. It now unwraps exactly as the JDBC projection does. The wrapper-returning getObjectId() is unchanged. NULLS FIRST and NULLS LAST were ignored: the comparator hard coded nulls last when ascending and first when descending, so two of the four combinations were right by chance and two were silently wrong. The requested precedence is now threaded through, falling back to the previous policy when none is given. Set operations produced a NullPointerException from a compound expression with no filter, or an empty result. They are now rejected with a message that says why the query is running in memory and how to avoid it: the executor is built for one candidate extent and has no multiset semantics, and a candidate collection has no defined meaning across operands. Note that the in-memory path cannot yet be exercised end to end from JPQL with an identification variable: JPQLExpressionBuilder casts the value from getThis() to Path, and the in-memory factory returns a Val, so it fails with a ClassCastException. That is an older, separate defect and wants its own issue.
solomax
reviewed
Sep 3, 2026
solomax
left a comment
solomax
left a comment
Contributor
There was a problem hiding this comment.
Do we need tests for this?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three separate defects, all reachable: the in-memory executor is selected whenever a candidate collection is supplied, when the store does not support datastore execution (openjpa-xmlstore), or when dirty instances are queried with
FlushBeforeQueriesdisabled.ID()returned the internal identity wrapper rather than the raw key, so a comparison against the plain key threwClassCastExceptionout ofFilters.convertfor numeric ids, never matched for an@EmbeddedId(OpenJPAId.equalsrequires class equality), and matched only by accident for a String id. It now unwraps exactly as the JDBC projection does. One correction to the review comment: JDBC does not unwrap the comparison operand — it normalises the other side, and its only unwrap isGetNativeObjectId.load(). The wrapper-returninggetObjectId()is unchanged.NULLS FIRST/NULLS LASTwere ignored: the comparator hard-coded nulls last when ascending and first when descending, so two of the four combinations were already right and two were silently wrong. The requested precedence is now threaded through, falling back to the previous policy when none is given — so rejecting these instead would have been a regression.Set operations produced a
NullPointerExceptionfrom a compoundQueryExpressionswith no filter, or an empty result. They are now rejected with a message saying why the query is running in memory and how to avoid it. Implementing them would be a feature, not a fix: the executor is built for a single candidate extent,setCandidateCollectionhas no defined meaning across operands, and the kernel has no multiset semantics for the ALL variants.Note that the in-memory path cannot yet be exercised end to end from JPQL with an identification variable:
JPQLExpressionBuildercasts the value fromgetThis()toPath, and the in-memory factory returns aVal, so it fails with aClassCastException. That is an older, separate defect and wants its own issue; it is why theID()change has no end-to-end test here.