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

SQL: do not ignore all fields whose names start with underscore #36214

Merged
merged 8 commits into from Dec 13, 2018

Conversation

astefan
Copy link
Contributor

@astefan astefan commented Dec 4, 2018

This is a fix for #36206.
The solution is to ignore the fields names starting with _ only if their type names also starts with _ (a particularity of ES meta fields).

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search

Copy link
Contributor

@matriv matriv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nik9000
Copy link
Member

nik9000 commented Dec 4, 2018

The test failure comes from the docs build. It installs the mapper-size plugin. Installing the mapper-size plugin creates a new field that comes back from _field_caps which SQL uses to get the schema for the query. This field is added to _field_caps whether or not the field is enabled in the mapping. This causes the tests to fail because suddenly the _size field starts coming back all over the place.

@astefan
Copy link
Contributor Author

astefan commented Dec 7, 2018

@nik9000 is the mapper-size plugin installed because all the plugins have to be installed or because it is actually needed? Better said, it's part of the procedure to install all plugins no matter what or can we skip it?

In case we need to keep it, @atorok do you know a way of running DocsClientYamlTestSuiteIT for this test only in a cluster where mapper-size plugin is not installed or somehow skip this plugin for this test or any other wild idea?

@nik9000
Copy link
Member

nik9000 commented Dec 7, 2018

The mapper-size plugin is installed here because we run tests for its docs. We install almost all of the plugins when we're testing the docs.

I don't think we should run the SQL tests in a cluster without the mapper-size plugin installed. I think SQL shouldn't include the _size field unless it is enabled. I don't really know how to do that though.

Copy link
Member

@costin costin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

// Skip internal fields (name starting with underscore and its type reported by field_caps starts with underscore
// as well). A meta field named "_version", for example, has the type named "_version".
// Also, skip any of the blacklisted field names.
if (false == (name.startsWith("_") && entry.getValue().values().stream().anyMatch(f -> f.getType().startsWith("_"))) &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potentially move the types declaration above the if to reduce the if:
types.values().stream().anyMatch()

Another option would be to do the type check further down when it is encountered - this would avoid creating a stream for every single field.

Is the name check still necessary ? Wouldn't the type check be enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@costin I have pushed a new commit with a slightly different approach.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems better this way.

Copy link
Member

@costin costin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@astefan astefan merged commit 347468e into elastic:master Dec 13, 2018
astefan added a commit that referenced this pull request Dec 13, 2018
* Do not ignore fields whose names start with underscore, unless they are
meta fields.
* Filter out _size field.

(cherry picked from commit 347468e)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants