From 6c94ca9cb33795cdc29797ff2d17f1869813d3f9 Mon Sep 17 00:00:00 2001 From: Mike Drob Date: Fri, 4 Sep 2020 09:46:03 -0500 Subject: [PATCH] LUCENE-9451 Sort.rewrite does not always return this when unchanged (#1731) --- .../lucene/search/DoubleValuesSource.java | 11 +++++---- .../lucene/search/LongValuesSource.java | 6 ++++- .../java/org/apache/lucene/search/Sort.java | 2 +- .../lucene/search/TestDoubleValuesSource.java | 5 ++++ .../lucene/search/TestLongValuesSource.java | 6 +++++ .../org/apache/lucene/search/TestSort.java | 24 ++++++++++++++++--- 6 files changed, 45 insertions(+), 9 deletions(-) diff --git a/lucene/core/src/java/org/apache/lucene/search/DoubleValuesSource.java b/lucene/core/src/java/org/apache/lucene/search/DoubleValuesSource.java index 106069d3f34..1c69edf5373 100644 --- a/lucene/core/src/java/org/apache/lucene/search/DoubleValuesSource.java +++ b/lucene/core/src/java/org/apache/lucene/search/DoubleValuesSource.java @@ -275,12 +275,12 @@ public abstract class DoubleValuesSource implements SegmentCacheable { this.value = value; this.doubleValues = new DoubleValues() { @Override - public double doubleValue() throws IOException { + public double doubleValue() { return value; } @Override - public boolean advanceExact(int doc) throws IOException { + public boolean advanceExact(int doc) { return true; } }; @@ -456,13 +456,16 @@ public abstract class DoubleValuesSource implements SegmentCacheable { @Override public SortField rewrite(IndexSearcher searcher) throws IOException { - DoubleValuesSortField rewritten = new DoubleValuesSortField(producer.rewrite(searcher), reverse); + DoubleValuesSource rewrittenSource = producer.rewrite(searcher); + if (rewrittenSource == producer) { + return this; + } + DoubleValuesSortField rewritten = new DoubleValuesSortField(rewrittenSource, reverse); if (missingValue != null) { rewritten.setMissingValue(missingValue); } return rewritten; } - } private static class DoubleValuesHolder { diff --git a/lucene/core/src/java/org/apache/lucene/search/LongValuesSource.java b/lucene/core/src/java/org/apache/lucene/search/LongValuesSource.java index 1eafc224a2b..54f8f19de5b 100644 --- a/lucene/core/src/java/org/apache/lucene/search/LongValuesSource.java +++ b/lucene/core/src/java/org/apache/lucene/search/LongValuesSource.java @@ -305,7 +305,11 @@ public abstract class LongValuesSource implements SegmentCacheable { @Override public SortField rewrite(IndexSearcher searcher) throws IOException { - LongValuesSortField rewritten = new LongValuesSortField(producer.rewrite(searcher), reverse); + LongValuesSource rewrittenSource = producer.rewrite(searcher); + if (producer == rewrittenSource) { + return this; + } + LongValuesSortField rewritten = new LongValuesSortField(rewrittenSource, reverse); if (missingValue != null) { rewritten.setMissingValue(missingValue); } diff --git a/lucene/core/src/java/org/apache/lucene/search/Sort.java b/lucene/core/src/java/org/apache/lucene/search/Sort.java index 3af6fa4c180..f09fcc953da 100644 --- a/lucene/core/src/java/org/apache/lucene/search/Sort.java +++ b/lucene/core/src/java/org/apache/lucene/search/Sort.java @@ -182,7 +182,7 @@ public class Sort { } } - return (changed) ? new Sort(rewrittenSortFields) : this; + return changed ? new Sort(rewrittenSortFields) : this; } @Override diff --git a/lucene/core/src/test/org/apache/lucene/search/TestDoubleValuesSource.java b/lucene/core/src/test/org/apache/lucene/search/TestDoubleValuesSource.java index 77cebbf026a..c9ab80d0311 100644 --- a/lucene/core/src/test/org/apache/lucene/search/TestDoubleValuesSource.java +++ b/lucene/core/src/test/org/apache/lucene/search/TestDoubleValuesSource.java @@ -284,6 +284,11 @@ public class TestDoubleValuesSource extends LuceneTestCase { doTestQueryDoubleValuesSources(approximatingQuery); } + public void testRewriteSame() throws IOException { + SortField doubleField = DoubleValuesSource.constant(1.0).getSortField(false); + assertSame(doubleField, doubleField.rewrite(searcher)); + } + private void doTestQueryDoubleValuesSources(Query q) throws Exception { DoubleValuesSource vs = DoubleValuesSource.fromQuery(q).rewrite(searcher); searcher.search(q, new SimpleCollector() { diff --git a/lucene/core/src/test/org/apache/lucene/search/TestLongValuesSource.java b/lucene/core/src/test/org/apache/lucene/search/TestLongValuesSource.java index 6380b732e68..fd96f65d886 100644 --- a/lucene/core/src/test/org/apache/lucene/search/TestLongValuesSource.java +++ b/lucene/core/src/test/org/apache/lucene/search/TestLongValuesSource.java @@ -17,6 +17,7 @@ package org.apache.lucene.search; +import java.io.IOException; import java.util.Arrays; import java.util.Collections; @@ -129,6 +130,11 @@ public class TestLongValuesSource extends LuceneTestCase { } } + public void testRewriteSame() throws IOException { + SortField longField = LongValuesSource.constant(1L).getSortField(false); + assertSame(longField, longField.rewrite(searcher)); + } + Sort randomSort() throws Exception { boolean reversed = random().nextBoolean(); SortField fields[] = new SortField[] { diff --git a/lucene/core/src/test/org/apache/lucene/search/TestSort.java b/lucene/core/src/test/org/apache/lucene/search/TestSort.java index d37c98ddc91..787705afb82 100644 --- a/lucene/core/src/test/org/apache/lucene/search/TestSort.java +++ b/lucene/core/src/test/org/apache/lucene/search/TestSort.java @@ -56,9 +56,9 @@ public class TestSort extends LuceneTestCase { } private void assertDifferent(Sort a, Sort b) { - assertFalse(a.equals(b)); - assertFalse(b.equals(a)); - assertFalse(a.hashCode() == b.hashCode()); + assertNotEquals(a, b); + assertNotEquals(b, a); + assertNotEquals(a.hashCode(), b.hashCode()); } public void testEquals() { @@ -878,4 +878,22 @@ public class TestSort extends LuceneTestCase { ir.close(); dir.close(); } + + public void testRewrite() throws IOException { + try (Directory dir = newDirectory()) { + RandomIndexWriter writer = new RandomIndexWriter(random(), dir); + IndexSearcher searcher = newSearcher(writer.getReader()); + writer.close(); + + LongValuesSource longSource = LongValuesSource.constant(1L); + Sort sort = new Sort(longSource.getSortField(false)); + + assertSame(sort, sort.rewrite(searcher)); + + DoubleValuesSource doubleSource = DoubleValuesSource.constant(1.0); + sort = new Sort(doubleSource.getSortField(false)); + + assertSame(sort, sort.rewrite(searcher)); + } + } }