Skip to content

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

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

Closed
ChlineSaurus wants to merge 3 commits into from
Closed

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

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
Copy link
Copy Markdown
Member

@thomasmueller thomasmueller Jan 19, 2026

Choose a reason for hiding this comment

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

The problem is now that the feature toggle needs to be changed. Otherwise we can never enable it, as there is no way to distinguish between the old and the new version.

Copy link
Copy Markdown
Contributor Author

@ChlineSaurus ChlineSaurus Jan 19, 2026

Choose a reason for hiding this comment

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

Oh, fair enough. How would you do this? Add a second feature toggle? Or is renaming the existing one enough?

Copy link
Copy Markdown
Member

@thomasmueller thomasmueller Jan 21, 2026

Choose a reason for hiding this comment

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

Renaming the existing one is enough.

Copy link
Copy Markdown
Member

@thomasmueller thomasmueller Jan 21, 2026

Choose a reason for hiding this comment

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

There is a large delay between us changing Oak and then enabling the feature toggle. Also, the changed behavior is not tested currently before the customer. I think we should try a different approach: we should enable the new behavior by default, and keep the feature toggle to be able to switch back to the old behavior where needed.

Copy link
Copy Markdown
Contributor Author

@ChlineSaurus ChlineSaurus Feb 3, 2026

Choose a reason for hiding this comment

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

I responded to the issues. However, I currently have problems when running the tests locally (OOM errors in Oak Segment AWS and Oak Segment Azure, it only works when running with -Dbaseline.skip=true, and even then it is sometimes flakey)

*/
private double getScoreFromRow(ResultRowImpl row) {
try {
PropertyValue scoreValue = row.getValue(QueryConstants.JCR_SCORE);
if (scoreValue == null) {
return 0.0;
}
return scoreValue.getValue(Type.DOUBLE);
} catch (IllegalArgumentException e) {
LOG.warn("Failed to get jcr:score for path={}", row.getPath(), e);
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)
};
results.add(new ResultRowImpl(mockQuery, null, values, null, null));
return this;
Expand Down