Skip to content

OAK-12051: Fix NPE when ordering union query by jcr:score #2746

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.

Sign up for GitHub

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

Open
ChlineSaurus wants to merge 3 commits into
base: OAK-12051
Choose a base branch
from
+79 −33
Open

OAK-12051: Fix NPE when ordering union query by jcr:score #2746

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fd972fc
OAK-12051: Fix NPE when ordering union query by jcr:score
Jan 8, 2026
5aa8db0
OAK-12051: Update naming, to distinguish flag after fix
Feb 2, 2026
ab14b1c
OAK-12051: Use sorting by score as new default.
Feb 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 6 additions & 7 deletions oak-core/src/main/java/org/apache/jackrabbit/oak/Oak.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -577,11 +577,10 @@ public Oak with(@NotNull Whiteboard whiteboard) {
}

if (queryEngineSettings != null) {
Feature sortUnionQueryByScoreFeature = newFeature(QueryEngineSettings.FT_SORT_UNION_QUERY_BY_SCORE, whiteboard);
LOG.info("Registered sort union query by score feature: " + QueryEngineSettings.FT_SORT_UNION_QUERY_BY_SCORE);
closer.register(sortUnionQueryByScoreFeature);
queryEngineSettings.setSortUnionQueryByScoreFeature(sortUnionQueryByScoreFeature);

Feature sortUnionQueryLegacyModeFeature = newFeature(QueryEngineSettings.FT_SORT_UNION_QUERY_LEGACY_MODE, whiteboard);
LOG.info("Registered sort union query legacy mode feature: " + QueryEngineSettings.FT_SORT_UNION_QUERY_LEGACY_MODE);
closer.register(sortUnionQueryLegacyModeFeature);
queryEngineSettings.setSortUnionQueryLegacyModeFeature(sortUnionQueryLegacyModeFeature);
Feature optimizeXPathUnion = newFeature(QueryEngineSettings.FT_OPTIMIZE_XPATH_UNION, whiteboard);
LOG.info("Registered optimize XPath union feature: " + QueryEngineSettings.FT_OPTIMIZE_XPATH_UNION);
closer.register(optimizeXPathUnion);
Expand Down Expand Up @@ -994,8 +993,8 @@ public void setPrefetchFeature(@Nullable Feature prefetch) {
settings.setPrefetchFeature(prefetch);
}

public void setSortUnionQueryByScoreFeature(@Nullable Feature feature) {
settings.setSortUnionQueryByScoreFeature(feature);
public void setSortUnionQueryLegacyModeFeature(@Nullable Feature feature) {
settings.setSortUnionQueryLegacyModeFeature(feature);
}

public void setOptimizeXPathUnion(@Nullable Feature feature) {
Expand Down
14 changes: 7 additions & 7 deletions oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryEngineSettings.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ public class QueryEngineSettings implements QueryEngineSettingsMBean, QueryLimit

public static final String OAK_QUERY_PREFETCH_COUNT = "oak.prefetchCount";

public static final String FT_SORT_UNION_QUERY_BY_SCORE = "FT_OAK-11949";
public static final String FT_SORT_UNION_QUERY_LEGACY_MODE = "FT_OAK-12051";

public static final String FT_OPTIMIZE_XPATH_UNION = "FT_OAK-12007";

Expand Down Expand Up @@ -120,7 +120,7 @@ public class QueryEngineSettings implements QueryEngineSettingsMBean, QueryLimit
private final long queryLengthErrorLimit = Long.getLong(OAK_QUERY_LENGTH_ERROR_LIMIT, 100 * 1024 * 1024); //100MB

private Feature prefetchFeature;
private Feature sortUnionQueryByScoreFeature;
private Feature sortUnionQueryLegacyModeFeature;
private Feature optimizeXPathUnion;

private String autoOptionsMappingJson = "{}";
Expand Down Expand Up @@ -226,13 +226,13 @@ public void setFastQuerySize(boolean fastQuerySize) {
System.setProperty(OAK_FAST_QUERY_SIZE, String.valueOf(fastQuerySize));
}

public void setSortUnionQueryByScoreFeature(@Nullable Feature feature) {
this.sortUnionQueryByScoreFeature = feature;
public void setSortUnionQueryLegacyModeFeature(@Nullable Feature feature) {
this.sortUnionQueryLegacyModeFeature = feature;
}

public boolean isSortUnionQueryByScoreEnabled() {
// disable if the feature toggle is not used
return sortUnionQueryByScoreFeature != null && sortUnionQueryByScoreFeature.isEnabled();
public boolean isSortUnionQueryLegacyModeEnabled() {
// Legacy mode (concatenate) is disabled by default; score-based sorting is the default behavior
return sortUnionQueryLegacyModeFeature != null && sortUnionQueryLegacyModeFeature.isEnabled();
}

public void setOptimizeXPathUnion(@Nullable Feature feature) {
Expand Down
12 changes: 6 additions & 6 deletions oak-core/src/main/java/org/apache/jackrabbit/oak/query/UnionQueryImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,8 +324,8 @@ public Iterator<ResultRowImpl> getRows() {
rightIter = ((MeasuringIterator) rightRows).getDelegate();
}
if (orderBy == null) {
if(!settings.isSortUnionQueryByScoreEnabled()) {
// Default old behavior
if(settings.isSortUnionQueryLegacyModeEnabled()) {
// Legacy mode: concatenate results without score-based merging
it = IteratorUtils.chainedIterator(leftIter, rightIter);
} else {
boolean leftHasScore = isScorePresent(left);
Expand Down Expand Up @@ -568,16 +568,16 @@ public int compare(ResultRowImpl left, ResultRowImpl right) {

/**
* @param row the result row
* @return the jcr:score as a double
* Precondition: {@link #isScorePresent(Query)} must be true. If the row lacks a jcr:score, 0.0 is returned and
* the issue is logged.
* @return the jcr:score as a double, or 0.0 if the row lacks a score value
*/
private double getScoreFromRow(ResultRowImpl row) {
try {
PropertyValue scoreValue = row.getValue(QueryConstants.JCR_SCORE);
if (scoreValue == null) {
return 0.0;
}
return scoreValue.getValue(Type.DOUBLE);
Copy link
Member

@thomasmueller thomasmueller Feb 25, 2026

Choose a reason for hiding this comment

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

I wonder if we should also check the type... maybe there are nodes with a "jcr:score" property but it's text? Probably not, but I guess we should be conservative.

} catch (IllegalArgumentException e) {
LOG.warn("Failed to get jcr:score for path={}", row.getPath(), e);
Copy link
Member

@thomasmueller thomasmueller Feb 25, 2026

Choose a reason for hiding this comment

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

I would still log

Copy link
Contributor Author

@ChlineSaurus ChlineSaurus Feb 25, 2026

Choose a reason for hiding this comment

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

Okey, I can change it. Two questions:

  • Should I reduce it to an Info/Debug log (in my understanding this could happen quite often)
  • Is path always given, or can it also be null (I don't want to introduce a new NPE when fixing one 😅)

Copy link
Member

@thomasmueller thomasmueller Feb 25, 2026

Choose a reason for hiding this comment

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

If it fails quite often, then we should probably change the code so that it first checks if the column is available, in order to not throw an exception in the first place.

return 0.0;
}
}
Expand Down
73 changes: 60 additions & 13 deletions oak-core/src/test/java/org/apache/jackrabbit/oak/query/UnionQueryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,8 @@ public class UnionQueryTest extends AbstractQueryTest {
protected ContentRepository createRepository() {
store = new MemoryNodeStore();
qeSettings = new QueryEngineSettings();
Feature sortFeature = createFeature(true);
qeSettings.setSortUnionQueryByScoreFeature(sortFeature);
Feature sortFeature = createFeature(false);
qeSettings.setSortUnionQueryLegacyModeFeature(sortFeature);

return new Oak(store)
.with(new OpenSecurityProvider())
Expand Down Expand Up @@ -439,36 +439,76 @@ public void testUnionMergingEmptyIterators() throws Exception {
}

@Test
public void testSortUnionQueryScoreFlagDisabled() throws Exception {
QueryEngineSettings disabledSettings = new QueryEngineSettings();
Feature sortFeature = createFeature(false);
disabledSettings.setSortUnionQueryByScoreFeature(sortFeature);
public void testSortUnionQueryLegacyModeEnabled() throws Exception {
// When legacy mode is enabled, query results should be concatenated
QueryEngineSettings legacySettings = new QueryEngineSettings();
Feature legacyModeFeature = createFeature(true);
legacySettings.setSortUnionQueryLegacyModeFeature(legacyModeFeature);
MockQueryBuilder leftQuery = new MockQueryBuilder(true)
.addResult("/left/doc1", 0.8)
.addResult("/left/doc2", 0.7);
MockQueryBuilder rightQuery = new MockQueryBuilder(true)
.addResult("/right/doc1", 0.9)
.addResult("/right/doc2", 0.6);

UnionQueryImpl unionQuery = new UnionQueryImpl(true, leftQuery.build(), rightQuery.build(), disabledSettings);
UnionQueryImpl unionQuery = new UnionQueryImpl(true, leftQuery.build(), rightQuery.build(), legacySettings);
List<ScoredResult> results = executeUnionAndGetScoredResults(unionQuery);
assertPathOrder(results, new String[]{"/left/doc1", "/left/doc2", "/right/doc1", "/right/doc2"});
}

@Test
public void testSortUnionQueryScoreFlagIsNull() throws Exception {
QueryEngineSettings disabledSettings = new QueryEngineSettings();
disabledSettings.setSortUnionQueryByScoreFeature(null);
public void testSortUnionQueryLegacyModeNotSet() throws Exception {
QueryEngineSettings defaultSettings = new QueryEngineSettings();
defaultSettings.setSortUnionQueryLegacyModeFeature(null);
MockQueryBuilder leftQuery = new MockQueryBuilder(true)
.addResult("/left/doc1", 0.8)
.addResult("/left/doc2", 0.7);
MockQueryBuilder rightQuery = new MockQueryBuilder(true)
.addResult("/right/doc1", 0.9)
.addResult("/right/doc2", 0.6);

UnionQueryImpl unionQuery = new UnionQueryImpl(true, leftQuery.build(), rightQuery.build(), disabledSettings);
UnionQueryImpl unionQuery = new UnionQueryImpl(true, leftQuery.build(), rightQuery.build(), defaultSettings);
List<ScoredResult> results = executeUnionAndGetScoredResults(unionQuery);
assertPathOrder(results, new String[]{"/left/doc1", "/left/doc2", "/right/doc1", "/right/doc2"});
assertPathOrder(results, new String[]{"/right/doc1", "/left/doc1", "/left/doc2", "/right/doc2"});
}

@Test
public void testUnionMergingWithNullScoreValue() throws Exception {
MockQueryBuilder leftQuery = new MockQueryBuilder(true)
.addResult("/left/doc1", 0.8)
.addResult("/left/docNull", (Double) null);
MockQueryBuilder rightQuery = new MockQueryBuilder(true)
.addResult("/right/doc1", 0.9)
.addResult("/right/doc2", 0.6);

UnionQueryImpl unionQuery = new UnionQueryImpl(true, leftQuery.build(), rightQuery.build(), qeSettings);
List<ScoredResult> results = executeUnionAndGetScoredResults(unionQuery);

assertPathOrder(results, new String[]{"/right/doc1", "/left/doc1", "/right/doc2", "/left/docNull"});
}

@Test
public void testNestedUnionWithMixedScores() throws Exception {
// Simulates scenario where score exists for some rows, and is null for others
MockQueryBuilder queryA = new MockQueryBuilder(true)
.addResult("/a/doc1", 0.9)
.addResult("/a/doc2", 0.5);
MockQueryBuilder queryB = new MockQueryBuilder(false)
.addResult("/b/doc1")
.addResult("/b/doc2");
MockQueryBuilder queryC = new MockQueryBuilder(true)
.addResult("/c/doc1", 0.8)
.addResult("/c/doc2", 0.3);

// Inner union: A (has score) UNION B (no score)
UnionQueryImpl innerUnion = new UnionQueryImpl(true, queryA.build(), queryB.build(), qeSettings);
// Outer union: innerUnion UNION C
UnionQueryImpl outerUnion = new UnionQueryImpl(true, innerUnion, queryC.build(), qeSettings);
List<ScoredResult> results = executeUnionAndGetScoredResults(outerUnion);

assertPathOrder(results, new String[]{
"/a/doc1", "/c/doc1", "/a/doc2", "/c/doc2", "/b/doc1", "/b/doc2"
});
}

private QueryImpl createQuery (String statement, ConstraintImpl c, SourceImpl sImpl) throws Exception {
Expand Down Expand Up @@ -512,12 +552,19 @@ public MockQueryBuilder(boolean hasScore) {
}

public MockQueryBuilder addResult(String path, double score) {
if (!hasScore) {
throw new IllegalStateException("Cannot provide score");
}
return addResult(path, Double.valueOf(score));
}

public MockQueryBuilder addResult(String path, Double score) {
if (!hasScore) {
throw new IllegalStateException("Cannot provide score");
}
PropertyValue[] values = new PropertyValue[] {
PropertyValues.newString(path),
PropertyValues.newDouble(score)
score == null ? null : PropertyValues.newDouble(score)
Copy link
Member

@thomasmueller thomasmueller Feb 25, 2026

Choose a reason for hiding this comment

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

This looks wrong to me. What about:

Suggested change
score == null ? null : PropertyValues.newDouble(score)
PropertyValues.newDouble(score == null ? 0.0 : score)

Copy link
Contributor Author

@ChlineSaurus ChlineSaurus Feb 25, 2026

Choose a reason for hiding this comment

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

This is only for mocking behavior in tests. The goal is that it returns null, so the change would defeat the purpose in my opinion. The mock should be able to return null to test if the actual code can handle a null return.

What this (somewhat weird) line does is to ensure, that we only try to turn the value to be returned by the mock into a Double if it isn't null, with the goal that the same mocking code can be used for tests with null and actual scores.

Copy link
Member

@thomasmueller thomasmueller Feb 25, 2026

Choose a reason for hiding this comment

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

Yes, I see, you are right! Sorry I got confused.

};
results.add(new ResultRowImpl(mockQuery, null, values, null, null));
return this;
Expand Down