Distinguish between simple matches with and without the terms index #63945

romseygeek · 2020-10-20T15:14:22Z

We currently use TextSearchInfo to let query parsers know when a field
will support match queries. Some field types (numeric, constant, range)
can produce simple match queries that don't use the terms index, and
it is useful to distinguish between these fields on the one hand and
keyword/text-type fields on the other. In particular, the
SignificantTextAggregation only works on fields that have indexed terms,
but there is currently no efficient way to see this at search time and so
the factory falls back on checking to see if an index analyzer has been
defined, with the result that some nonsensical field types are permitted.

This commit adds a new static TextSearchInfo implementation called
SIMPLE_MATCH_WITHOUT_TERMS that can be returned by field
types with no corresponding terms index. It changes significant text
to check for this rather than for the presence of an index analyzer.

This is a breaking change, in that the significant text agg will now throw
an error up-front if you try and apply it to a numeric field, whereas before
you would get an empty result.

elasticmachine · 2020-10-20T15:14:29Z

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

polyfractal

This is great! I stumbled into that oddity when working up tests for SigText after the VS refactoring, but didn't really have an idea how to fix it cleanly at the time :)

Probably best to wait on Mark's review since he knows the agg better, but 👍 from me

Do we need to add a note to the breaking-changes doc page?

…hinfo

…ongs

javanna

I left a couple of minor comments/questions, LGTM otherwise

javanna · 2020-10-21T09:57:48Z

...t/java/org/elasticsearch/search/aggregations/bucket/SignificantTermsSignificanceScoreIT.java

-        } else {
-            r = client().prepareSearch("cache_test_idx").setSize(0).addAggregation(significantTerms("foo").field("s")).get();
-        }
+        r = client().prepareSearch("cache_test_idx").setSize(0).addAggregation(significantTerms("foo").field("s")).get();


don't we lose coverage with these changes?

Hm, you're right, I had thought this was just testing a useless agg but actually I should change it instead to also have some text and to run the sigText agg over the text field instead.

javanna · 2020-10-21T10:00:19Z

server/src/main/java/org/elasticsearch/index/mapper/VersionFieldMapper.java

@@ -42,7 +42,7 @@
        public static final VersionFieldType INSTANCE = new VersionFieldType();

        private VersionFieldType() {
-            super(NAME, false, false, true, TextSearchInfo.SIMPLE_MATCH_ONLY, Collections.emptyMap());
+            super(NAME, false, false, true, TextSearchInfo.NONE, Collections.emptyMap());


This seems to have slipped when I double checked all the mappers, maybe it does not have a test so we don't enforce its consistency.

Yes, there's no test for VersionFieldMapper - I'll add one as a follow up.

javanna · 2020-10-21T10:03:20Z

...ava/org/elasticsearch/search/aggregations/bucket/terms/SignificantTextAggregatorFactory.java

+        if (ft.getTextSearchInfo() == TextSearchInfo.NONE) {
+            return false;
+        }
+        return ft.getTextSearchInfo() != TextSearchInfo.SIMPLE_MATCH_WITHOUT_TERMS;


should it be return ft.getTextSearchInfo != NONE && gt.getTextSearchIngo != SIMPLE_MATCH_WITHOUT_TERMS ?

…ndexes longs" This reverts commit 807bae1.

romseygeek · 2020-10-21T12:46:48Z

Do we need to add a note to the breaking-changes doc page?

We do, but I think I can only do that on backport as we don't have per-7.x-version changes in master?

markharwood · 2020-10-21T15:52:37Z

Looks good but I'm not sure about the terms part of naming TextSearchInfo.SIMPLE_MATCH_WITHOUT_TERMS.
While it relates to Lucene's termsEnum it may be misinterpreted as relating to supporting term queries.

Number fields support a term query e.g. this works:

"term":{"num_tags" : "1"}

I recall they actually do a range query behind the scenes.
I don't have a better suggestion for SIMPLE_MATCH_WITHOUT_TERMS currently.

markharwood

Is there a test that checks annotated_text is supported by significant_text?

polyfractal · 2020-10-21T17:00:06Z

We do, but I think I can only do that on backport as we don't have per-7.x-version changes in master?

Doh, yes, ignore me. :)

romseygeek · 2020-10-22T08:00:08Z

it may be misinterpreted as relating to supporting term queries.

I'm less worried about this, because it's an internal name and I think the javadoc is clear enough?

Is there a test that checks annotated_text is supported by significant_text?

There isn't, I'll see if I can add one.

…hinfo

romseygeek · 2020-10-27T10:50:35Z

Adding aggs tests to annotated text turns out to not be trivial. In the interest of not blocking this PR, I've opened a separate issue to deal with that.

…hinfo

romseygeek · 2020-10-27T11:34:26Z

@elasticmachine run elasticsearch-ci/packaging-sample-unix (gradle download failure)

…63945) We currently use TextSearchInfo to let query parsers know when a field will support match queries. Some field types (numeric, constant, range) can produce simple match queries that don't use the terms index, and it is useful to distinguish between these fields on the one hand and keyword/text-type fields on the other. In particular, the SignificantTextAggregation only works on fields that have indexed terms, but there is currently no efficient way to see this at search time and so the factory falls back on checking to see if an index analyzer has been defined, with the result that some nonsensical field types are permitted. This commit adds a new static TextSearchInfo implementation called SIMPLE_MATCH_WITHOUT_TERMS that can be returned by field types with no corresponding terms index. It changes significant text to check for this rather than for the presence of an index analyzer. This is a breaking change, in that the significant text agg will now throw an error up-front if you try and apply it to a numeric field, whereas before you would get an empty result.

Distinguish between simple matches with and without the terms index

5e0c1dd

romseygeek added >bug >breaking :Search Foundations/Mapping Index mappings, including merging and defining field types v8.0.0 v7.11.0 labels Oct 20, 2020

romseygeek requested review from markharwood and polyfractal October 20, 2020 15:14

romseygeek self-assigned this Oct 20, 2020

elasticmachine added the Team:Search Meta label for search team label Oct 20, 2020

test should use text field, not long...

172c803

$polyfractal$

polyfractal approved these changes Oct 20, 2020

View reviewed changes

romseygeek added 2 commits October 21, 2020 09:32

Merge remote-tracking branch 'origin/master' into mapper/numericsearc…

f2037bf

…hinfo

sigterms test should not try and use sigtext agg because it indexes l…

807bae1

…ongs

romseygeek mentioned this pull request Oct 21, 2020

Move index analyzer management to FieldMapper/MapperService #63937

Merged

javanna approved these changes Oct 21, 2020

View reviewed changes

romseygeek added 2 commits October 21, 2020 11:19

Revert "sigterms test should not try and use sigtext agg because it i…

1f81fbe

…ndexes longs" This reverts commit 807bae1.

keep test coverage; simplify support test

8714356

markharwood reviewed Oct 21, 2020

View reviewed changes

Merge remote-tracking branch 'origin/master' into mapper/numericsearc…

6adf89e

…hinfo

Merge remote-tracking branch 'origin/master' into mapper/numericsearc…

11fc9d4

…hinfo

romseygeek merged commit 4191c72 into elastic:master Oct 27, 2020

romseygeek deleted the mapper/numericsearchinfo branch October 27, 2020 12:08

romseygeek added a commit to romseygeek/elasticsearch that referenced this pull request Jan 13, 2021

Add breaking changes for elastic#64597 and elastic#63945

56b54c9

romseygeek added a commit that referenced this pull request Jan 13, 2021

Add breaking changes for #64597 and #63945 (#67420)

33f0163

romseygeek added a commit that referenced this pull request Jan 13, 2021

Add breaking changes for #64597 and #63945 (#67420)

c6890b8

jakelandis mentioned this pull request Feb 22, 2021

DRAFT [META] REST Compatible API V7 completeness #68905

Closed

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Distinguish between simple matches with and without the terms index #63945

Distinguish between simple matches with and without the terms index #63945

romseygeek commented Oct 20, 2020

elasticmachine commented Oct 20, 2020

$@polyfractal$ polyfractal left a comment

javanna left a comment

javanna Oct 21, 2020

romseygeek Oct 21, 2020

javanna Oct 21, 2020

javanna Oct 21, 2020

romseygeek Oct 21, 2020

javanna Oct 21, 2020

javanna Oct 21, 2020

romseygeek Oct 21, 2020

romseygeek commented Oct 21, 2020

markharwood commented Oct 21, 2020 •

edited

Loading

markharwood left a comment

polyfractal commented Oct 21, 2020

romseygeek commented Oct 22, 2020

romseygeek commented Oct 27, 2020

romseygeek commented Oct 27, 2020

Distinguish between simple matches with and without the terms index #63945

Distinguish between simple matches with and without the terms index #63945

Conversation

romseygeek commented Oct 20, 2020

elasticmachine commented Oct 20, 2020

polyfractal left a comment

Choose a reason for hiding this comment

javanna 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

romseygeek commented Oct 21, 2020

markharwood commented Oct 21, 2020 • edited Loading

markharwood left a comment

Choose a reason for hiding this comment

polyfractal commented Oct 21, 2020

romseygeek commented Oct 22, 2020

romseygeek commented Oct 27, 2020

romseygeek commented Oct 27, 2020

$@polyfractal$ polyfractal left a comment

markharwood commented Oct 21, 2020 •

edited

Loading