Enable Point based optimization for custom comparators (#8168)

* Enable Point based optimization for custom comparators Signed-off-by: gashutos <[email protected]> * Simplifying enableSkipping logic in IndexField Signed-off-by: gashutos <[email protected]> * Empty commit Signed-off-by: gashutos <[email protected]> * Update server/src/main/java/org/opensearch/index/fielddata/IndexNumericFieldData.java Co-authored-by: Andrew Ross <[email protected]> Signed-off-by: Chaitanya Gohel <[email protected]> --------- Signed-off-by: gashutos <[email protected]> Signed-off-by: Chaitanya Gohel <[email protected]> Co-authored-by: Andrew Ross <[email protected]>
opensearch-project · Jun 29, 2023 · 87a833f · 87a833f
1 parent 544b1ca
commit 87a833f
Show file tree

Hide file tree

Showing 8 changed files with 30 additions and 12 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -92,6 +92,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Make remote cluster connection setup in async ([#8038](https://github.com/opensearch-project/OpenSearch/pull/8038))
 - Add API to initialize extensions ([#8029]()https://github.com/opensearch-project/OpenSearch/pull/8029)
 - Add distributed tracing framework ([#7543](https://github.com/opensearch-project/OpenSearch/issues/7543))
+- Enable Point based optimization for custom comparators ([#8168](https://github.com/opensearch-project/OpenSearch/pull/8168))
 
 ### Dependencies
 - Bump `com.azure:azure-storage-common` from 12.21.0 to 12.21.1 (#7566, #7814)

diff --git a/server/src/main/java/org/opensearch/index/fielddata/IndexFieldData.java b/server/src/main/java/org/opensearch/index/fielddata/IndexFieldData.java
@@ -120,11 +120,13 @@ abstract class XFieldComparatorSource extends FieldComparatorSource {
         protected final MultiValueMode sortMode;
         protected final Object missingValue;
         protected final Nested nested;
+        protected boolean enableSkipping;
 
         public XFieldComparatorSource(Object missingValue, MultiValueMode sortMode, Nested nested) {
             this.sortMode = sortMode;
             this.missingValue = missingValue;
             this.nested = nested;
+            this.enableSkipping = true; // true by default
         }
 
         public MultiValueMode sortMode() {
@@ -135,6 +137,10 @@ public Nested nested() {
             return this.nested;
         }
 
+        public void disableSkipping() {
+            this.enableSkipping = false;
+        }
+
         /**
          * Simple wrapper class around a filter that matches parent documents
          * and a filter that matches child documents. For every root document R,

diff --git a/server/src/main/java/org/opensearch/index/fielddata/IndexNumericFieldData.java b/server/src/main/java/org/opensearch/index/fielddata/IndexNumericFieldData.java
@@ -198,24 +198,35 @@ private XFieldComparatorSource comparatorSource(
         MultiValueMode sortMode,
         Nested nested
     ) {
+        final XFieldComparatorSource source;
         switch (targetNumericType) {
             case HALF_FLOAT:
             case FLOAT:
-                return new FloatValuesComparatorSource(this, missingValue, sortMode, nested);
+                source = new FloatValuesComparatorSource(this, missingValue, sortMode, nested);
+                break;
             case DOUBLE:
-                return new DoubleValuesComparatorSource(this, missingValue, sortMode, nested);
+                source = new DoubleValuesComparatorSource(this, missingValue, sortMode, nested);
+                break;
             case UNSIGNED_LONG:
-                return new UnsignedLongValuesComparatorSource(this, missingValue, sortMode, nested);
+                source = new UnsignedLongValuesComparatorSource(this, missingValue, sortMode, nested);
+                break;
             case DATE:
-                return dateComparatorSource(missingValue, sortMode, nested);
+                source = dateComparatorSource(missingValue, sortMode, nested);
+                break;
             case DATE_NANOSECONDS:
-                return dateNanosComparatorSource(missingValue, sortMode, nested);
+                source = dateNanosComparatorSource(missingValue, sortMode, nested);
+                break;
             case LONG:
-                return new LongValuesComparatorSource(this, missingValue, sortMode, nested);
+                source = new LongValuesComparatorSource(this, missingValue, sortMode, nested);
+                break;
             default:
                 assert !targetNumericType.isFloatingPoint();
-                return new IntValuesComparatorSource(this, missingValue, sortMode, nested);
+                source = new IntValuesComparatorSource(this, missingValue, sortMode, nested);
         }
+        if (targetNumericType != getNumericType()) {
+            source.disableSkipping(); // disable skipping logic for caste of sort field
+        }
+        return source;
     }
 
     protected XFieldComparatorSource dateComparatorSource(@Nullable Object missingValue, MultiValueMode sortMode, Nested nested) {

diff --git a/...ain/java/org/opensearch/index/fielddata/fieldcomparator/DoubleValuesComparatorSource.java b/...ain/java/org/opensearch/index/fielddata/fieldcomparator/DoubleValuesComparatorSource.java
@@ -104,7 +104,7 @@ public FieldComparator<?> newComparator(String fieldname, int numHits, boolean e
         final double dMissingValue = (Double) missingObject(missingValue, reversed);
         // NOTE: it's important to pass null as a missing value in the constructor so that
         // the comparator doesn't check docsWithField since we replace missing values in select()
-        return new DoubleComparator(numHits, null, null, reversed, false) {
+        return new DoubleComparator(numHits, fieldname, null, reversed, enableSkipping && this.enableSkipping) {
             @Override
             public LeafFieldComparator getLeafComparator(LeafReaderContext context) throws IOException {
                 return new DoubleLeafComparator(context) {

diff --git a/...main/java/org/opensearch/index/fielddata/fieldcomparator/FloatValuesComparatorSource.java b/...main/java/org/opensearch/index/fielddata/fieldcomparator/FloatValuesComparatorSource.java
@@ -97,7 +97,7 @@ public FieldComparator<?> newComparator(String fieldname, int numHits, boolean e
         final float fMissingValue = (Float) missingObject(missingValue, reversed);
         // NOTE: it's important to pass null as a missing value in the constructor so that
         // the comparator doesn't check docsWithField since we replace missing values in select()
-        return new FloatComparator(numHits, null, null, reversed, false) {
+        return new FloatComparator(numHits, fieldname, null, reversed, enableSkipping && this.enableSkipping) {
             @Override
             public LeafFieldComparator getLeafComparator(LeafReaderContext context) throws IOException {
                 return new FloatLeafComparator(context) {

diff --git a/...c/main/java/org/opensearch/index/fielddata/fieldcomparator/IntValuesComparatorSource.java b/...c/main/java/org/opensearch/index/fielddata/fieldcomparator/IntValuesComparatorSource.java
@@ -76,7 +76,7 @@ public FieldComparator<?> newComparator(String fieldname, int numHits, boolean e
         final int iMissingValue = (Integer) missingObject(missingValue, reversed);
         // NOTE: it's important to pass null as a missing value in the constructor so that
         // the comparator doesn't check docsWithField since we replace missing values in select()
-        return new IntComparator(numHits, null, null, reversed, false) {
+        return new IntComparator(numHits, fieldname, null, reversed, enableSkipping && this.enableSkipping) {
             @Override
             public LeafFieldComparator getLeafComparator(LeafReaderContext context) throws IOException {
                 return new IntLeafComparator(context) {

diff --git a/.../main/java/org/opensearch/index/fielddata/fieldcomparator/LongValuesComparatorSource.java b/.../main/java/org/opensearch/index/fielddata/fieldcomparator/LongValuesComparatorSource.java
@@ -120,7 +120,7 @@ public FieldComparator<?> newComparator(String fieldname, int numHits, boolean e
         final long lMissingValue = (Long) missingObject(missingValue, reversed);
         // NOTE: it's important to pass null as a missing value in the constructor so that
         // the comparator doesn't check docsWithField since we replace missing values in select()
-        return new LongComparator(numHits, null, null, reversed, false) {
+        return new LongComparator(numHits, fieldname, null, reversed, enableSkipping && this.enableSkipping) {
             @Override
             public LeafFieldComparator getLeafComparator(LeafReaderContext context) throws IOException {
                 return new LongLeafComparator(context) {

diff --git a/...va/org/opensearch/index/fielddata/fieldcomparator/UnsignedLongValuesComparatorSource.java b/...va/org/opensearch/index/fielddata/fieldcomparator/UnsignedLongValuesComparatorSource.java
@@ -92,7 +92,7 @@ public FieldComparator<?> newComparator(String fieldname, int numHits, boolean e
         assert indexFieldData == null || fieldname.equals(indexFieldData.getFieldName());
 
         final BigInteger ulMissingValue = (BigInteger) missingObject(missingValue, reversed);
-        return new UnsignedLongComparator(numHits, null, null, reversed, false) {
+        return new UnsignedLongComparator(numHits, fieldname, null, reversed, enableSkipping && this.enableSkipping) {
             @Override
             public LeafFieldComparator getLeafComparator(LeafReaderContext context) throws IOException {
                 return new UnsignedLongLeafComparator(context) {