Bring back sparse_vector mapping by carlosdelest · Pull Request #98996 · elastic/elasticsearch

carlosdelest · 2023-08-29T15:11:10Z

Adds back sparse_vector field type, as a copy of rank_features.

The main goal is to have the sparse_vector field type available so we can switch ELSER queries to use the new type.

server/src/main/java/org/elasticsearch/index/mapper/vectors/SparseVectorFieldMapper.java

carlosdelest · 2023-08-29T17:07:02Z

server/src/main/java/org/elasticsearch/index/mapper/vectors/SparseVectorFieldMapper.java

This is copied and pasted from RankFeaturesFieldMapper with some checks on index version.

carlosdelest · 2023-08-29T17:07:43Z

server/src/main/java/org/elasticsearch/index/mapper/vectors/SparseVectorFieldMapper.java

+    public void parse(DocumentParserContext context) throws IOException {
+
+        // No support for indexing / searching 7.x sparse_vector field types
+        if (context.indexSettings().getIndexVersionCreated().before(IndexVersion.V_8_0_0)) {


Same here - checking that we're not operating on indices created before 8.0. Return the same errors that the previous implementation had.

carlosdelest · 2023-08-29T17:08:12Z

server/src/test/java/org/elasticsearch/index/mapper/vectors/SparseVectorFieldMapperTests.java

Copied from RankFeaturesFieldMapperTests, with some additions

carlosdelest · 2023-08-29T17:09:09Z

server/src/test/java/org/elasticsearch/index/mapper/vectors/SparseVectorFieldMapperTests.java

+        return Strings.concatStringArrays(super.getParseMinimalWarnings(indexVersion), additionalWarnings);
+    }
+
+    public void testSparseVectorWith7xIndex() throws Exception {


Added from the previous version of the test. The main idea is to allow indices to exist with the previous sparse_vector implementation (<8.0), but no new docs can be added to it.

carlosdelest · 2023-08-29T17:09:54Z

test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java

        return Strings.EMPTY_ARRAY;
    }

+    protected String[] getParseMinimalWarnings(IndexVersion indexVersion) {


Added this method so we can specify warnings that will be included when using mappings that go back to specific index versions.

benwtrent · 2023-08-29T17:33:28Z

server/src/main/java/org/elasticsearch/index/mapper/vectors/SparseVectorFieldMapper.java

        @Override
        protected Parameter<?>[] getParameters() {
-            return new Parameter<?>[] { meta };
+            return new Parameter<?>[] { positiveScoreImpact, meta };


remove positiveScoreImpact we can simplify this feature.

benwtrent · 2023-08-29T17:41:19Z

...ec/src/yamlRestTest/resources/rest-api-spec/test/search.vectors/90_sparse_vector_mapping.yml

@@ -0,0 +1,21 @@
+setup:


I think we should have a simple set of tests for exercising the mapped type & queries and such. No need to have a single file only for mapping tests.

This file should test:

Creating the mapping (in the correct versions)

Ensure creation fails in older 8.x versions

That queries work

That indexing works

carlosdelest · 2023-08-31T10:10:23Z

rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/search.vectors/90_sparse_vector.yml

@@ -0,0 +1,143 @@
+---
+"Indexing and searching sparse vectors":


Added a test that checks indexing and performs a simulated text_expansion query.

server/src/main/java/org/elasticsearch/index/mapper/vectors/SparseVectorFieldMapper.java

carlosdelest · 2023-08-31T10:12:09Z

test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java

        }
+    }

+    public void testBoostNotAllowed() throws IOException {


I've separated boost checking in two, for deprecated warnings and boosts not allowed.

carlosdelest · 2023-08-31T10:13:15Z

test/framework/src/main/java/org/elasticsearch/index/mapper/MapperTestCase.java

        assertParseMinimalWarnings();
    }

+    protected IndexVersion boostNotAllowedIndexVersion() {


There is the problem that checking for boosts not allowed caused sparse_vector mappings to error because sparse_vector is not allowed for 8.0.0.

I've created an overrridable method that returns the index version to test boosting for.

I'm happy to hear about other ideas for solving this in a simpler way 👍

carlosdelest · 2023-08-31T10:14:21Z

...tests/src/javaRestTest/java/org/elasticsearch/xpack/ml/integration/TextExpansionQueryIT.java

        RAW_MODEL_SIZE = Base64.getDecoder().decode(BASE_64_ENCODED_MODEL).length;
    }

+    public void testRankFeaturesTextExpansionQuery() throws IOException {


I've duped the tests on text_expansion queries so we check they can be used both with rank_features and sparse_vector field types.

I could as well have created separate classes for this, LMK if you can think of better ways to handle this.

Lets not do this. Thank you for taking initiative here. But, all the ML related changes should be done after sparse_vector is merged. It will be good to separate the changes to make the change log cleaner and reviews easier.

Got it - I'll revert this change and the text_expansion_search_sparse_vector.yml test as well.

carlosdelest · 2023-08-31T10:15:13Z

...gin/src/yamlRestTest/resources/rest-api-spec/test/ml/text_expansion_search_sparse_vector.yml

@@ -0,0 +1,109 @@
+# This test uses the simple model defined in


Added a separate YAML test that performs the same checks for text_expansion queries for sparse_vector field type

carlosdelest · 2023-08-31T11:09:57Z

@elasticsearchmachine run elasticsearch-ci/part-1

elasticsearchmachine · 2023-08-31T13:24:47Z

Hi @carlosdelest, I've created a changelog YAML for you.

elasticsearchmachine · 2023-08-31T13:24:47Z

Pinging @elastic/es-search (Team:Search)

kderusso

Nice work Carlos!

docs/changelog/98996.yaml

kderusso · 2023-08-31T17:16:37Z

rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/search.vectors/90_sparse_vector.yml

+      reason: "sparse_vector field type supported in 7.x"
+  - do:
+      allowed_warnings:
+        - "The [sparse_vector] field type is deprecated and will be removed in 8.0."


Do we need to keep the 8.0 warning here, since the test is 7.x only?

8.0.0 - indicates to me that this test only runs in 7.x as well. I am not sure why both warnings would be returned.

I have received both warnings on BWC tests, depending on the BWC test being performed:

All nodes under test are on 7.x

A mixed cluster test like this one. I understand that a node could have been updated to 8.x as part of this test, and thus return this warning.

kderusso · 2023-08-31T17:17:42Z

server/src/main/java/org/elasticsearch/index/mapper/vectors/SparseVectorFieldMapper.java

+
    public static final String CONTENT_TYPE = "sparse_vector";

+    static final String ERROR_MESSAGE_7X = "[sparse_vector] field type in old 7.x indices is allowed to "


Nitpicky question: Should we use the CONTENT_TYPE constant in error messages instead of hardcoding sparse_vector?

I copied / pasted from the previous sparse_vector error messages. As content type constant is unlikely to change, I personally think this is more readable as well (and in line with how error messages are created as well)

kderusso · 2023-08-31T17:18:33Z

server/src/main/java/org/elasticsearch/index/mapper/vectors/SparseVectorFieldMapper.java

-        public DocValueFormat docValueFormat(String format, ZoneId timeZone) {
-            throw new UnsupportedOperationException(ERROR_MESSAGE_7X);
+        public Query existsQuery(SearchExecutionContext context) {
+            throw new IllegalArgumentException("[sparse_vector] fields do not support [exists] queries");


Why did we decide to move from UnsupportedOperationException to IllegalArgumentException? Both are fine, just curious.

It's not really a change in the exception, as the method is different. Keep in mind that I copied / pasted RankFeaturesFieldMapper, so there are a lot of changes and the diff is misleading.

kderusso · 2023-08-31T17:20:43Z

server/src/test/java/org/elasticsearch/index/mapper/vectors/SparseVectorFieldMapperTests.java

-        );
+    @Override
+    protected boolean allowsNullValues() {
+        return false;       // TODO should this allow null values?


Nitpick: Remove TODO before merging

Copied / pasted from RankFeaturesFieldMapper, so I think it makes sense for re-evaluating in the future.

Co-authored-by: Kathleen DeRusso <kathleen.derusso@elastic.co>

carlosdelest · 2023-09-01T08:22:01Z

@elasticsearchmachine update branch

server/src/main/java/org/elasticsearch/index/mapper/vectors/SparseVectorFieldMapper.java

mayya-sharipova · 2023-09-04T18:24:05Z

server/src/main/java/org/elasticsearch/index/mapper/vectors/SparseVectorFieldMapper.java

+    static final String ERROR_MESSAGE_8X = "The [sparse_vector] field type is not supported from 8.0 to 8.10 versions.";
+    static final IndexVersion PREVIOUS_SPARSE_VECTOR_INDEX_VERSION = IndexVersion.V_8_0_0;
+
+    static final IndexVersion NEW_SPARSE_VECTOR_INDEX_VERSION = IndexVersion.V_8_500_000;


Should we have here V_8_500_001?

mayya-sharipova · 2023-09-04T18:31:25Z

server/src/main/java/org/elasticsearch/index/mapper/vectors/SparseVectorFieldMapper.java

+        // No support for indexing / searching 7.x sparse_vector field types
+        if (context.indexSettings().getIndexVersionCreated().before(PREVIOUS_SPARSE_VECTOR_INDEX_VERSION)) {
+            throw new UnsupportedOperationException(ERROR_MESSAGE_7X);
+        } else if (context.indexSettings().getIndexVersionCreated().before(IndexVersion.V_8_11_0)) {


Here instead of 8_11 we should also put a new V_8_500_001 version

mayya-sharipova · 2023-09-04T18:34:19Z

server/src/main/java/org/elasticsearch/index/mapper/vectors/SparseVectorFieldMapper.java

+                    if (context.doc().getByKey(key) != null) {
+                        throw new IllegalArgumentException(
+                            "[sparse_vector] fields do not support indexing multiple values for the same "
+                                + "rank feature ["


Instead of "rank feature", I think we can just say "feature"

mayya-sharipova · 2023-09-04T18:49:42Z

server/src/main/java/org/elasticsearch/index/mapper/vectors/SparseVectorFieldMapper.java

+
+    static final IndexVersion NEW_SPARSE_VECTOR_INDEX_VERSION = IndexVersion.V_8_500_000;
+
+    private static SparseVectorFieldType ft(FieldMapper in) {


It seems that it is never used. Should we remove it?

mayya-sharipova

@carlosdelest Thanks Carlos, I left minor comments, but the change overall LGTM!

…ector_type_comeback

carlosdelest · 2023-09-05T17:27:57Z

@elasticsearchmachine update branch

carlosdelest · 2023-09-06T08:46:45Z

@elasticsearchmachine run elasticsearch-ci/full-bwc

saikatsarkar056 · 2023-09-06T22:01:02Z

@carlosdelest

This PR has been merged. However, I would like to ask some questions to improve my understanding of this work. 😄

Will sparse_vector be searchable in the new version of deployment (8.11.0)?
Can sparse_vector contain negative values?
When are we planning to deprecate rank_features?

benwtrent · 2023-09-07T11:52:14Z

@saikatsarkar056

Will sparse_vector be searchable in the new version of deployment (8.11.0)?

Yes

Can sparse_vector contain negative values?

No

When are we planning to deprecate rank_features?

No

Add SparseVectorFieldMapper as a copy of RankFeaturesFieldMapper

2a55359

elasticsearchmachine added the v8.11.0 label Aug 29, 2023

Add basic YAML test for mapping

a262af9

carlosdelest commented Aug 29, 2023

View reviewed changes

carlosdelest requested a review from benwtrent August 29, 2023 17:10

carlosdelest mentioned this pull request Aug 29, 2023

Bring back sparse_vector mapping #98104

Closed

benwtrent reviewed Aug 29, 2023

View reviewed changes

carlosdelest added 8 commits August 30, 2023 10:51

Remove positive_score_impact

44c9a9b

Throw error when versions < 8.11

8910e1b

Added tests

ab8f9ed

Spotless

d915d64

Fix test

19a0d1f

Spotless

6178103

Fix test versions

8bbe283

Fix error message catch in test

223bd3c

carlosdelest commented Aug 31, 2023

View reviewed changes

carlosdelest added 2 commits August 31, 2023 14:08

Revert TextExpansionQueryIT changes

90d74e4

Revert text_expansion_search changes

6bfad1b

carlosdelest marked this pull request as ready for review August 31, 2023 12:14

carlosdelest requested a review from a team August 31, 2023 12:14

elasticsearchmachine added the needs:triage Requires assignment of a team area label label Aug 31, 2023

benwtrent added :Search Foundations/Mapping Index mappings, including merging and defining field types >enhancement test-full-bwc Trigger full BWC version matrix tests and removed needs:triage Requires assignment of a team area label labels Aug 31, 2023

elasticsearchmachine added the Team:Search Meta label for search team label Aug 31, 2023

kderusso reviewed Aug 31, 2023

View reviewed changes

benwtrent requested a review from mayya-sharipova August 31, 2023 19:53

Update changelog

6b818a3

Co-authored-by: Kathleen DeRusso <kathleen.derusso@elastic.co>

Merge branch 'main' into carlosdelest/sparse_vector_type_comeback

5a66ee6

mayya-sharipova reviewed Sep 4, 2023

View reviewed changes

server/src/main/java/org/elasticsearch/index/mapper/vectors/SparseVectorFieldMapper.java Outdated Show resolved Hide resolved

Use a new IndexVersion for checking index version

bfcd7aa

mayya-sharipova reviewed Sep 4, 2023

View reviewed changes

mayya-sharipova approved these changes Sep 4, 2023

View reviewed changes

carlosdelest and others added 6 commits September 5, 2023 09:15

PR review

0f718ee

Merge remote-tracking branch 'origin/main' into carlosdelest/sparse_v…

647136a

…ector_type_comeback

Fix test

48350a9

Merge remote-tracking branch 'origin/main' into carlosdelest/sparse_v…

523fda9

…ector_type_comeback

Spotless

a63790a

Merge branch 'main' into carlosdelest/sparse_vector_type_comeback

997ed30

carlosdelest added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Sep 5, 2023

elasticsearchmachine merged commit 6bbf6c2 into elastic:main Sep 6, 2023

carlosdelest deleted the carlosdelest/sparse_vector_type_comeback branch September 6, 2023 09:27

DaveCTurner mentioned this pull request Sep 6, 2023

New sparse_vector YAML tests fail in serverless #99238

Closed

pmpailis mentioned this pull request Sep 21, 2023

sparse_vector fields should support exists queries #99319

Closed

This was referenced Oct 6, 2023

sparse_vector field support elastic/kibana#168186

Merged

Add sparse_vector field type elastic/elasticsearch-specification#2304

Merged

		@@ -0,0 +1,143 @@
		---
		"Indexing and searching sparse vectors":

		@@ -0,0 +1,109 @@
		# This test uses the simple model defined in


		public static final String CONTENT_TYPE = "sparse_vector";

		static final String ERROR_MESSAGE_7X = "[sparse_vector] field type in old 7.x indices is allowed to "


		static final IndexVersion NEW_SPARSE_VECTOR_INDEX_VERSION = IndexVersion.V_8_500_000;

		private static SparseVectorFieldType ft(FieldMapper in) {

Conversation

carlosdelest commented Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlosdelest commented Aug 31, 2023

Uh oh!

elasticsearchmachine commented Aug 31, 2023

Uh oh!

elasticsearchmachine commented Aug 31, 2023

Uh oh!

kderusso left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlosdelest commented Sep 1, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mayya-sharipova Sep 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mayya-sharipova left a comment

Choose a reason for hiding this comment

Uh oh!

carlosdelest commented Sep 5, 2023

Uh oh!

carlosdelest commented Aug 29, 2023 •

edited

Loading

mayya-sharipova Sep 4, 2023 •

edited

Loading