Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Source Filtering #72

Closed
dclemenzi opened this issue Jun 10, 2018 · 5 comments
Closed

Source Filtering #72

dclemenzi opened this issue Jun 10, 2018 · 5 comments

Comments

@dclemenzi
Copy link
Contributor

The following setting:

ElasticDataStoreFactory.SOURCE_FILTERING_ENABLED

Enables source filtering but it appears to add all of the attributes which defeats the purpose.

        if (dataStore.isSourceFilteringEnabled()) {
            // add source includes
            setSourceIncludes(searchRequest);
        }
...

    private void setSourceIncludes(final ElasticRequest searchRequest) throws IOException {
        final ElasticDataStore dataStore = getDataStore();
        final List<ElasticAttribute> attributes = dataStore.getElasticAttributes(entry.getName());
        for (final ElasticAttribute attribute : attributes) {
            if (attribute.isUse() && attribute.isStored()) {
                searchRequest.addField(attribute.getName());
            } else if (attribute.isUse()) {
                searchRequest.addSourceInclude(attribute.getName());
            }
        }
    }

Is there a currently a means to only include attributes specified in query.getProperties().

@dclemenzi
Copy link
Contributor Author

If not I am happy to add it. Wanted to make sure I am not missing something first.

@dclemenzi
Copy link
Contributor Author

Don't want to break what is already there so I added another feature flag to do what we need for bandwidth performance.

       if (dataStore.isSourceFilteringEnabled()) {
           if (dataStore.isSourceFilteringProperties()) {
               for (String property : query.getPropertyNames()) {
                   searchRequest.addSourceInclude(property);
               }
           } else {
               // add source includes
               setSourceIncludes(searchRequest);
           }
       }

If there is another means to enable this without this code change please let us know. Thanks.

@sjudeng
Copy link
Contributor

sjudeng commented Jun 10, 2018

No I think the original implementation is wrong to always include all fields from the feature type. When source filtering is enabled and query.getProperties() != Query.ALL_PROPERTIES then it should limit to attributes in query.getProperties() as you indicate. Good catch.

Can you implement this under the existing isSourceFilteringEnabled configuration (no need to add another config)?

@dclemenzi
Copy link
Contributor Author

My concern would be breakage with existing code that is expecting to get all fields back. Since your good with it I will move the implementation under the existing feature flag and submit a pull request.

Have run into a few things but overall this plugin has saved us a ton of work and is very well done. Thank you so much for all your effort and quick responses!!!

@dclemenzi dclemenzi mentioned this issue Jun 11, 2018
Closed
dclemenzi added a commit to dclemenzi/elasticgeo that referenced this issue Jun 11, 2018
sjudeng added a commit that referenced this issue Jun 11, 2018
#72 Limited the source filtering to the properties specified by the q…
@sjudeng
Copy link
Contributor

sjudeng commented Jun 12, 2018

Fixed in #76

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants