Super randomized tests for fetch fields API #70278

nik9000 · 2021-03-10T22:13:59Z

We've had a few bugs in the fields API where is doesn't behave like we'd
expect. Typically this happens because it isn't obvious what we expct. So
we'll try and use randomized testing to ferret out what we want. This adds
a test for most field types that asserts that fields works similarly
to docvalues_fields. We expect this to be true for most fields.

It does so by forcing all subclasses of MapperTestCase to define a
method that makes random values. It declares a few other hooks that
subclasses can override to further randomize the test.

We skip the test for a few field types that don't have doc values:

annotated_text
completion
search_as_you_type
We should come up with some way to test these without doc values, even
if it isn't as nice. But that is a problem for another time, I think.

For text fields we use fielddata which works, but its not a great
test becuase we intentionally don't mimick docvalue_fields in
fields. So we have to pick our input data in a funny way. It's better
than nothing.

We skip the test for a few more types just because I wanted to cut this
PR in half so we could get to reviewing it earlier. We'll get to those
in a follow up change.

I've filed a few bugs for things that are inconsistent with
docvalues_fields. Typically that means that we have to limit the
random values that we generate to those that do round trip properly.

We've had a few bugs in the fields API where is doesn't behave like we'd expect. Typically this happens because it isn't obvious what we expct. So we'll try and use randomized testing to ferret out what we want. This adds a test for most field types that asserts that `fields` works similarly to `docvalues_fields`. We expect this to be true for most fields. It does so by forcing all subclasses of `MapperTestCase` to define a method that makes random values. It declares a few other hooks that subclasses can override to further randomize the test. We skip the test for a few field types that don't have doc values: * `annotated_text` * `completion` * `search_as_you_type` We should come up with some way to test these without doc values, even if it isn't as nice. But that is a problem for another time, I think. For `text` fields we use `fielddata` which *works*, but its not a great test becuase we intentionally don't mimick `docvalue_fields` in `fields`. So we have to pick our input data in a funny way. It's better than nothing. We skip the test for a few more types just because I wanted to cut this PR in half so we could get to reviewing it earlier. We'll get to those in a follow up change. I've filed a few bugs for things that are inconsistent with `docvalues_fields`. Typically that means that we have to limit the random values that we generate to those that *do* round trip properly.

nik9000 · 2021-03-10T22:15:09Z

.../mapper-extras/src/test/java/org/elasticsearch/index/mapper/RankFeatureFieldMapperTests.java

@@ -136,4 +137,10 @@ public void testRejectMultiValuedFields() throws MapperParsingException, IOExcep
        assertEquals("[rank_feature] fields do not support indexing multiple values for the same field [foo.field] in the same document",
                e.getCause().getMessage());
    }
+
+    @Override
+    protected Supplier<? extends Object> randomFetchTestValueVendor(MappedFieldType ft) {


I chose a Supplier here because we have a test that tests many values at once and I wanted to be able to build many values that have a similar "theme". Check out the tests for dates for an example of where this is useful.

Could you explain this more? I looked at the date tests but couldn't tell where having a 'supplier' was critical.

Rereading this I don't think it really is critical. It was when was sorting the values and deduping them, but it isn't really critical any more. It is a bit more realistic to generate similarly themed values though. But we don't really need it any more. What do you think?

+1 to making this just an Object

nik9000 · 2021-03-10T22:15:43Z

.../mapper-extras/src/test/java/org/elasticsearch/index/mapper/RankFeatureFieldMapperTests.java

+
+    @Override
+    protected Supplier<? extends Object> randomFetchTestValueVendor(MappedFieldType ft) {
+        assumeFalse("Test implemented in a follow up", true);


I did this because I wanted to be able to open the PR and for everything to compile. I do expect to handle this in a follow up.

elasticmachine · 2021-03-15T16:23:49Z

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

nik9000 · 2021-03-15T16:24:44Z

I've removed draft. The folks I talked to were comfortable with this as an incremental approach. It might not be where we land but its a whole lot of tests we don't have now and found a few fun things already.

not-napoleon

I really like this idea. Eventually, I'd like to replace the logic in AggregatorTestCase#writeTestDoc to use these suppliers.

not-napoleon · 2021-03-15T17:13:56Z

server/src/test/java/org/elasticsearch/index/mapper/RangeFieldMapperTests.java

+                return Map.of(randomBoolean() ? "gt" : "gte", lhs, randomBoolean() ? "lt" : "lte", rhs);
+            };
+        }
+        throw new UnsupportedOperationException("unsupported field type: " + ft);


Maybe leave a TODO comment for IP and Date ranges?

Yikes. I forgot that one. Yes.

Small comment, maybe it's cleaner to avoid adding this initial logic until we remove the assumeFalse.

I wanted to come up with some way to make sure we don't throw it away for when we're ready for it. I can revert it from the PR though because I can use the revert commit in the PR to revive it.

nik9000 · 2021-03-15T18:46:35Z

@elasticmachine, update branch

jtibshirani

I left some (mostly small) comments. I really like the approach overall!

One high-level thought: these tests actually test more than field fetching, they also verify document parsing and doc values loading. I don't have a great naming idea, maybe testParseAndFetchValues?

jtibshirani · 2021-03-16T19:57:34Z

server/src/test/java/org/elasticsearch/index/mapper/TextFieldMapperTests.java

@@ -1079,4 +1081,18 @@ public void testSimpleMerge() throws IOException {
        assertThat(mapperService.documentMapper().mappers().getMapper("field"), instanceOf(TextFieldMapper.class));
        assertThat(mapperService.documentMapper().mappers().getMapper("other_field"), instanceOf(KeywordFieldMapper.class));
    }
+
+    @Override
+    protected Supplier<? extends Object> randomFetchTestValueVendor(MappedFieldType ft) {


To me it doesn't feel worth testing random fetch for text fields. It's not really accurate to compare the fetch output to field data, since field data is based on analyzed tokens. Plus, the fetching/ parsing logic is straightforward and randomized tests might not bring much insight.

I don't really think its worth it either because doc values are so different. Do you think I should just assumeFalse on this?

Yep assumeFalse here makes sense to me.

jtibshirani · 2021-03-16T20:00:56Z

server/src/test/java/org/elasticsearch/index/mapper/RangeFieldMapperTests.java

+                return Map.of(randomBoolean() ? "gt" : "gte", lhs, randomBoolean() ? "lt" : "lte", rhs);
+            };
+        }
+        throw new UnsupportedOperationException("unsupported field type: " + ft);


Small comment, maybe it's cleaner to avoid adding this initial logic until we remove the assumeFalse.

jtibshirani · 2021-03-16T20:02:49Z

server/src/test/java/org/elasticsearch/index/mapper/NumberFieldMapperTests.java

+         * fetcher doesn't.
+         */
+        // https://github.com/elastic/elasticsearch/issues/70263
+//        if (randomBoolean()) {


It's nice to avoid commented-out logic, maybe we can remove it while we resolve the above issue.

++. I'll drop it and link it to the issue.

jtibshirani · 2021-03-16T20:08:26Z

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

+             * and the native fetchers usually don't sort. We're ok with this
+             * difference. But we have to convince the test we're ok with it.
+             */
+            assertThat(fromNative, containsInAnyOrder(fromDocValues.stream().map(Matchers::equalTo).collect(toList())));


Could you explain how this check differs from the below?

assertThat(fromNative, containsInAnyOrder(fromDocValues));

Yours is just better. I was scanning the list of methods in Matchers and didn't see yours. I haven't a clue how I missed it.

jtibshirani · 2021-03-16T20:36:43Z

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

+     * and {@link #testFetchMany}. The returned objects must be supported by
+     * {@link XContentBuilder#value(Object)}
+     */
+    protected abstract Supplier<? extends Object> randomFetchTestValueVendor(MappedFieldType ft);


Maybe a name like randomFetchTestValueSupplier would be clearer, I haven't seen the 'vendor' terminology used elsewhere. (I could also see this name being generalized in the future to randomValueSupplier, as we add more tests that take advantage of random value generation).

When I first wrote it I had named it randomValueSupplier but I figured it wasn't right to call it that if we were going to use it for other things. I can certainly rename it if you think we will. I'm happy to replace Vendor with Supplier too. Both work for me. I'm not sure why I went with vendor there. maybe I just felt like being different.

How about generateRandomInputValue? Or we could reuse getSampleValueForDocument and add some randomization there?

I'm going to go with generateRandomInputValue and we can cut getSampleValueForDocument over to it in a follow up. I'd like to contain all of the "fun" randomization failures that show up from that little change to their own PR if you'll allow it.

jtibshirani · 2021-03-16T20:38:29Z

server/src/test/java/org/elasticsearch/index/mapper/KeywordFieldMapperTests.java

-        return createMapperService(mapping(b -> b.startObject("field").field("type", "keyword").endObject()));
+    @Override
+    protected Supplier<Comparable<?>> randomFetchTestValueVendor(MappedFieldType ft) {
+        return () -> randomAlphaOfLengthBetween(1, 100);


Should we generate a broader range of valid values, like numbers and booleans?

Yeah, I guess numbers and booleans do get converted. We should indeed generate them.

jtibshirani · 2021-03-16T20:43:37Z

...amework/src/main/java/org/elasticsearch/index/mapper/AbstractNumericFieldMapperTestCase.java

+    }
+
+    protected Supplier<Number> randomFetchValueVendor(NumericType nt) {
+        switch (nt) {


It'd be great to exercise 'coercion' here too: numbers can be specified as strings, and we also allow passing float point values to integral types.

And it found #70585

jtibshirani · 2021-03-16T20:49:39Z

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

+        try {
+            MappedFieldType ft = mapperService.fieldType("field");
+            /*
+             * When we have many values doc values will sort and unique them.


Is this comment out-of-date? It doesn't seem we sort or deduplicate values here. This got me thinking, should we deduplicate fetched values in assertFetch before comparing?

It is out of date.

It looks to me like we shouldn't deduplicate because doc values doesn't deduplicate. Unless I've messed up my test somehow.

jtibshirani · 2021-03-16T20:54:40Z

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

+            MappedFieldType ft = mapperService.fieldType("field");
+            assertFetch(mapperService, "field", randomFetchTestValueVendor(ft).get(), randomFetchTestFormat());
+        } finally {
+            assertFetchWarnings();


I wonder if we could remove this method assertFetchWarnings() and just call assertParseMinimalWarnings here.

I think it'd work for now. Maybe not later. But if we need it then we can add it then.

jtibshirani · 2021-03-16T20:58:26Z

.../mapper-extras/src/test/java/org/elasticsearch/index/mapper/RankFeatureFieldMapperTests.java

@@ -136,4 +137,10 @@ public void testRejectMultiValuedFields() throws MapperParsingException, IOExcep
        assertEquals("[rank_feature] fields do not support indexing multiple values for the same field [foo.field] in the same document",
                e.getCause().getMessage());
    }
+
+    @Override
+    protected Supplier<? extends Object> randomFetchTestValueVendor(MappedFieldType ft) {


Could you explain this more? I looked at the date tests but couldn't tell where having a 'supplier' was critical.

romseygeek · 2021-03-17T14:49:59Z

I'm still unsure about the tests on this one, but I think that's because I'm unsure about what we want to guarantee is returned in fields. Given no format, do we want to return:

the value that was added in _source - ie, precisely what would have been returned if you asked for _source filtered down to just this field
the value that would have been used if this field was searched as part of the original query
a value that can be lifted out and used in subsequent queries or aggregations

Each of these options is perfectly reasonable, and each one requires different guarantees - this PR is basically testing for the second option here, but if we instead want to go for the first or third then we'll need different tests. In particular, the fact that you can apply a format implies that fields is mainly about display, and so option 1 seems reasonable.

nik9000 · 2021-03-18T19:45:29Z

@jtibshirani and @romseygeek I think this is ready for another round.

In response to @romseygeek's last comment - for the most part we're trying to be consistent with how we parse the field in the first place and to apply the format. This mostly means we line up with docvalues_fields. We allow ourselves to deviate from it if we feel like we have a good reason, but our "default" is to behave the same way.

Only arrays for equality.

romseygeek

Definitely getting closer! I left a couple more questions.

romseygeek · 2021-03-22T18:03:54Z

.../mapper-extras/src/test/java/org/elasticsearch/index/mapper/RankFeatureFieldMapperTests.java

@@ -136,4 +137,10 @@ public void testRejectMultiValuedFields() throws MapperParsingException, IOExcep
        assertEquals("[rank_feature] fields do not support indexing multiple values for the same field [foo.field] in the same document",
                e.getCause().getMessage());
    }
+
+    @Override
+    protected Supplier<? extends Object> randomFetchTestValueVendor(MappedFieldType ft) {


+1 to making this just an Object

romseygeek · 2021-03-22T18:06:44Z

server/src/test/java/org/elasticsearch/index/mapper/BooleanFieldMapperTests.java

+
+    @Override
+    protected Supplier<? extends Object> randomFetchTestValueVendor(MappedFieldType ft) {
+        switch (between(0, 3)) {


This will definitely be nicer when it doesn't return a Supplier as then we get a random mix of types within a single doc.

romseygeek · 2021-03-22T18:10:11Z

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

+     * and {@link #testFetchMany}. The returned objects must be supported by
+     * {@link XContentBuilder#value(Object)}
+     */
+    protected abstract Supplier<? extends Object> randomFetchTestValueVendor(MappedFieldType ft);


How about generateRandomInputValue? Or we could reuse getSampleValueForDocument and add some randomization there?

nik9000 · 2021-03-22T20:53:45Z

OK folks! Time for another round. I've remove the Supplier. I don't like the kinds of values it makes as much but I can live with it.

romseygeek

LGTM - one nit and one query but no need for another go round from me.

romseygeek · 2021-03-23T09:13:13Z

server/src/test/java/org/elasticsearch/index/mapper/NumberFieldMapperTests.java

+    @Override
+    protected final Object generateRandomInputValue(MappedFieldType ft) {
+        Number n = randomNumber();
+        return randomBoolean() ? n : n.toString();


Is this going to run into #69382 and the issues with scientific format? Or does that only apply to date parsing?

Numbers are fine parsing scientific notation. It's dates that hate 'em. There is an issue with buried in the somewhere but I've awaitsFixed that bit and opened another issue.

romseygeek · 2021-03-23T09:17:01Z

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

+    }
+
+    /**
+     * Create a vendor for similar random values to round trip in {@link #testFetch}


'Generates random values to round trip in...`

We've had a few bugs in the fields API where is doesn't behave like we'd expect. Typically this happens because it isn't obvious what we expct. So we'll try and use randomized testing to ferret out what we want. This adds a test for most field types that asserts that `fields` works similarly to `docvalues_fields`. We expect this to be true for most fields. It does so by forcing all subclasses of `MapperTestCase` to define a method that makes random values. It declares a few other hooks that subclasses can override to further randomize the test. We skip the test for a few field types that don't have doc values: * `annotated_text` * `completion` * `search_as_you_type` * `text` We should come up with some way to test these without doc values, even if it isn't as nice. But that is a problem for another time, I think. We skip the test for a few more types just because I wanted to cut this PR in half so we could get to reviewing it earlier. We'll get to those in a follow up change. I've filed a few bugs for things that are inconsistent with `docvalues_fields`. Typically that means that we have to limit the random values that we generate to those that *do* round trip properly.

) We've had a few bugs in the fields API where is doesn't behave like we'd expect. Typically this happens because it isn't obvious what we expct. So we'll try and use randomized testing to ferret out what we want. This adds a test for most field types that asserts that `fields` works similarly to `docvalues_fields`. We expect this to be true for most fields. It does so by forcing all subclasses of `MapperTestCase` to define a method that makes random values. It declares a few other hooks that subclasses can override to further randomize the test. We skip the test for a few field types that don't have doc values: * `annotated_text` * `completion` * `search_as_you_type` * `text` We should come up with some way to test these without doc values, even if it isn't as nice. But that is a problem for another time, I think. We skip the test for a few more types just because I wanted to cut this PR in half so we could get to reviewing it earlier. We'll get to those in a follow up change. I've filed a few bugs for things that are inconsistent with `docvalues_fields`. Typically that means that we have to limit the random values that we generate to those that *do* round trip properly.

nik9000 requested review from not-napoleon, romseygeek and jtibshirani March 10, 2021 22:13

nik9000 commented Mar 10, 2021

View reviewed changes

nik9000 added 2 commits March 10, 2021 17:29

More

7bf4d60

Merge branch 'master' into random_random_fetch

26033ca

nik9000 marked this pull request as ready for review March 15, 2021 16:23

nik9000 added :Search Foundations/Mapping Index mappings, including merging and defining field types >test Issues or PRs that are addressing/adding tests v7.13.0 v8.0.0 labels Mar 15, 2021

elasticmachine added the Team:Search Meta label for search team label Mar 15, 2021

not-napoleon reviewed Mar 15, 2021

View reviewed changes

nik9000 added 2 commits March 15, 2021 13:58

Merge branch 'master' into random_random_fetch

24a9062

TODO

3802c55

Merge branch 'master' into random_random_fetch

6a3696d

romseygeek mentioned this pull request Mar 16, 2021

Add calculated numeric fields #69531

Merged

jtibshirani reviewed Mar 16, 2021

View reviewed changes

nik9000 added 7 commits March 18, 2021 15:04

Merge branch 'master' into random_random_fetch

e03b5a5

Use string values

3e5fc11

Funny string values of boolean

a7f0b29

Other randomness

a3d9402

Coercion tests

821f3a6

Use other assertion

9ab9151

Skip text and drop old method

b37d90c

Use simpler metod

80d3e20

nik9000 added 7 commits March 18, 2021 15:46

Drop commented out stuff while we wait on bug

75f4f0c

Merge branch 'master' into random_random_fetch

d7bfa96

Sigh

43f9df6

Only arrays for equality.

Checkstyle

e9bbb22

Merge branch 'master' into random_random_fetch

c712c6f

Moar extras

08224f6

Merge branch 'master' into random_random_fetch

9ebe3b8

romseygeek reviewed Mar 22, 2021

View reviewed changes

Rename

ff51867

romseygeek approved these changes Mar 23, 2021

View reviewed changes

nik9000 added 2 commits March 24, 2021 13:01

Merge branch 'master' into random_random_fetch

7ad821d

Doc doc doc doc doc

463171e

nik9000 merged commit 91c700b into elastic:master Mar 24, 2021

nik9000 added the backport pending label Mar 24, 2021

nik9000 removed the backport pending label Mar 24, 2021

nik9000 mentioned this pull request Mar 26, 2021

More super randomized fields tests #70929

Open

24 tasks

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

Super randomized tests for fetch fields API #70278

Super randomized tests for fetch fields API #70278

Conversation

nik9000 commented Mar 10, 2021

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

elasticmachine commented Mar 15, 2021

nik9000 commented Mar 15, 2021

not-napoleon 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

nik9000 commented Mar 15, 2021

jtibshirani 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtibshirani Mar 16, 2021 • edited Loading

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 Mar 17, 2021

nik9000 commented Mar 18, 2021

romseygeek 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

nik9000 commented Mar 22, 2021

romseygeek 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

jtibshirani Mar 16, 2021 •

edited

Loading