semantic_text: Add exists query #110027

carlosdelest · 2024-06-21T09:01:03Z

Does what it says on the tin

…-query

...nference/src/main/java/org/elasticsearch/xpack/inference/mapper/SemanticTextFieldMapper.java

carlosdelest · 2024-06-21T10:43:02Z

...nce/src/test/java/org/elasticsearch/xpack/inference/mapper/SemanticTextFieldMapperTests.java

@@ -178,36 +181,10 @@ public void testDynamicUpdate() throws IOException {
 final String fieldName = "semantic";
 final String inferenceId = "test_service";

- MapperService mapperService = createMapperService(mapping(b -> {}));


I extracted this as a method to enable updating the mapping to check the actual exists queries created

carlosdelest · 2024-06-21T10:43:26Z

...nce/src/test/java/org/elasticsearch/xpack/inference/mapper/SemanticTextFieldMapperTests.java

+ assertThat(toParentBlockJoinQuery.getChildQuery(), instanceOf(FieldExistsQuery.class));
+ }
+
+ private MapperService mapperServiceForFieldWithModelSettings(


... and this is the extracted method

elasticsearchmachine · 2024-06-21T11:42:26Z

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

...nference/src/main/java/org/elasticsearch/xpack/inference/mapper/SemanticTextFieldMapper.java

...nce/src/test/java/org/elasticsearch/xpack/inference/mapper/SemanticTextFieldMapperTests.java

jimczi

I left two suggestions.

...nference/src/main/java/org/elasticsearch/xpack/inference/mapper/SemanticTextFieldMapper.java

jimczi

LGTM

jimczi · 2024-06-21T16:23:27Z

server/src/main/java/org/elasticsearch/index/query/NestedQueryBuilder.java

+
+ /**
+ * Returns the primitive Lucene query for a nested query given the primitive query to wrap
+ * @param innerQuery query to wraqp in a nested query


Suggested change

* @param innerQuery query to wraqp in a nested query

* @param innerQuery query to wrap in a nested query

corrqected 😝

server/src/main/java/org/elasticsearch/index/query/NestedQueryBuilder.java

benwtrent

I am not sure the doToQuery and toQuery logic for the rewrite is 100% correct.

benwtrent · 2024-06-21T18:42:45Z

server/src/main/java/org/elasticsearch/index/query/NestedQueryBuilder.java

@@ -260,6 +260,34 @@ protected int doHashCode() {

 @Override
 protected Query doToQuery(SearchExecutionContext context) throws IOException {
+ Query innerQuery;
+ NestedObjectMapper mapper = context.nestedLookup().getNestedMappers().get(path);


I think all checks that were previously done in toQuery before this nested lookup need to occur.

Including

NestedObjectMapper mapper = context.nestedLookup().getNestedMappers().get(path); if (mapper == null) { if (ignoreUnmapped) { return new MatchNoDocsQuery(); } else { throw new IllegalStateException("[" + NAME + "] failed to find nested object under path [" + path + "]"); } }

And

if (context.allowExpensiveQueries() == false) {

Totally - I moved the meat out of the sandwich. I have corrected it now.

...nference/src/main/java/org/elasticsearch/xpack/inference/mapper/SemanticTextFieldMapper.java

Mikep86 · 2024-06-21T21:30:19Z

server/src/main/java/org/elasticsearch/index/query/NestedQueryBuilder.java

+ * @return the primitive Lucene query
+ */
+ public static <E extends Exception> Query toQuery(
+ CheckedFunction<SearchExecutionContext, Query, E> queryProvider,


Great CheckedFunction usage!

Mikep86 · 2024-06-21T21:34:36Z

...nce/src/test/java/org/elasticsearch/xpack/inference/mapper/SemanticTextFieldMapperTests.java

+ Mapper mapper = mapperService.mappingLookup().getMapper(fieldName);
+ assertNotNull(mapper);
+ Query existsQuery = ((SemanticTextFieldMapper) mapper).fieldType().existsQuery(createSearchExecutionContext(mapperService));
+ assertThat(existsQuery, instanceOf(ESToParentBlockJoinQuery.class));


Can you add back the inner query check after this?

I was thinking that's too much coupling and details - it's not about checking the bool query with two clauses that are resulting from this, but the actual need to do so given we have YAML coverage. We're functionally covered, and we probably shouldn't care about the actual details of the query rewriting.

Happy to add the checks if you think it's absolutely needed

I agree that hard-coding the expected query class creates too much coupling. However, as written testExistsQuerySparseVector & testExistsQueryDenseVector are not meaningfully different. In particular, the lack of assertions on the child query means that we're not testing that the child query is reflective of the backing embedding field type (sparse_vector or dense_vector).

I thought about how to resolve this without creating unmaintainable couplings and came up with this approach:

MapperBuilderContext mapperBuilderContext = MapperBuilderContext.root(false, false); SearchExecutionContext searchExecutionContext = createSearchExecutionContext(mapperService); SparseVectorFieldMapper sparseVectorFieldMapper = new SparseVectorFieldMapper.Builder(SemanticTextField.getEmbeddingsFieldName(fieldName)).build(mapperBuilderContext); ... assertThat(toParentBlockJoinQuery.getChildQuery(), equalTo(sparseVectorFieldMapper.fieldType().existsQuery(searchExecutionContext)));

And something similar for testExistsQueryDenseVector, except creating a DenseVectorFieldMapper. WDYT?

I like that, I think it's a good middle ground!

@Mikep86 I've included additional checks in cabf436.

I'm not convinced on this - we're essentially re-doing what the implementation does internally. But I couldn't find another way of including the nested path in the final bool query otherwise.

Mikep86 · 2024-06-21T21:34:50Z

...nce/src/test/java/org/elasticsearch/xpack/inference/mapper/SemanticTextFieldMapperTests.java

+ Mapper mapper = mapperService.mappingLookup().getMapper(fieldName);
+ assertNotNull(mapper);
+ Query existsQuery = ((SemanticTextFieldMapper) mapper).fieldType().existsQuery(createSearchExecutionContext(mapperService));
+ assertThat(existsQuery, instanceOf(ESToParentBlockJoinQuery.class));


Can you add back the inner query check after this?

…-query # Conflicts: # x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/mapper/SemanticTextFieldMapperTests.java

Mikep86 · 2024-07-01T12:12:50Z

...nce/src/test/java/org/elasticsearch/xpack/inference/mapper/SemanticTextFieldMapperTests.java

+ Query expectedQuery = NestedQueryBuilder.toQuery(
+ context -> sparseVectorFieldMapper.fieldType().existsQuery(context),
+ SemanticTextField.getChunksFieldName(fieldName),
+ toParentBlockJoinQuery.getScoreMode(),
+ false,
+ searchExecutionContext
+ );
+ assertThat(toParentBlockJoinQuery, equalTo(expectedQuery));


I see what you mean. WDYT about something like this?

Suggested change

Query expectedQuery = NestedQueryBuilder.toQuery(

context -> sparseVectorFieldMapper.fieldType().existsQuery(context),

SemanticTextField.getChunksFieldName(fieldName),

toParentBlockJoinQuery.getScoreMode(),

false,

searchExecutionContext

);

assertThat(toParentBlockJoinQuery, equalTo(expectedQuery));

assertThat(toParentBlockJoinQuery.getChildQuery(), instanceOf(BooleanQuery.class));

BooleanQuery booleanQuery = (BooleanQuery) toParentBlockJoinQuery.getChildQuery();

assertTrue(

booleanQuery.clauses()

.stream()

.anyMatch(b -> b.getQuery().equals(sparseVectorFieldMapper.fieldType().existsQuery(searchExecutionContext)))

);

We still need to know that we're creating a boolean query under the hood (which isn't entirely bad IMO), but we're duplicating less.

I still find this very leaky. Given that we're already testing the query with the YAML tests, I'd opt for the simplest solution here, and just asserting the top ESToParentBlockJoinQuery.

Fair enough. Thanks for iterating on this with me!

It was a good idea Mike, thanks for sharing!

carlosdelest added 2 commits June 21, 2024 10:59

Add exists query to semantic_text

b0a5b53

Merge branch 'refs/heads/main' into carlosdelest/semantic-text-exists…

12b8533

…-query

elasticsearchmachine added the v8.15.0 label Jun 21, 2024

carlosdelest commented Jun 21, 2024

View reviewed changes

...nference/src/main/java/org/elasticsearch/xpack/inference/mapper/SemanticTextFieldMapper.java Outdated Show resolved Hide resolved

carlosdelest added 2 commits June 21, 2024 12:18

Fix exists query test

8ca22c8

Added tests for the actual implementation of the exists query

63cc67b

carlosdelest commented Jun 21, 2024

View reviewed changes

carlosdelest added >non-issue :Search Foundations/Mapping Index mappings, including merging and defining field types Team:Search Meta label for search team labels Jun 21, 2024

carlosdelest marked this pull request as ready for review June 21, 2024 11:42

carlosdelest requested review from Mikep86 and jimczi June 21, 2024 11:42

Mikep86 reviewed Jun 21, 2024

View reviewed changes

...nference/src/main/java/org/elasticsearch/xpack/inference/mapper/SemanticTextFieldMapper.java Outdated Show resolved Hide resolved

...nce/src/test/java/org/elasticsearch/xpack/inference/mapper/SemanticTextFieldMapperTests.java Show resolved Hide resolved

jimczi reviewed Jun 21, 2024

View reviewed changes

...nference/src/main/java/org/elasticsearch/xpack/inference/mapper/SemanticTextFieldMapper.java Outdated Show resolved Hide resolved

...nference/src/main/java/org/elasticsearch/xpack/inference/mapper/SemanticTextFieldMapper.java Outdated Show resolved Hide resolved

carlosdelest added 3 commits June 21, 2024 16:59

Use rewrite - bad idea

9c7ba83

Refactor NestedQueryBuilder to expose toQuery method

3dc6c1d

Fix nested scope

384a5ad

jimczi approved these changes Jun 21, 2024

View reviewed changes

carlosdelest added 2 commits June 21, 2024 18:28

Small fixes

6825311

checkstyle fix

a5a3bbe

Mikep86 reviewed Jun 21, 2024

View reviewed changes

server/src/main/java/org/elasticsearch/index/query/NestedQueryBuilder.java Outdated Show resolved Hide resolved

benwtrent reviewed Jun 21, 2024

View reviewed changes

Use a Query provider to restore previous doToQuery() semantics

b8d7315

Mikep86 reviewed Jun 21, 2024

View reviewed changes

benwtrent approved these changes Jun 24, 2024

View reviewed changes

Mikep86 mentioned this pull request Jun 25, 2024

Some queries do not respect the index.query.parse.allow_unmapped_fields index setting #110156

Open

sorenlouv mentioned this pull request Jun 27, 2024

[AI Assistant] Use semantic_text for internal knowledge base elastic/kibana#186499

Open

Extra test checks

cabf436

carlosdelest added 2 commits July 1, 2024 13:31

Merge branch 'refs/heads/main' into carlosdelest/semantic-text-exists…

8843eb7

…-query # Conflicts: # x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/mapper/SemanticTextFieldMapperTests.java

Fix merge with main

c6394b5

Mikep86 reviewed Jul 1, 2024

View reviewed changes

Mikep86 approved these changes Jul 1, 2024

View reviewed changes

Remove extra test checks

ff4c980

carlosdelest merged commit 258a8b5 into elastic:main Jul 1, 2024
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

semantic_text: Add exists query #110027

semantic_text: Add exists query #110027

carlosdelest commented Jun 21, 2024

carlosdelest Jun 21, 2024

carlosdelest Jun 21, 2024

elasticsearchmachine commented Jun 21, 2024

jimczi left a comment

jimczi left a comment

jimczi Jun 21, 2024

carlosdelest Jun 21, 2024

benwtrent left a comment

benwtrent Jun 21, 2024

carlosdelest Jun 21, 2024

Mikep86 Jun 21, 2024

Mikep86 Jun 21, 2024

carlosdelest Jun 25, 2024

Mikep86 Jun 25, 2024

carlosdelest Jun 26, 2024

carlosdelest Jul 1, 2024

Mikep86 Jun 21, 2024

Mikep86 Jul 1, 2024

carlosdelest Jul 1, 2024

Mikep86 Jul 1, 2024

carlosdelest Jul 1, 2024

	* @param innerQuery query to wraqp in a nested query
	* @param innerQuery query to wrap in a nested query

semantic_text: Add exists query #110027

semantic_text: Add exists query #110027

Conversation

carlosdelest commented Jun 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticsearchmachine commented Jun 21, 2024

jimczi left a comment

Choose a reason for hiding this comment

jimczi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benwtrent left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment