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

Make keyword a family of field types #58315

Merged
merged 5 commits into from
Jun 24, 2020
Merged

Conversation

markharwood
Copy link
Contributor

@markharwood markharwood commented Jun 18, 2020

This change introduces a familyTypeName method to MappedFieldType for use in the field capabilities API.
The constant_keyword and wildcard fields override this to return keyword to describe the family to which they belong.

Relates to #53175

@markharwood markharwood added the :Search Foundations/Mapping Index mappings, including merging and defining field types label Jun 18, 2020
@elasticmachine
Copy link
Collaborator

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

@markharwood
Copy link
Contributor Author

@costin - do you have any pointers for the best place to reset the SQL expectations that are failing here (see test output) now that I've changed the field capabilities as per what we agreed in #53175 ?

@jimczi
Copy link
Contributor

jimczi commented Jun 18, 2020

do you have any pointers for the best place to reset the SQL expectations that are failing here (see test output) now that I've changed the field capabilities as per what we agreed in #53175 ?

I guess we have no choice but to replace all instance of constant_keyword by keyword in SQL. It seems that most of the errors are on the test expectation that should be changed to keyword or KEYWORD depending on the context ?

@markharwood
Copy link
Contributor Author

I got a little lost chasing expected vs actuals discrepancies when it looks like a lot of the expectations might come from *csv-spec files.

@costin
Copy link
Member

costin commented Jun 18, 2020

@markharwood you're on the right path. The *.csv-spec files are the ones that indicate what we expect. For example:
org.elasticsearch.xpack.sql.qa.security.JdbcCsvSpecIT.test {alias.testDescribeAlias} indicates there's a failure inside alias.csv-spec for describeAlias, namely here.

Hope this helps.

@costin
Copy link
Member

costin commented Jun 18, 2020

Tagging @astefan for awareness since once this PR lands, we should remove the constant_keyword handling.

Copy link
Contributor

@astefan astefan left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@jimczi jimczi left a comment

Choose a reason for hiding this comment

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

LGTM, we should also update the documentation in _field_caps to explain the keyword family ?

@markharwood markharwood merged commit cdc1be1 into elastic:master Jun 24, 2020
markharwood added a commit to markharwood/elasticsearch that referenced this pull request Jun 24, 2020
…58315)

Introduces a new method on `MappedFieldType` to return a family type name which defaults to the field type.
Changes `wildcard` and `constant_keyword` field types to return `keyword` for field capabilities.

Relates to elastic#53175
markharwood added a commit that referenced this pull request Jun 24, 2020
…58483)

Introduces a new method on `MappedFieldType` to return a family type name which defaults to the field type.
Changes `wildcard` and `constant_keyword` field types to return `keyword` for field capabilities.

Relates to #53175
@jtibshirani
Copy link
Contributor

@markharwood perhaps we should make a note in the migration/ breaking changes docs for 7.x explaining that constant_keyword is now presented as keyword.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement :Search Foundations/Mapping Index mappings, including merging and defining field types Team:Search Meta label for search team v7.9.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants