keep one bock loader impl and fixed long script unit test

martijnvg · Dec 2, 2024 · 7be360c · 7be360c
1 parent fdee715
commit 7be360c
Show file tree

Hide file tree

Showing 4 changed files with 24 additions and 30 deletions.
diff --git a/server/src/main/java/org/elasticsearch/index/mapper/LongScriptBlockDocValuesReader.java b/server/src/main/java/org/elasticsearch/index/mapper/LongScriptBlockDocValuesReader.java
@@ -18,7 +18,7 @@
  * {@link BlockDocValuesReader} implementation for {@code long} scripts.
  */
 public class LongScriptBlockDocValuesReader extends BlockDocValuesReader {
-    static class LongScriptBlockLoader extends DocValuesBlockLoader {
+    static class LongScriptBlockLoader extends AbstractScriptBlockLoader {
         private final LongFieldScript.LeafFactory factory;
 
         LongScriptBlockLoader(LongFieldScript.LeafFactory factory) {
@@ -31,30 +31,11 @@ public Builder builder(BlockFactory factory, int expectedCount) {
         }
 
         @Override
-        public AllReader reader(LeafReaderContext context) throws IOException {
+        public AllReader rowStrideReader(LeafReaderContext context) throws IOException {
             return new LongScriptBlockDocValuesReader(factory.newInstance(context));
         }
     }
 
-    static class RowStrideLongScriptBlockLoader extends AbstractScriptBlockLoader {
-        private final LongFieldScript.LeafFactory factory;
-
-        RowStrideLongScriptBlockLoader(LongFieldScript.LeafFactory factory) {
-            this.factory = factory;
-        }
-
-        @Override
-        public Builder builder(BlockFactory factory, int expectedCount) {
-            return factory.longs(expectedCount);
-        }
-
-        @Override
-        public RowStrideReader rowStrideReader(LeafReaderContext context) throws IOException {
-            return new LongScriptBlockDocValuesReader(factory.newInstance(context));
-        }
-
-    }
-
     private final LongFieldScript script;
     private int docId;
 

diff --git a/server/src/main/java/org/elasticsearch/index/mapper/LongScriptFieldType.java b/server/src/main/java/org/elasticsearch/index/mapper/LongScriptFieldType.java
@@ -108,12 +108,7 @@ public DocValueFormat docValueFormat(String format, ZoneId timeZone) {
 
     @Override
     public BlockLoader blockLoader(BlockLoaderContext blContext) {
-        boolean isSyntheticSource = SourceFieldMapper.isSynthetic(blContext.indexSettings());
-        if (isSyntheticSource) {
-            return new LongScriptBlockDocValuesReader.RowStrideLongScriptBlockLoader(leafFactory(blContext.lookup()));
-        } else {
-            return new LongScriptBlockDocValuesReader.LongScriptBlockLoader(leafFactory(blContext.lookup()));
-        }
+        return new LongScriptBlockDocValuesReader.LongScriptBlockLoader(leafFactory(blContext.lookup()));
     }
 
     @Override

diff --git a/server/src/test/java/org/elasticsearch/index/mapper/LongScriptFieldTypeTests.java b/server/src/test/java/org/elasticsearch/index/mapper/LongScriptFieldTypeTests.java
@@ -295,7 +295,7 @@ public void testBlockLoader() throws IOException {
             );
             try (DirectoryReader reader = iw.getReader()) {
                 LongScriptFieldType fieldType = build("add_param", Map.of("param", 1), OnScriptError.FAIL);
-                assertThat(blockLoaderReadValuesFromColumnAtATimeReader(reader, fieldType), equalTo(List.of(2L, 3L)));
+                assertThat(hasNullColumnarReader(reader, fieldType), equalTo(true));
                 assertThat(blockLoaderReadValuesFromRowStrideReader(reader, fieldType), equalTo(List.of(2L, 3L)));
             }
         }

diff --git a/...amework/src/main/java/org/elasticsearch/index/mapper/AbstractScriptFieldTypeTestCase.java b/...amework/src/main/java/org/elasticsearch/index/mapper/AbstractScriptFieldTypeTestCase.java
@@ -25,8 +25,10 @@
 import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.bytes.BytesReference;
 import org.elasticsearch.common.geo.ShapeRelation;
+import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.xcontent.XContentHelper;
 import org.elasticsearch.index.IndexSettings;
+import org.elasticsearch.index.IndexVersion;
 import org.elasticsearch.index.fielddata.FieldDataContext;
 import org.elasticsearch.index.fielddata.IndexFieldDataCache;
 import org.elasticsearch.index.query.ExistsQueryBuilder;
@@ -314,6 +316,8 @@ protected static SearchExecutionContext mockContext(
                 .build(new IndexFieldDataCache.None(), new NoneCircuitBreakerService());
         });
         when(context.getMatchingFieldNames(any())).thenReturn(Set.of("dummy_field"));
+        var indexSettings = createIndexSettings(IndexVersion.current(), Settings.EMPTY);
+        when(context.getIndexSettings()).thenReturn(indexSettings);
         return context;
     }
 
@@ -430,6 +434,19 @@ protected final List<Object> blockLoaderReadValuesFromColumnAtATimeReader(Direct
         return all;
     }
 
+    protected final boolean hasNullColumnarReader(DirectoryReader reader, MappedFieldType fieldType)
+        throws IOException {
+        BlockLoader loader = fieldType.blockLoader(blContext());
+        for (LeafReaderContext ctx : reader.leaves()) {
+            TestBlock block = (TestBlock) loader.columnAtATimeReader(ctx);
+            if (block != null) {
+                return false;
+            }
+        }
+
+        return true;
+    }
+
     protected final List<Object> blockLoaderReadValuesFromRowStrideReader(DirectoryReader reader, MappedFieldType fieldType)
         throws IOException {
         BlockLoader loader = fieldType.blockLoader(blContext());
@@ -449,6 +466,7 @@ protected final List<Object> blockLoaderReadValuesFromRowStrideReader(DirectoryR
     }
 
     private MappedFieldType.BlockLoaderContext blContext() {
+        var mockContext = mockContext();
         return new MappedFieldType.BlockLoaderContext() {
             @Override
             public String indexName() {
@@ -457,7 +475,7 @@ public String indexName() {
 
             @Override
             public IndexSettings indexSettings() {
-                throw new UnsupportedOperationException();
+                return mockContext.getIndexSettings();
             }
 
             @Override
@@ -467,7 +485,7 @@ public MappedFieldType.FieldExtractPreference fieldExtractPreference() {
 
             @Override
             public SearchLookup lookup() {
-                return mockContext().lookup();
+                return mockContext.lookup();
             }
 
             @Override