Update BooleanFieldMapper(boolean fields) to support ignore_malformed… #29522

mP1 · 2018-04-15T21:24:19Z

…=true

Also skips passed arrays and objects where boolean expected
Lots of tests for all data types and strict/lenient parser
Introduced and use pkg private constants replacing literals within BooleanFieldMapper

Have you signed the contributor license agreement?
YES
Have you followed the contributor guidelines?
YES
If submitting code, have you built your formula locally prior to submission with gradle check?
YES
If submitting code, is your pull request against master? Unless there is a good reason otherwise, we prefer pull requests against master and will backport as needed.
YES, against sundays master, will rebase again as required.
If submitting code, have you checked that your submission is for an OS that we support?
YES (im OSX)
If you are submitting this code for a class then read our policy for that.
NO not in a class

elasticmachine · 2018-04-16T10:13:12Z

Pinging @elastic/es-search-aggs

mP1 · 2018-04-16T11:30:17Z

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

+                        value = fieldType().nullValue();
+                    }
+                } else {
+                    if (indexCreatedVersion.onOrAfter(Version.V_6_0_0_alpha1)) {


when i grabbed master the other day this was included, and now its gone. Im not sure if i should keep the lenient parsing support.

Will remove if required.

cbuescher

Hi @mP1 thanks for opening this PR. I left a few questions and remarks, I really like the detailed work on the tests but didn't go into depth on comments there yet because I want to confirm a few things about the general goal of this PR first.

You mention this change relates to #11498 but I couldn't quiet get the relation yet. If I understand correctly, the issue deals with ignore_malformed on integer fields while this is about the boolean fields? Could you elaborate?

I think the PR looks good on many levels, but I'm not completely sure if adding this option to boolean fields is something that was discussed in any issue already and is something that we really want to do, if you can point me to any comment or discussion about this that would be helpful. Otherwise I think we should discuss the generall goals before further diving into further reviews.

cbuescher · 2018-04-16T11:11:14Z

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

@@ -115,6 +144,9 @@ public BooleanFieldMapper build(BuilderContext context) {
                    }
                    builder.nullValue(XContentMapValues.nodeBooleanValue(propNode, name + ".null_value"));
                    iterator.remove();
+                } else if (propName.equals("ignore_malformed")) {
+                    builder.ignoreMalformed(XContentMapValues.nodeBooleanValue(propNode,name + ".ignore_malformed"));


nit: whitespace between propNode,name

cbuescher · 2018-04-16T11:14:04Z

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

+            if (context.indexSettings() != null) {
+                return new Explicit<>(IGNORE_MALFORMED_SETTING.get(context.indexSettings()), false);
+            }
+            return NumberFieldMapper.Defaults.IGNORE_MALFORMED;


I'd probably introduce an own default value for BooleanFieldMapper, that way the two defaults are not tied to each other (or should they be?)

Fixed introduced Defaults.IGNORE_MALFORMED for BooleanFieldMapper.Defaults.

cbuescher · 2018-04-16T11:21:44Z

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

+                            // need to consume array/object, otherwise parser doesnt skip to the next field as expected
+                            parser.skipChildren();
+
+                            throw new IllegalArgumentException(parser.currentToken().toString());


Is this the reason for the try/catch that was introduced? If so, can the control flow somehow be done withouth thrrowing the exception here?

see my longer comment slightly below. if the legacy stuff is removed this will be simplified and you will need to double check again.

Consider the throw a nasty way to keep flow control while i await your commentary whether i should keep/remove legacy lenient support that was recently removed.

cbuescher · 2018-04-16T11:23:39Z

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

+                        value = fieldType().nullValue();
+                    }
+                } else {
+                    if (indexCreatedVersion.onOrAfter(Version.V_6_0_0_alpha1)) {


Wondering what the general goal is for backwards-compatiblity here? Should this change got to some 6.x branch? If yes, I think its version should be used here, otherwise probably V_7_0_0?

When i grabbed master the other day, this entire block was present with all the version checking and logging.

When i rebased earlier today, the legacy check/handling was removed.

Should i remove the legacy code and my associated tests ? Just wondering if the legacy should be remove entirely ?

Your comments for lines above/below with this block are all legacy code which i have kept. I only added the try/catch and skipChildren bit to fix "skipping" over the object/array so the parser would be the proper spot to continue parsing the next field.

cbuescher · 2018-04-16T11:33:42Z

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

+
+                            if (!parser.isBooleanValue()) {
+                                String rawValue = parser.text();
+                                deprecationLogger.deprecated("Expected a boolean for property [{}] but got [{}]",


Could you explain why you log a deprecation her? Might be missing some context, but I thought this PR wasn't about deprecation but about adding some option to the field mapper.

see my comment above about legacy lenient support recently removed.

mP1 · 2018-04-16T11:58:27Z

@cbuescher

You mention this change relates to #11498 but I couldn't quiet get the relation yet. If I understand correctly, the issue deals with ignore_malformed on integer fields while this is about the boolean fields? Could you elaborate?

yeh probably right was probably a mistake to quote 11498.

I think the PR looks good on many levels, but I'm not completely sure if adding this option to boolean fields is something that was discussed in any issue already and is something that we really want to do, if you can point me to any comment or discussion about this that would be helpful. Otherwise I think we should discuss the generall goals before further diving into further reviews.

Ignore the mention of 11498.

Its just a simple fix for BooleanFieldMapper(boolean fields) so ignore_malformed=true defined for a single field mapping works as expected.

mP1 · 2018-04-16T12:00:27Z

@cbuescher

The real issue before you review too hard is whether i should remove the lenient handling which i kept in my commit. As i said before when i grabbed master the other day it was still there, and then it disappeared and i wasnt sure whether i should keep it and it looked kind of useful and removing it would break legacy support others might be expecting.

keep legacy handling of booleans (ive got tests for that now)
delete legacy handling of booleans (remove my tests)

Which should i do 1) or 2) ?

The remove legacy support commit is 2018/5/11 Adrien Grand #29224 4918924.

Pretty sure you wont the legacy stuff removed, but please confirm.

…=true - Also skips passed arrays and objects where boolean expected - Lots of tests for all data types and strict/lenient parser - Introduced and use pkg private constants replacing literals within BooleanFieldMapper

mP1 · 2018-04-16T13:06:29Z

@cbuescher

Hi mate, i removed the legacy lenient boolean support which makes my BooleanFieldMapper work as expected but with the improvement mentioned in the PR description.

Please forget the previous comments/questions and lets start again!

cbuescher

Thanks for removing the legacy checks, I think this code should then go to master only first. If we backport, those would need to be take care of separately.
I agree with most of the code changes and I really like the level of detail in the added tests, but I think the test code is a bit too scattered, making each test hard to read. I left a few comments as an example but think this applies to other places as well. Before diving into those changes again, I'd like to make sure we agree on adding the ignore_malformed option to boolean fields at all. If there is no issue yet that discussed this we need to get an agreement of this first.

cbuescher · 2018-04-16T13:22:05Z

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

+                        parser.skipChildren();
+                    }
+                } else {
+                    // if value is really an array/object/number let parser crash and burn!


;-) nice comment, I'd like it to stay but could you make an addition that this should be the exceptional case? Maybe something like "Usually we expect a boolean here, but ..."

cbuescher · 2018-04-16T13:42:29Z

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

@@ -130,26 +132,176 @@ public void testSerialization() throws IOException {
        assertEquals("{\"field\":{\"type\":\"boolean\",\"doc_values\":false,\"null_value\":true}}", Strings.toString(builder));
    }

-    public void testParsesBooleansStrict() throws IOException {
-        String mapping = Strings.toString(XContentFactory.jsonBuilder()
+    public void test_null() throws Exception{


I like the level of detail in these tests, the degree of splitting them in into so many test methods and helpers is maybe a little bit hard to read. The following are just some suggestions of how to maybe group some of the tests.
e.g the null, true and false test could be grouped into one case that checks allowed fields fort the allowMalformed field.

cbuescher · 2018-04-16T13:43:43Z

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

+        this.parseFails("0");
+    }
+
+    public void test_1() throws Exception{


Could you group all the failure tests for different input strings into one tests that e.g. loops over all of the values you want to check?

This group has several values and its better to test them individually, so we get failures for each case, if it becomes one test, then all we know is that it fails the first which is less helpful.

By creating these helpers its very simple to add a new test for a new input, most of each test is boilerplate, that leaves each test method to be down to the bare minimum.

Take an input(value, ignore_malformed(Y/N), execute, and some expectation (fail or expected value).

Super clean, super concise, super easy to add more tests for more inputs as necessary.

cbuescher · 2018-04-16T14:01:07Z

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

+            .endObject());
+    }
+
+    public void test_object_IgnoreMalformed() throws Exception{


Although the code is clear, the level of indirection here makes it hard for the reader to figure out that this (and similar other) test does. As a suggestion, what about combining test_object/test_object_IgnoreMalformed, then the code from sourceWithObject can be inlined in the test case. Also I would make the assertion on field1 and field2 explicit, even if that means a few more lines of code. In this case I would trade repetition for readability.

As a suggestion, what about combining test_object/test_object_IgnoreMalformed, then the code from sourceWithObject can be inlined in the test case.

Again if you combine them into one method, and it fails you dont know if the "2nd" test is also broken because the test fails and aborts on the first.

More small tests are better than one bigger test that fails on the first.

mP1 · 2018-04-16T14:24:39Z

Before diving into those changes again, I'd like to make sure we agree on adding the ignore_malformed option to boolean fields at all

yes thats the purpose of this commit. Its concentrating only on boolean fields.

cbuescher · 2018-04-16T14:26:07Z

If there is no issue yet that discussed this we need to get an agreement of this first

I just found the summary of a longer discussion of ignore_malformed in #12366 #12366 (comment), I think the same arguments apply here and we need to get to an agreement whether we want to support this option on boolean fields first. Given the amount of discussions this might take some while, so I'm marking this as "stalled" in the meantime.
@mP1 thanks for the PR, the fix itself and the testing looks good so far but we need to get agreement on this first.

mP1 · 2018-04-16T14:49:08Z

I just found the summary of a longer discussion of ignore_malformed in #12366 #12366 (comment), I think the same arguments apply here and we need to get to an agreement whether we want to support this option on boolean fields first.

Currently ignore_malformed is a concept or feature that is supported on some types and thats confusing/annoying because we want it for all types to help solve the problem of skipping bad field values.

Some people in #12366 for example have suggested other hack(unofficial) solutions, which might disappear and cause them future problems. It seems to me that ignore_malformed with all its nasty reasons exists and should be present on all field types, if the user choses to use it.

There are a few other tickets discussing ignore_malformed for other types, and im going to guess they all probably want this feature for all types. I know i do and to try and help ive created another PR for text -> #29532

cbuescher · 2018-04-20T14:02:04Z

This was discussed internally and we agreed that we only want to proceed once #29494 has been fully discussed and implemented.

javanna · 2018-08-16T13:23:10Z

I am removing the stalled label as the #29494 is now closed.

cbuescher added review :Search Foundations/Mapping Index mappings, including merging and defining field types labels Apr 16, 2018

mP1 commented Apr 16, 2018

View reviewed changes

cbuescher reviewed Apr 16, 2018

View reviewed changes

cbuescher self-assigned this Apr 16, 2018

cbuescher requested changes Apr 16, 2018

View reviewed changes

cbuescher added the discuss label Apr 16, 2018

cbuescher added the stalled label Apr 16, 2018

cbuescher mentioned this pull request Apr 16, 2018

TextFieldMapper honour ignore_malformed and handle object/array values. #29532

Closed

cbuescher removed the discuss label Apr 20, 2018

javanna removed the stalled label Aug 16, 2018

rjernst removed the review label Oct 10, 2018

tomcallahan closed this Dec 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update BooleanFieldMapper(boolean fields) to support ignore_malformed… #29522

Update BooleanFieldMapper(boolean fields) to support ignore_malformed… #29522

mP1 commented Apr 15, 2018 •

edited

Loading

elasticmachine commented Apr 16, 2018

mP1 Apr 16, 2018

cbuescher left a comment

cbuescher Apr 16, 2018

cbuescher Apr 16, 2018

mP1 Apr 16, 2018

cbuescher Apr 16, 2018

mP1 Apr 16, 2018

mP1 Apr 16, 2018

cbuescher Apr 16, 2018

mP1 Apr 16, 2018

cbuescher Apr 16, 2018

mP1 Apr 16, 2018

mP1 commented Apr 16, 2018 •

edited

Loading

mP1 commented Apr 16, 2018 •

edited

Loading

mP1 commented Apr 16, 2018

cbuescher left a comment

cbuescher Apr 16, 2018

cbuescher Apr 16, 2018

cbuescher Apr 16, 2018

mP1 Apr 16, 2018

mP1 Apr 16, 2018

cbuescher Apr 16, 2018

mP1 Apr 16, 2018

mP1 commented Apr 16, 2018

cbuescher commented Apr 16, 2018

mP1 commented Apr 16, 2018

cbuescher commented Apr 20, 2018 •

edited

Loading

javanna commented Aug 16, 2018

Update BooleanFieldMapper(boolean fields) to support ignore_malformed… #29522

Update BooleanFieldMapper(boolean fields) to support ignore_malformed… #29522

Conversation

mP1 commented Apr 15, 2018 • edited Loading

elasticmachine commented Apr 16, 2018

Choose a reason for hiding this comment

cbuescher 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

mP1 commented Apr 16, 2018 • edited Loading

mP1 commented Apr 16, 2018 • edited Loading

mP1 commented Apr 16, 2018

cbuescher 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

mP1 commented Apr 16, 2018

cbuescher commented Apr 16, 2018

mP1 commented Apr 16, 2018

cbuescher commented Apr 20, 2018 • edited Loading

javanna commented Aug 16, 2018

mP1 commented Apr 15, 2018 •

edited

Loading

mP1 commented Apr 16, 2018 •

edited

Loading

mP1 commented Apr 16, 2018 •

edited

Loading

cbuescher commented Apr 20, 2018 •

edited

Loading