Fix and optimize "Add XContentFieldFilter (#81970)" #83152

mushao999 · 2022-01-26T15:27:27Z

In "Add XContentFieldFilter (#81970)"，I introduced XContentFieldFilter to improve filtering performance for SourceFieldMapper and ShardGetService. But no tests is added. Something unexpected was then found in issue #82891 .So changes in that PR was reverted in #82897 .

This PR fix the bug reported in @82891 and add some tests that similiar to XContentMapValuesTests to make new filter behave in the same way with the old map filter.
We found that new filter will not preserve empty array and object. @nik9000 had fixed this in jackson-core repository : Allow TokenFilter to preserve empty FasterXML/jackson-core#729. We marked the related tests to @AwaitsFix to wait for this patch to be added into ES.
We found that dot in fieldName is not supported in the new filter. So we made some changes to support this feature.(moved to FilterPathBasedFilter support match fieldname with dot #83178 )
We also add some null and empty check for the new filter.

This commit introduces XContentFieldFilter, which applies field includes/excludes to XContent without having to realise the xcontent itself as a java map. SourceFieldMapper and ShardGetService are cut over to use this class

…ject

nik9000 · 2022-01-26T15:37:28Z

Neat! This really is almost the same as #83148. Cool.

@elasticmachine, ok to tests.

nik9000

I wonder if we're stuck behind:

We found that new filter will not preserve empty array and object. @nik9000 had fixed this in jackson-core repository : Allow TokenFilter to preserve empty FasterXML/jackson-core#729. We marked the related tests to @AwaitsFix to wait for this patch to be added into ES.

But, otherwise, it makes sense to me.

nik9000 · 2022-01-26T15:38:10Z

libs/x-content/src/main/java/org/elasticsearch/xcontent/XContentParserConfiguration.java

        );
    }

+    public XContentParserConfiguration withSupportDotInFieldName(boolean supportDotInFieldName) {
+        return new XContentParserConfiguration(registry, deprecationHandler, restApiVersion, includes, excludes, supportDotInFieldName);
+    }


I like the name I used for this variable more. And I like forcing folks to configure it when configuring filtering. That way then they build the filter they have to think about it.

I like yours too. Forgive my poor Englisth, LOL

nik9000 · 2022-01-26T15:39:28Z

libs/x-content/src/main/java/org/elasticsearch/xcontent/support/filtering/FilterPath.java

@@ -71,6 +71,17 @@ private boolean isFinalNode() {
     * @return true if the name equal a final node, otherwise return false
     */
    boolean matches(String name, List<FilterPath> nextFilters) {
+        return matches(name, nextFilters, false);


I don't think we need to keep the two argument version of this method.

Exactly! I did this because I want to make the changes as smaller as possible. I will change it.

nik9000 · 2022-01-26T15:41:45Z

libs/x-content/src/main/java/org/elasticsearch/xcontent/support/filtering/FilterPath.java

@@ -99,6 +110,23 @@ boolean matches(String name, List<FilterPath> nextFilters) {
            nextFilters.add(this);
        }

+        // support dot in path
+        if (supportDotInFieldName && nextFilters.isEmpty()) {


I haven't really wrapped my head around when nextFilters is empty. That's why I did mine in the caller. Could you explain what's up here?

NextFilters are used to filter nested sub objects if current fieldName is matched and current FilterPathNode is not a final node. So if nextFilters is not empty, current fieldName must have been matched.

And if nextFilters is not empty and our code reach here, current fieldName is not matched. We can then check whether it is the dot case or not.

I see! So if nextFilters is non-empty that means that some other filter has already matched - it could be from a previous pattern.

Now, here's a question! Is it valid to skip? What if we had an object like {"foo.bar": {"baz": 2}} and two patterns foo.bar.baz and f*r.bort. If the second one matches first we won't try to do the dots in field names thing and never notice the second one.

Now, my PR has similar but different problems for sure. If you turn on the mode it'll never match f*r for foo.bar keys. I'm realizing now this is wrong. Your approach here of running the test both times is good.

nik9000 · 2022-01-26T15:42:23Z

...ontent/src/main/java/org/elasticsearch/xcontent/support/filtering/FilterPathBasedFilter.java

@@ -42,6 +42,13 @@ public String toString() {

    private final boolean inclusive;

+    private boolean supportDotInFieldName = false;
+
+    public FilterPathBasedFilter supportDotInFieldName(boolean supportDotInFieldName) {


I'd prefer adding another boolean parameter to the ctor.

nik9000 · 2022-01-26T15:43:26Z

libs/x-content/src/test/java/org/elasticsearch/xcontent/support/filtering/FilterPathTests.java

+        filterPaths = FilterPath.compile(singleton("foo.bar.test"));
+        assertFalse(filterPaths[0].matches("foo.bar.text", nextFilters, true));
+        assertEquals(nextFilters.size(), 0);
+    }


I think it's worth taking some of the tests from my PR into this one. Just to test the filtering "above" this layer.

Thanks , that's what I intended to do.

elasticmachine · 2022-01-26T15:51:02Z

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

…into a new PR elastic#83178

mushao999 · 2022-01-27T07:42:36Z

The match fieldname with dot part was reverted and move to PR #83178 @nik9000 @romseygeek

Current `FilterPathBasedFilter` does not support match fieldName with dot. This PR merges the changes of #83148 and #83152 together to add the ability of `matching fieldName with dot` to the `FilterPathBasedFilter`.

nik9000 · 2022-01-28T17:14:10Z

Now that #83178 is in I think you'll probably want to merge it into this.

mushao999 · 2022-01-29T03:22:43Z

Now that #83178 is in I think you'll probably want to merge it into this.

@nik9000 @romseygeek #83178 is merged and related code and tests are updated, thanks.

romseygeek

Hi @mushao999, thanks for working on this. I left a couple of comments but it's looking very close.

server/src/test/java/org/elasticsearch/common/xcontent/support/XContentFieldFilterTests.java

server/src/main/java/org/elasticsearch/common/xcontent/XContentFieldFilter.java

romseygeek

LGTM, thanks for your work on this. I will merge tomorrow morning.

javanna · 2022-03-29T11:32:28Z

server/src/main/java/org/elasticsearch/index/get/ShardGetService.java

@@ -253,15 +249,11 @@ private GetResult innerGetLoadFromStoredFields(
            if (fetchSourceContext.fetchSource() == false) {
                source = null;
            } else if (fetchSourceContext.includes().length > 0 || fetchSourceContext.excludes().length > 0) {
-                Map<String, Object> sourceAsMap;
                // TODO: The source might be parsed and available in the sourceLookup but that one uses unordered maps so different.
                // Do we care?


@nik9000 @romseygeek is this TODO still valid here? It seems like the code around it changed quite a bit.

SourceLookup in ShardGetSevice has been removed in #20158, so this todo is invalid now.
I open #85921 to remove another similar todo in this file, please help to reivew it.@javanna

668d557 added some `todo` in `ShardGetService` suggest that source might parsed and available in the sourceLookup. https://github.com/elastic/elasticsearch/blob/668d55758b751e848f2b82299e2c5e74af55cd12/src/main/java/org/elasticsearch/index/get/ShardGetService.java#L315 https://github.com/elastic/elasticsearch/blob/668d55758b751e848f2b82299e2c5e74af55cd12/src/main/java/org/elasticsearch/index/get/ShardGetService.java#L431 However ,`SourceLookup` in `ShardGetService` has been remove in #20158 which made these `todo` invalid. One of them has been removed in #83152 This PR remove the left invalid `todo` in `ShardGetService`

mushao999 added 4 commits January 24, 2022 14:21

Add XContentFieldFilter (elastic#81970)

cbcfe0e

This commit introduces XContentFieldFilter, which applies field includes/excludes to XContent without having to realise the xcontent itself as a java map. SourceFieldMapper and ShardGetService are cut over to use this class

fix XContentFieldFilter bug when source is filtered to be an empty ob…

58f5615

…ject

FilterPathBasedFilter support dot in field name

56983ff

XContentFieldFilter support source to be null or empty

e5a322a

elasticsearchmachine added v8.1.0 external-contributor Pull request authored by a developer outside the Elasticsearch team labels Jan 26, 2022

nik9000 reviewed Jan 26, 2022

View reviewed changes

nik9000 self-assigned this Jan 26, 2022

nik9000 added the :Search Foundations/Mapping Index mappings, including merging and defining field types label Jan 26, 2022

elasticmachine added the Team:Search Meta label for search team label Jan 26, 2022

nik9000 requested a review from romseygeek January 26, 2022 15:51

nik9000 added the >enhancement label Jan 26, 2022

nik9000 mentioned this pull request Jan 26, 2022

Handle fields.with.dots in routing_path #83148

Merged

pugnascotia assigned nik9000 and unassigned nik9000 Jan 26, 2022

mushao999 mentioned this pull request Jan 26, 2022

FilterPathBasedFilter support match fieldname with dot #83178

Merged

mushao999 added 2 commits January 27, 2022 15:29

revert <FilterPathBasedFilter support dot in field name> and move it …

37f19a5

…into a new PR elastic#83178

fix XContentFieldFilterTests typo

fd054d3

mushao999 added 2 commits January 27, 2022 17:13

Merge branch 'master' into fix_Add_XContentFieldFilter

3b244ed

update AwaitsFix bugUrl

3789d76

mushao999 added 2 commits January 29, 2022 10:19

Merge branch 'master' into fix_Add_XContentFieldFilter

2cf2266

update after elastic#83178 is merged

77c0c9a

romseygeek suggested changes Jan 31, 2022

View reviewed changes

server/src/test/java/org/elasticsearch/common/xcontent/support/XContentFieldFilterTests.java Outdated Show resolved Hide resolved

server/src/main/java/org/elasticsearch/common/xcontent/XContentFieldFilter.java Show resolved Hide resolved

mushao999 added 3 commits February 2, 2022 00:07

update readability

5edf685

update

df9cbc4

Merge branch 'master' into fix_Add_XContentFieldFilter

6a8f3a3

romseygeek approved these changes Feb 1, 2022

View reviewed changes

romseygeek merged commit 919ed3a into elastic:master Feb 2, 2022

javanna mentioned this pull request Mar 29, 2022

Do not fail on duplicated content field filters #85382

Merged

javanna reviewed Mar 29, 2022

View reviewed changes

mushao999 mentioned this pull request Apr 15, 2022

Remove invalid todo in ShardGetService #85921

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix and optimize "Add XContentFieldFilter (#81970)" #83152

Fix and optimize "Add XContentFieldFilter (#81970)" #83152

mushao999 commented Jan 26, 2022 •

edited

Loading

nik9000 commented Jan 26, 2022

nik9000 left a comment

nik9000 Jan 26, 2022

mushao999 Jan 26, 2022

nik9000 Jan 26, 2022

mushao999 Jan 26, 2022

nik9000 Jan 26, 2022

mushao999 Jan 26, 2022

mushao999 Jan 26, 2022

nik9000 Jan 26, 2022

nik9000 Jan 26, 2022

nik9000 Jan 26, 2022

mushao999 Jan 26, 2022

nik9000 Jan 26, 2022

elasticmachine commented Jan 26, 2022

mushao999 commented Jan 27, 2022

nik9000 commented Jan 28, 2022

mushao999 commented Jan 29, 2022

romseygeek left a comment

romseygeek left a comment

javanna Mar 29, 2022

mushao999 Apr 15, 2022 •

edited

Loading

Fix and optimize "Add XContentFieldFilter (#81970)" #83152

Fix and optimize "Add XContentFieldFilter (#81970)" #83152

Conversation

mushao999 commented Jan 26, 2022 • edited Loading

nik9000 commented Jan 26, 2022

nik9000 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

elasticmachine commented Jan 26, 2022

mushao999 commented Jan 27, 2022

nik9000 commented Jan 28, 2022

mushao999 commented Jan 29, 2022

romseygeek left a comment

Choose a reason for hiding this comment

romseygeek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mushao999 Apr 15, 2022 • edited Loading

Choose a reason for hiding this comment

mushao999 commented Jan 26, 2022 •

edited

Loading

mushao999 Apr 15, 2022 •

edited

Loading