Fixed a bug that does not get a space type from index setting when it…

… is empty for compatibility. Signed-off-by: Dooyong Kim <[email protected]>
opensearch-project · Dec 3, 2024 · 9518c52 · 9518c52
1 parent 36ed563
commit 9518c52
Show file tree

Hide file tree

Showing 5 changed files with 116 additions and 58 deletions.
diff --git a/.github/workflows/CI.yml b/.github/workflows/CI.yml
@@ -30,8 +30,6 @@ on:
       - 'jni/**'
       - 'micro-benchmarks/**'
       - '.github/workflows/CI.yml'
-env:
-  ACTIONS_ALLOW_USE_UNSECURE_NODE_VERSION: true
 
 jobs:
   Get-CI-Image-Tag:
@@ -42,7 +40,7 @@ jobs:
   Build-k-NN-Linux:
     strategy:
       matrix:
-        java: [11, 17, 21]
+        java: [21]
 
     name: Build and Test k-NN Plugin on Linux
     runs-on: ubuntu-latest
@@ -52,11 +50,14 @@ jobs:
       # this image tag is subject to change as more dependencies and updates will arrive over time
       image: ${{ needs.Get-CI-Image-Tag.outputs.ci-image-version-linux }}
       # need to switch to root so that github actions can install runner binary on container without permission issues.
-      options: --user root
+      options: ${{ needs.Get-CI-Image-Tag.outputs.ci-image-start-options }}
 
     steps:
+      - name: Run start commands
+        run: ${{ needs.Get-CI-Image-Tag.outputs.ci-image-start-command }}
+
       - name: Checkout k-NN
-        uses: actions/checkout@v1
+        uses: actions/checkout@v4
 
       # Setup git user so that patches for native libraries can be applied and committed
       - name: Setup git user
@@ -65,41 +66,46 @@ jobs:
           su `id -un 1000` -c 'git config --global user.email "github-actions[bot]@users.noreply.github.com"'
 
       - name: Setup Java ${{ matrix.java }}
-        uses: actions/setup-java@v1
+        uses: actions/setup-java@v4
         with:
           java-version: ${{ matrix.java }}
+          distribution: 'temurin'
 
       - name: Run build
         # switching the user, as OpenSearch cluster can only be started as root/Administrator on linux-deb/linux-rpm/windows-zip.
         run: |
           chown -R 1000:1000 `pwd`
-          if lscpu  | grep -i avx2
+          if lscpu  | grep -i avx512f | grep -i avx512cd | grep -i avx512vl | grep -i avx512dq | grep -i avx512bw          
           then
-            echo "avx2 available on system"
+            echo "avx512 available on system"
             su `id -un 1000` -c "whoami && java -version && ./gradlew build -Dnproc.count=`nproc`"
+          elif lscpu  | grep -i avx2
+          then
+            echo "avx2 available on system"
+            su `id -un 1000` -c "whoami && java -version && ./gradlew build -Dnproc.count=`nproc` -Davx512.enabled=false"
           else
-            echo "avx2 not available on system"
-            su `id -un 1000` -c "whoami && java -version && ./gradlew build -Dsimd.enabled=false -Dnproc.count=`nproc`"
+            echo "avx512 and avx2 not available on system"
+            su `id -un 1000` -c "whoami && java -version && ./gradlew build -Davx2.enabled=false -Davx512.enabled=false -Dnproc.count=`nproc`"
           fi  
 
 
       - name: Upload Coverage Report
-        uses: codecov/codecov-action@v1
+        uses: codecov/codecov-action@v4
         with:
           token: ${{ secrets.CODECOV_TOKEN }}
 
   Build-k-NN-MacOS:
     strategy:
       matrix:
-        java: [ 11, 17, 21 ]
+        java: [ 21 ]
 
     name: Build and Test k-NN Plugin on MacOS
     needs: Get-CI-Image-Tag
-    runs-on: macos-12
+    runs-on: macos-13
 
     steps:
       - name: Checkout k-NN
-        uses: actions/checkout@v1
+        uses: actions/checkout@v4
 
       # Setup git user so that patches for native libraries can be applied and committed
       - name: Setup git user
@@ -108,12 +114,14 @@ jobs:
           git config --global user.email "github-actions[bot]@users.noreply.github.com"
 
       - name: Setup Java ${{ matrix.java }}
-        uses: actions/setup-java@v1
+        uses: actions/setup-java@v4
         with:
           java-version: ${{ matrix.java }}
+          distribution: 'temurin'
 
       - name: Install dependencies on macos
         run: |
+          brew install libomp
           brew reinstall gcc
           export FC=/usr/local/Cellar/gcc/12.2.0/bin/gfortran
 
@@ -126,21 +134,21 @@ jobs:
               ./gradlew build -Dnproc.count=3
           else
               echo "avx2 not available on system"
-              ./gradlew build -Dsimd.enabled=false -Dnproc.count=3
+              ./gradlew build -Davx2.enabled=false -Davx512.enabled=false -Dnproc.count=3
           fi
 
   Build-k-NN-Windows:
     strategy:
       matrix:
-        java: [ 11, 17, 21 ]
+        java: [ 21 ]
 
     name: Build and Test k-NN Plugin on Windows
     needs: Get-CI-Image-Tag
     runs-on: windows-latest
 
     steps:
       - name: Checkout k-NN
-        uses: actions/checkout@v1
+        uses: actions/checkout@v4
 
       # Setup git user so that patches for native libraries can be applied and committed
       - name: Setup git user
@@ -149,9 +157,10 @@ jobs:
           git config --global user.email "github-actions[bot]@users.noreply.github.com"
 
       - name: Setup Java ${{ matrix.java }}
-        uses: actions/setup-java@v1
+        uses: actions/setup-java@v4
         with:
           java-version: ${{ matrix.java }}
+          distribution: 'temurin'
 
       - name: Install MinGW Using Scoop
         run: |
@@ -187,4 +196,4 @@ jobs:
       # TODO: Detect processor count and set the value of nproc.count
       - name: Run build
         run: |
-          ./gradlew.bat build -D'simd.enabled=false' -D'nproc.count=4'
+          ./gradlew.bat build -D'avx2.enabled=false' -D'avx512.enabled=false' -D'nproc.count=4'
diff --git a/.github/workflows/test_security.yml b/.github/workflows/test_security.yml
@@ -28,8 +28,6 @@ on:
       - 'gradle/**'
       - 'jni/**'
       - '.github/workflows/test_security.yml'
-env:
-  ACTIONS_ALLOW_USE_UNSECURE_NODE_VERSION: true
 
 jobs:
   Get-CI-Image-Tag:
@@ -50,24 +48,38 @@ jobs:
       # this image tag is subject to change as more dependencies and updates will arrive over time
       image: ${{ needs.Get-CI-Image-Tag.outputs.ci-image-version-linux }}
       # need to switch to root so that github actions can install runner binary on container without permission issues.
-      options: --user root
+      options: ${{ needs.Get-CI-Image-Tag.outputs.ci-image-start-options }}
 
     steps:
+      - name: Run start commands
+        run: ${{ needs.Get-CI-Image-Tag.outputs.ci-image-start-command }}
       - name: Checkout k-NN
-        uses: actions/checkout@v1
+        uses: actions/checkout@v4
       # Setup git user so that patches for native libraries can be applied and committed
       - name: Setup git user
         run: |
           su `id -un 1000` -c 'git config --global user.name "github-actions[bot]"'
           su `id -un 1000` -c 'git config --global user.email "github-actions[bot]@users.noreply.github.com"'
 
       - name: Setup Java ${{ matrix.java }}
-        uses: actions/setup-java@v1
+        uses: actions/setup-java@v4
         with:
           java-version: ${{ matrix.java }}
+          distribution: 'temurin'
 
       - name: Run build
         # switching the user, as OpenSearch cluster can only be started as root/Administrator on linux-deb/linux-rpm/windows-zip.
         run: |
           chown -R 1000:1000 `pwd`
-          su `id -un 1000` -c "whoami && java -version && ./gradlew integTest -Dsecurity.enabled=true -Dsimd.enabled=true -Dnproc.count=`nproc`"
+          if lscpu  | grep -i avx512f | grep -i avx512cd | grep -i avx512vl | grep -i avx512dq | grep -i avx512bw          
+          then
+            echo "avx512 available on system"
+            su `id -un 1000` -c "whoami && java -version && ./gradlew build -Dnproc.count=`nproc`"
+          elif lscpu  | grep -i avx2
+          then
+            echo "avx2 available on system"
+            su `id -un 1000` -c "whoami && java -version && ./gradlew build -Dnproc.count=`nproc` -Davx512.enabled=false"
+          else
+            echo "avx512 and avx2 not available on system"
+            su `id -un 1000` -c "whoami && java -version && ./gradlew build -Davx2.enabled=false -Davx512.enabled=false -Dnproc.count=`nproc`"
+          fi  
diff --git a/src/main/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapper.java b/src/main/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapper.java
@@ -385,7 +385,7 @@ public Mapper.Builder<?> parse(String name, Map<String, Object> node, ParserCont
                     SpaceTypeResolver.INSTANCE.pickDefaultSpaceTypeWhenEmpty(resolvedSpaceType, builder.vectorDataType.get())
                 );
                 validateSpaceType(builder);
-                resolveKNNMethodComponents(builder, parserContext, resolvedSpaceType);
+                resolveKNNMethodComponents(builder, parserContext, resolvedSpaceType, builder.vectorDataType.get());
                 validateFromKNNMethod(builder);
             }
 
@@ -476,9 +476,10 @@ private void validateCompressionAndModeNotSet(KNNVectorFieldMapper.Builder build
         }
 
         private void resolveKNNMethodComponents(
-            KNNVectorFieldMapper.Builder builder,
+            Builder builder,
             ParserContext parserContext,
-            SpaceType resolvedSpaceType
+            SpaceType resolvedSpaceType,
+            VectorDataType vectorDataType
         ) {
             // Setup the initial configuration that is used to help resolve parameters.
             builder.setKnnMethodConfigContext(
@@ -512,7 +513,7 @@ private void resolveKNNMethodComponents(
                 builder.originalParameters.getResolvedKnnMethodContext(),
                 builder.knnMethodConfigContext,
                 false,
-                resolvedSpaceType
+                SpaceTypeResolver.INSTANCE.pickDefaultSpaceTypeWhenEmpty(resolvedSpaceType, vectorDataType)
             );
 
             // The original parameters stores both the resolveMethodContext as well as the original provided by the

diff --git a/src/test/java/org/opensearch/knn/index/engine/SpaceTypeResolverTests.java b/src/test/java/org/opensearch/knn/index/engine/SpaceTypeResolverTests.java
@@ -16,23 +16,47 @@ public class SpaceTypeResolverTests extends KNNTestCase {
     private static final SpaceTypeResolver SPACE_TYPE_RESOLVER = SpaceTypeResolver.INSTANCE;
 
     public void testResolveSpaceType_whenNoConfigProvided_thenFallbackToVectorDataType() {
-        assertEquals(SpaceType.DEFAULT, SPACE_TYPE_RESOLVER.resolveSpaceType(null, VectorDataType.FLOAT, ""));
-        assertEquals(SpaceType.DEFAULT, SPACE_TYPE_RESOLVER.resolveSpaceType(null, VectorDataType.BYTE, ""));
         assertEquals(
             SpaceType.DEFAULT,
-            SPACE_TYPE_RESOLVER.resolveSpaceType(
-                new KNNMethodContext(KNNEngine.DEFAULT, SpaceType.UNDEFINED, MethodComponentContext.EMPTY),
-                VectorDataType.FLOAT,
-                ""
+            SPACE_TYPE_RESOLVER.pickDefaultSpaceTypeWhenEmpty(
+                SPACE_TYPE_RESOLVER.resolveSpaceType(null, VectorDataType.FLOAT, ""),
+                VectorDataType.FLOAT
+            )
+        );
+        assertEquals(
+            SpaceType.DEFAULT,
+            SPACE_TYPE_RESOLVER.pickDefaultSpaceTypeWhenEmpty(
+                SPACE_TYPE_RESOLVER.resolveSpaceType(null, VectorDataType.BYTE, ""),
+                VectorDataType.FLOAT
+            )
+        );
+        assertEquals(
+            SpaceType.DEFAULT,
+            SPACE_TYPE_RESOLVER.pickDefaultSpaceTypeWhenEmpty(
+                SPACE_TYPE_RESOLVER.resolveSpaceType(
+                    new KNNMethodContext(KNNEngine.DEFAULT, SpaceType.UNDEFINED, MethodComponentContext.EMPTY),
+                    VectorDataType.FLOAT,
+                    ""
+                ),
+                VectorDataType.FLOAT
             )
         );
-        assertEquals(SpaceType.DEFAULT_BINARY, SPACE_TYPE_RESOLVER.resolveSpaceType(null, VectorDataType.BINARY, ""));
         assertEquals(
             SpaceType.DEFAULT_BINARY,
-            SPACE_TYPE_RESOLVER.resolveSpaceType(
-                new KNNMethodContext(KNNEngine.DEFAULT, SpaceType.UNDEFINED, MethodComponentContext.EMPTY),
-                VectorDataType.BINARY,
-                ""
+            SPACE_TYPE_RESOLVER.pickDefaultSpaceTypeWhenEmpty(
+                SPACE_TYPE_RESOLVER.resolveSpaceType(null, VectorDataType.BINARY, ""),
+                VectorDataType.BINARY
+            )
+        );
+        assertEquals(
+            SpaceType.DEFAULT_BINARY,
+            SPACE_TYPE_RESOLVER.pickDefaultSpaceTypeWhenEmpty(
+                SPACE_TYPE_RESOLVER.resolveSpaceType(
+                    new KNNMethodContext(KNNEngine.DEFAULT, SpaceType.UNDEFINED, MethodComponentContext.EMPTY),
+                    VectorDataType.BINARY,
+                    ""
+                ),
+                VectorDataType.BINARY
             )
         );
     }
@@ -49,34 +73,46 @@ public void testResolveSpaceType_whenMethodSpaceTypeAndTopLevelSpecified_thenThr
         );
         assertEquals(
             SpaceType.DEFAULT,
-            SPACE_TYPE_RESOLVER.resolveSpaceType(
-                new KNNMethodContext(KNNEngine.DEFAULT, SpaceType.DEFAULT, MethodComponentContext.EMPTY),
-                VectorDataType.FLOAT,
-                SpaceType.DEFAULT.getValue()
+            SPACE_TYPE_RESOLVER.pickDefaultSpaceTypeWhenEmpty(
+                SPACE_TYPE_RESOLVER.resolveSpaceType(
+                    new KNNMethodContext(KNNEngine.DEFAULT, SpaceType.DEFAULT, MethodComponentContext.EMPTY),
+                    VectorDataType.FLOAT,
+                    SpaceType.DEFAULT.getValue()
+                ),
+                VectorDataType.FLOAT
             )
         );
         assertEquals(
             SpaceType.DEFAULT,
-            SPACE_TYPE_RESOLVER.resolveSpaceType(
-                new KNNMethodContext(KNNEngine.DEFAULT, SpaceType.DEFAULT, MethodComponentContext.EMPTY),
-                VectorDataType.FLOAT,
-                SpaceType.UNDEFINED.getValue()
+            SPACE_TYPE_RESOLVER.pickDefaultSpaceTypeWhenEmpty(
+                SPACE_TYPE_RESOLVER.resolveSpaceType(
+                    new KNNMethodContext(KNNEngine.DEFAULT, SpaceType.DEFAULT, MethodComponentContext.EMPTY),
+                    VectorDataType.FLOAT,
+                    SpaceType.UNDEFINED.getValue()
+                ),
+                VectorDataType.FLOAT
             )
         );
         assertEquals(
             SpaceType.DEFAULT,
-            SPACE_TYPE_RESOLVER.resolveSpaceType(
-                new KNNMethodContext(KNNEngine.DEFAULT, SpaceType.UNDEFINED, MethodComponentContext.EMPTY),
-                VectorDataType.FLOAT,
-                SpaceType.DEFAULT.getValue()
+            SPACE_TYPE_RESOLVER.pickDefaultSpaceTypeWhenEmpty(
+                SPACE_TYPE_RESOLVER.resolveSpaceType(
+                    new KNNMethodContext(KNNEngine.DEFAULT, SpaceType.UNDEFINED, MethodComponentContext.EMPTY),
+                    VectorDataType.FLOAT,
+                    SpaceType.DEFAULT.getValue()
+                ),
+                VectorDataType.FLOAT
             )
         );
         assertEquals(
             SpaceType.DEFAULT,
-            SPACE_TYPE_RESOLVER.resolveSpaceType(
-                new KNNMethodContext(KNNEngine.DEFAULT, SpaceType.UNDEFINED, MethodComponentContext.EMPTY),
-                VectorDataType.FLOAT,
-                SpaceType.UNDEFINED.getValue()
+            SPACE_TYPE_RESOLVER.pickDefaultSpaceTypeWhenEmpty(
+                SPACE_TYPE_RESOLVER.resolveSpaceType(
+                    new KNNMethodContext(KNNEngine.DEFAULT, SpaceType.UNDEFINED, MethodComponentContext.EMPTY),
+                    VectorDataType.FLOAT,
+                    SpaceType.UNDEFINED.getValue()
+                ),
+                VectorDataType.FLOAT
             )
         );
     }

diff --git a/src/test/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapperTests.java b/src/test/java/org/opensearch/knn/index/mapper/KNNVectorFieldMapperTests.java
@@ -1635,7 +1635,7 @@ public void testTypeParser_whenBinaryWithLegacyKNNEnabled_thenException() throws
             typeParser.parse(fieldName, xContentBuilderToMap(xContentBuilder), buildParserContext(indexName, settings));
         });
 
-        assertTrue(ex.getMessage(), ex.getMessage().contains("does not support space type"));
+        assertTrue(ex.getMessage(), ex.getMessage().contains("Space type [l2] is not supported with [binary] data type"));
     }
 
     public void testBuild_whenInvalidCharsInFieldName_thenThrowException() {