This is an automated email from the ASF dual-hosted git repository.
thomasm pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/jackrabbit-oak.git
The following commit(s) were added to refs/heads/trunk by this push:
new b9cb0d8c92 OAK-11949: Sort union queries without "order-by" by score
(#2540)
b9cb0d8c92 is described below
commit b9cb0d8c920ccc2ff503368f75ef17651158d733
Author: Marvin <[email protected]>
AuthorDate: Mon Oct 20 15:14:40 2025 +0200
OAK-11949: Sort union queries without "order-by" by score (#2540)
* OAK-11949: Sort Union Queries without order-by by score
* OAK-11936: restructure if-clause, move import and one additional test
* OAK-11949: use double for Null protection
---------
Co-authored-by: marvinw <[email protected]>
---
.../main/java/org/apache/jackrabbit/oak/Oak.java | 8 +
.../jackrabbit/oak/query/QueryEngineSettings.java | 12 ++
.../jackrabbit/oak/query/UnionQueryImpl.java | 58 +++++-
.../jackrabbit/oak/query/UnionQueryTest.java | 214 +++++++++++++++++++++
4 files changed, 290 insertions(+), 2 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 8af5260fbc..3f39239168 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
@@ -589,6 +589,10 @@ public class Oak {
LOG.info("Registered optimize in restrictions for functions
feature: " + QueryEngineSettings.FT_OPTIMIZE_IN_RESTRICTIONS_FOR_FUNCTIONS);
closer.register(optimizeInRestrictionsForFunctions);
queryEngineSettings.setOptimizeInRestrictionsForFunctions(optimizeInRestrictionsForFunctions);
+ 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);
}
return this;
@@ -1000,6 +1004,10 @@ public class Oak {
settings.setOptimizeInRestrictionsForFunctions(feature);
}
+ public void setSortUnionQueryByScoreFeature(@Nullable Feature feature)
{
+ settings.setSortUnionQueryByScoreFeature(feature);
+ }
+
@Override
public void setQueryValidatorPattern(String key, String pattern,
String comment, boolean failQuery) {
settings.getQueryValidator().setPattern(key, pattern, comment,
failQuery);
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 9527ddb927..9a143f91ec 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
@@ -67,6 +67,8 @@ public class QueryEngineSettings implements
QueryEngineSettingsMBean, QueryLimit
public static final String FT_OPTIMIZE_IN_RESTRICTIONS_FOR_FUNCTIONS =
"FT_OAK-11214";
+ public static final String FT_SORT_UNION_QUERY_BY_SCORE = "FT_OAK-11949";
+
public static final int DEFAULT_PREFETCH_COUNT =
Integer.getInteger(OAK_QUERY_PREFETCH_COUNT, -1);
public static final String OAK_QUERY_FAIL_TRAVERSAL =
"oak.queryFailTraversal";
@@ -124,6 +126,7 @@ public class QueryEngineSettings implements
QueryEngineSettingsMBean, QueryLimit
private Feature prefetchFeature;
private Feature improvedIsNullCostFeature;
private Feature optimizeInRestrictionsForFunctions;
+ private Feature sortUnionQueryByScoreFeature;
private String autoOptionsMappingJson = "{}";
private QueryOptions.AutomaticQueryOptionsMapping autoOptionsMapping = new
QueryOptions.AutomaticQueryOptionsMapping(autoOptionsMappingJson);
@@ -248,6 +251,15 @@ public class QueryEngineSettings implements
QueryEngineSettingsMBean, QueryLimit
return optimizeInRestrictionsForFunctions == null ||
optimizeInRestrictionsForFunctions.isEnabled();
}
+ public void setSortUnionQueryByScoreFeature(@Nullable Feature feature) {
+ this.sortUnionQueryByScoreFeature = feature;
+ }
+
+ public boolean isSortUnionQueryByScoreEnabled() {
+ // disable if the feature toggle is not used
+ return sortUnionQueryByScoreFeature != null &&
sortUnionQueryByScoreFeature.isEnabled();
+ }
+
public String getStrictPathRestriction() {
return strictPathRestriction.name();
}
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 894fb6d9e7..321a2aa4e1 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
@@ -313,7 +313,7 @@ public class UnionQueryImpl implements Query {
FacetMerger facetMerger = new FacetMerger(left, right);
Iterator<ResultRowImpl> it;
- final Iterator<ResultRowImpl> leftRows =
facetMerger.getLeftIterator();;
+ final Iterator<ResultRowImpl> leftRows = facetMerger.getLeftIterator();
final Iterator<ResultRowImpl> rightRows =
facetMerger.getRightIterator();
Iterator<ResultRowImpl> leftIter = leftRows;
Iterator<ResultRowImpl> rightIter = rightRows;
@@ -324,7 +324,24 @@ public class UnionQueryImpl implements Query {
rightIter = ((MeasuringIterator) rightRows).getDelegate();
}
if (orderBy == null) {
- it = IteratorUtils.chainedIterator(leftIter, rightIter);
+ if(!settings.isSortUnionQueryByScoreEnabled()) {
+ // Default old behavior
+ it = IteratorUtils.chainedIterator(leftIter, rightIter);
+ } else {
+ boolean leftHasScore = isScorePresent(left);
+ boolean rightHasScore = isScorePresent(right);
+ if (leftHasScore && rightHasScore) {
+ // Both sides have scores => merge
+ Comparator<ResultRowImpl> scoreComparator =
createScoreBasedComparator();
+ it = IteratorUtils.mergeSorted(List.of(leftIter,
rightIter), scoreComparator);
+ } else if (rightHasScore) {
+ // Only right has score => start with right
+ it = IteratorUtils.chainedIterator(rightIter, leftIter);
+ } else {
+ // only left has score, or neither has scores => start
with left
+ it = IteratorUtils.chainedIterator(leftIter, rightIter);
+ }
+ }
} else {
// This would suggest either the sub queries are sorted by index
or explicitly by QueryImpl (in case of traversing index)
// So use mergeSorted here.
@@ -527,4 +544,41 @@ public class UnionQueryImpl implements Query {
public Optional<Long> getOffset() {
return offset;
}
+
+ /**
+ * @param query the query to check.
+ * @return true if the query contains a jcr:score column, false otherwise.
+ */
+ private static boolean isScorePresent(Query query) {
+ return query.getColumnIndex("jcr:score") != -1;
+ }
+
+ /**
+ * @return a comparator to sort results by jcr:score in descending order.
+ * Precondition: {@link #isScorePresent(Query)} is true.
+ */
+ private Comparator<ResultRowImpl> createScoreBasedComparator() {
+ return new Comparator<ResultRowImpl>() {
+ @Override
+ public int compare(ResultRowImpl left, ResultRowImpl right) {
+ return Double.compare(getScoreFromRow(right),
getScoreFromRow(left));
+ }
+ };
+ }
+
+ /**
+ * @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.
+ */
+ private double getScoreFromRow(ResultRowImpl row) {
+ try {
+ PropertyValue scoreValue = row.getValue(QueryConstants.JCR_SCORE);
+ 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 969c164f32..3b8ae9455e 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
@@ -21,18 +21,22 @@ import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertTrue;
+import java.util.ArrayList;
import java.util.Arrays;
+import java.util.Iterator;
import java.util.List;
import java.util.Optional;
import org.apache.jackrabbit.oak.InitialContent;
import org.apache.jackrabbit.oak.Oak;
import org.apache.jackrabbit.oak.api.ContentRepository;
+import org.apache.jackrabbit.oak.api.PropertyValue;
import org.apache.jackrabbit.oak.api.QueryEngine;
import org.apache.jackrabbit.oak.api.Result;
import org.apache.jackrabbit.oak.api.ResultRow;
import org.apache.jackrabbit.oak.api.Tree;
import org.apache.jackrabbit.oak.api.Type;
+import org.apache.jackrabbit.oak.plugins.memory.PropertyValues;
import org.apache.jackrabbit.oak.commons.collections.ListUtils;
import org.apache.jackrabbit.oak.namepath.NamePathMapper;
import org.apache.jackrabbit.oak.namepath.impl.GlobalNameMapper;
@@ -50,6 +54,7 @@ import org.apache.jackrabbit.oak.query.ast.SelectorImpl;
import org.apache.jackrabbit.oak.query.ast.SourceImpl;
import org.apache.jackrabbit.oak.query.stats.QueryStatsData;
import org.apache.jackrabbit.oak.spi.security.OpenSecurityProvider;
+import org.apache.jackrabbit.oak.spi.toggle.Feature;
import org.junit.After;
import org.junit.Before;
import org.junit.Test;
@@ -64,6 +69,8 @@ public class UnionQueryTest extends AbstractQueryTest {
protected ContentRepository createRepository() {
store = new MemoryNodeStore();
qeSettings = new QueryEngineSettings();
+ Feature sortFeature = createFeature(true);
+ qeSettings.setSortUnionQueryByScoreFeature(sortFeature);
return new Oak(store)
.with(new OpenSecurityProvider())
@@ -362,6 +369,108 @@ public class UnionQueryTest extends AbstractQueryTest {
}
}
+ @Test
+ public void testUnionMergingBothQueryHaveScores() throws Exception {
+ 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(), qeSettings);
+ List<ScoredResult> results =
executeUnionAndGetScoredResults(unionQuery);
+ assertPathOrder(results, new String[]{"/right/doc1", "/left/doc1",
"/left/doc2", "/right/doc2"});
+ }
+
+ @Test
+ public void testUnionMergingOnlyRightQueryHasScores() throws Exception {
+ MockQueryBuilder leftQuery = new MockQueryBuilder(false)
+ .addResult("/left/doc1")
+ .addResult("/left/doc2");
+ 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", "/right/doc2",
"/left/doc1", "/left/doc2"});
+ }
+
+ @Test
+ public void testUnionMergingOnlyLeftQueryHasScores() throws Exception {
+ MockQueryBuilder leftQuery = new MockQueryBuilder(true)
+ .addResult("/left/doc1", 0.8)
+ .addResult("/left/doc2", 0.7);
+ MockQueryBuilder rightQuery = new MockQueryBuilder(false)
+ .addResult("/right/doc1")
+ .addResult("/right/doc2");
+
+ UnionQueryImpl unionQuery = new UnionQueryImpl(true,
leftQuery.build(), rightQuery.build(), qeSettings);
+ List<ScoredResult> results =
executeUnionAndGetScoredResults(unionQuery);
+ assertPathOrder(results, new String[]{"/left/doc1", "/left/doc2",
"/right/doc1", "/right/doc2"});
+ }
+
+ @Test
+ public void testUnionMergingNeitherQueryHasScores() throws Exception {
+ MockQueryBuilder leftQuery = new MockQueryBuilder(false)
+ .addResult("/left/doc1")
+ .addResult("/left/doc2");
+ MockQueryBuilder rightQuery = new MockQueryBuilder(false)
+ .addResult("/right/doc1")
+ .addResult("/right/doc2");
+
+ UnionQueryImpl unionQuery = new UnionQueryImpl(true,
leftQuery.build(), rightQuery.build(), qeSettings);
+ List<ScoredResult> results =
executeUnionAndGetScoredResults(unionQuery);
+ assertPathOrder(results, new String[]{"/left/doc1", "/left/doc2",
"/right/doc1", "/right/doc2"});
+ }
+
+ @Test
+ public void testUnionMergingEmptyIterators() throws Exception {
+ MockQueryBuilder leftQuery = new MockQueryBuilder(true);
+ MockQueryBuilder rightQuery = new MockQueryBuilder(true)
+ .addResult("/right/doc1", 0.9);
+
+ UnionQueryImpl unionQuery = new UnionQueryImpl(true,
leftQuery.build(), rightQuery.build(), qeSettings);
+ List<ScoredResult> results =
executeUnionAndGetScoredResults(unionQuery);
+
+ assertEquals(1, results.size());
+ assertEquals("/right/doc1", results.get(0).path);
+ }
+
+ @Test
+ public void testSortUnionQueryScoreFlagDisabled() throws Exception {
+ QueryEngineSettings disabledSettings = new QueryEngineSettings();
+ Feature sortFeature = createFeature(false);
+ disabledSettings.setSortUnionQueryByScoreFeature(sortFeature);
+ 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);
+ 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);
+ 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);
+ List<ScoredResult> results =
executeUnionAndGetScoredResults(unionQuery);
+ assertPathOrder(results, new String[]{"/left/doc1", "/left/doc2",
"/right/doc1", "/right/doc2"});
+ }
+
private QueryImpl createQuery (String statement, ConstraintImpl c,
SourceImpl sImpl) throws Exception {
NamePathMapper namePathMapper = new NamePathMapperImpl(new
GlobalNameMapper(root));
@@ -369,4 +478,109 @@ public class UnionQueryTest extends AbstractQueryTest {
new ColumnImpl[]{new ColumnImpl("a", "jcr:path", "jcr:path")},
namePathMapper, qeSettings, new QueryStatsData("", "").new
QueryExecutionStats());
}
+
+ /**
+ * Builder class for creating mock queries with controllable scores
+ */
+ private static class MockQueryBuilder {
+ private final boolean hasScore;
+ private final List<ResultRowImpl> results = new ArrayList<>();
+ private final QueryImpl mockQuery;
+ private final ColumnImpl[] columns;
+
+ public MockQueryBuilder(boolean hasScore) {
+ this.hasScore = hasScore;
+ this.mockQuery = Mockito.mock(QueryImpl.class);
+
+ if (hasScore) {
+ this.columns = new ColumnImpl[] {
+ new ColumnImpl("a", "jcr:path", "jcr:path"),
+ new ColumnImpl("a", "jcr:score", "jcr:score")
+ };
+ // return path at position 0, and score at 1
+
Mockito.when(mockQuery.getColumnIndex("jcr:path")).thenReturn(0);
+
Mockito.when(mockQuery.getColumnIndex("jcr:score")).thenReturn(1);
+ } else {
+ this.columns = new ColumnImpl[] {
+ new ColumnImpl("a", "jcr:path", "jcr:path")
+ };
+ // return path at position 0, and score can't be found
+
Mockito.when(mockQuery.getColumnIndex("jcr:path")).thenReturn(0);
+
Mockito.when(mockQuery.getColumnIndex("jcr:score")).thenReturn(-1);
+ }
+ Mockito.when(mockQuery.getColumns()).thenReturn(columns);
+ }
+
+ 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)
+ };
+ results.add(new ResultRowImpl(mockQuery, null, values, null,
null));
+ return this;
+ }
+
+ public MockQueryBuilder addResult(String path) {
+ if (hasScore) {
+ throw new IllegalStateException("Must provide score");
+ }
+ PropertyValue[] values = new PropertyValue[] {
+ PropertyValues.newString(path)
+ };
+ results.add(new ResultRowImpl(mockQuery, null, values, null,
null));
+ return this;
+ }
+
+ public QueryImpl build() {
+ Mockito.when(mockQuery.getRows()).thenReturn(results.iterator());
+ return mockQuery;
+ }
+ }
+
+ /**
+ * Helper class to hold path and according score
+ */
+ private static class ScoredResult {
+ final String path;
+ final Double score;
+ ScoredResult(String path, Double score) {
+ this.path = path;
+ this.score = score;
+ }
+ }
+
+ private List<ScoredResult> executeUnionAndGetScoredResults(UnionQueryImpl
unionQuery) throws Exception {
+ Iterator<ResultRowImpl> results = unionQuery.getRows();
+ List<ScoredResult> scoredResults = new ArrayList<>();
+
+ while (results.hasNext()) {
+ ResultRowImpl row = results.next();
+ String path = row.getValue("jcr:path").getValue(Type.STRING);
+ try {
+ PropertyValue scoreValue = row.getValue("jcr:score");
+ Double score = scoreValue.getValue(Type.DOUBLE);
+ scoredResults.add(new ScoredResult(path, score));
+ } catch (Exception e) {
+ scoredResults.add(new ScoredResult(path, null));
+ }
+ }
+
+ return scoredResults;
+ }
+
+ private void assertPathOrder(List<ScoredResult> results, String[]
expectedPaths) {
+ assertEquals(expectedPaths.length, results.size());
+ for (int i = 0; i < expectedPaths.length; i++) {
+ assertEquals(expectedPaths[i], results.get(i).path);
+ }
+ }
+
+ private static Feature createFeature(boolean enabled) {
+ Feature feature = Mockito.mock(Feature.class);
+ Mockito.when(feature.isEnabled()).thenReturn(enabled);
+ return feature;
+ }
}
