From fd972fc522043b14bf62e0eb4eb9c9b3abeb6907 Mon Sep 17 00:00:00 2001 From: marvinw Date: Thu, 8 Jan 2026 09:41:15 +0100 Subject: [PATCH 1/3] OAK-12051: Fix NPE when ordering union query by jcr:score --- .../jackrabbit/oak/query/UnionQueryImpl.java | 8 ++-- .../jackrabbit/oak/query/UnionQueryTest.java | 48 ++++++++++++++++++- 2 files changed, 51 insertions(+), 5 deletions(-) diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/query/UnionQueryImpl.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/query/UnionQueryImpl.java index 321a2aa4e17..d3018c5b410 100644 --- a/oak-core/src/main/java/org/apache/jackrabbit/oak/query/UnionQueryImpl.java +++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/query/UnionQueryImpl.java @@ -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); } catch (IllegalArgumentException e) { - LOG.warn("Failed to get jcr:score for path={}", row.getPath(), e); return 0.0; } } diff --git a/oak-core/src/test/java/org/apache/jackrabbit/oak/query/UnionQueryTest.java b/oak-core/src/test/java/org/apache/jackrabbit/oak/query/UnionQueryTest.java index 3b8ae9455e2..6e6ddb64a22 100644 --- a/oak-core/src/test/java/org/apache/jackrabbit/oak/query/UnionQueryTest.java +++ b/oak-core/src/test/java/org/apache/jackrabbit/oak/query/UnionQueryTest.java @@ -471,6 +471,45 @@ public void testSortUnionQueryScoreFlagIsNull() throws Exception { assertPathOrder(results, new String[]{"/left/doc1", "/left/doc2", "/right/doc1", "/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 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 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 { NamePathMapper namePathMapper = new NamePathMapperImpl(new GlobalNameMapper(root)); @@ -512,12 +551,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; From 5aa8db0aa85072a82a0ded9db3a5e420929a2c59 Mon Sep 17 00:00:00 2001 From: marvinw Date: Mon, 2 Feb 2026 09:31:00 +0100 Subject: [PATCH 2/3] OAK-12051: Update naming, to distinguish flag after fix --- .../org/apache/jackrabbit/oak/query/QueryEngineSettings.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryEngineSettings.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryEngineSettings.java index f7304749afa..a1bd2e3b369 100644 --- a/oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryEngineSettings.java +++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryEngineSettings.java @@ -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_BY_SCORE = "FT_OAK-12051"; public static final String FT_OPTIMIZE_XPATH_UNION = "FT_OAK-12007"; From ab14b1c62412ea8ae7f664e9917c8b77881caa6a Mon Sep 17 00:00:00 2001 From: marvinw Date: Mon, 2 Feb 2026 16:22:05 +0100 Subject: [PATCH 3/3] OAK-12051: Use sorting by score as new default. --- .../java/org/apache/jackrabbit/oak/Oak.java | 13 +++++----- .../oak/query/QueryEngineSettings.java | 14 +++++------ .../jackrabbit/oak/query/UnionQueryImpl.java | 4 +-- .../jackrabbit/oak/query/UnionQueryTest.java | 25 ++++++++++--------- 4 files changed, 28 insertions(+), 28 deletions(-) diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/Oak.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/Oak.java index 66b5f523d56..50a494cddac 100644 --- a/oak-core/src/main/java/org/apache/jackrabbit/oak/Oak.java +++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/Oak.java @@ -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); @@ -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) { diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryEngineSettings.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryEngineSettings.java index a1bd2e3b369..36d9c29be24 100644 --- a/oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryEngineSettings.java +++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryEngineSettings.java @@ -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-12051"; + public static final String FT_SORT_UNION_QUERY_LEGACY_MODE = "FT_OAK-12051"; public static final String FT_OPTIMIZE_XPATH_UNION = "FT_OAK-12007"; @@ -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 = "{}"; @@ -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) { diff --git a/oak-core/src/main/java/org/apache/jackrabbit/oak/query/UnionQueryImpl.java b/oak-core/src/main/java/org/apache/jackrabbit/oak/query/UnionQueryImpl.java index d3018c5b410..2f5f30d8ad1 100644 --- a/oak-core/src/main/java/org/apache/jackrabbit/oak/query/UnionQueryImpl.java +++ b/oak-core/src/main/java/org/apache/jackrabbit/oak/query/UnionQueryImpl.java @@ -324,8 +324,8 @@ public Iterator 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); diff --git a/oak-core/src/test/java/org/apache/jackrabbit/oak/query/UnionQueryTest.java b/oak-core/src/test/java/org/apache/jackrabbit/oak/query/UnionQueryTest.java index 6e6ddb64a22..7b59606eb97 100644 --- a/oak-core/src/test/java/org/apache/jackrabbit/oak/query/UnionQueryTest.java +++ b/oak-core/src/test/java/org/apache/jackrabbit/oak/query/UnionQueryTest.java @@ -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()) @@ -439,10 +439,11 @@ 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); @@ -450,15 +451,15 @@ public void testSortUnionQueryScoreFlagDisabled() throws Exception { .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 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); @@ -466,9 +467,9 @@ public void testSortUnionQueryScoreFlagIsNull() throws Exception { .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 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