Skip to content

Add tests for the supported query types.#35319

Merged
jtibshirani merged 5 commits intoelastic:object-fieldsfrom
jtibshirani:json-field-tests
Nov 8, 2018
Merged

Add tests for the supported query types.#35319
jtibshirani merged 5 commits intoelastic:object-fieldsfrom
jtibshirani:json-field-tests

Conversation

@jtibshirani
Copy link
Copy Markdown
Contributor

@jtibshirani jtibshirani commented Nov 6, 2018

The following query types should now be covered:

  • term, terms and terms_set
  • prefix
  • range
  • match and multi_match
  • query_string and simple_query_string
  • exists

I preferred unit tests as much as possible, but certain query types (like match and query_string) were more natural to check through integration tests.

@jtibshirani jtibshirani added >test Issues or PRs that are addressing/adding tests :Search Foundations/Mapping Index mappings, including merging and defining field types labels Nov 6, 2018
@elasticmachine
Copy link
Copy Markdown
Collaborator

elasticmachine commented Nov 6, 2018

Pinging @elastic/es-search-aggs

@jtibshirani jtibshirani mentioned this pull request Nov 6, 2018
Closed
11 tasks
jtibshirani
jtibshirani commented Nov 6, 2018
View reviewed changes
server/src/main/java/org/elasticsearch/index/mapper/JsonFieldMapper.java Outdated
Copy link
Copy Markdown
Contributor Author

@jtibshirani jtibshirani Nov 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

We could fix this instead of disallowing unbounded range queries, but I wasn't sure the extra complexity was worth it. It's not clear how useful this range functionality will be right now, especially without support for numerics.

Copy link
Copy Markdown
Contributor

@colings86 colings86 Nov 7, 2018

Choose a reason for hiding this comment

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

I feel like this limitation is ok for now. We can revisit this later if necessary

@jtibshirani jtibshirani force-pushed the json-field-tests branch 4 times, most recently from 909c276 to f693e46 Compare November 6, 2018 23:23
colings86
colings86 reviewed Nov 7, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@colings86 colings86 left a comment

Choose a reason for hiding this comment

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

I left a comment but otherwise I think this is good. I'd like @romseygeek to take a look too though

server/src/main/java/org/elasticsearch/index/mapper/JsonFieldMapper.java Outdated
Copy link
Copy Markdown
Contributor

@colings86 colings86 Nov 7, 2018

Choose a reason for hiding this comment

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

I feel like this limitation is ok for now. We can revisit this later if necessary

server/src/test/java/org/elasticsearch/index/mapper/KeyedJsonFieldTypeTests.java Outdated
Copy link
Copy Markdown
Contributor

@colings86 colings86 Nov 7, 2018

Choose a reason for hiding this comment

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

should we also check that we do the right thing if the bounds are inclusive?

Copy link
Copy Markdown
Contributor Author

@jtibshirani jtibshirani Nov 7, 2018

Choose a reason for hiding this comment

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

👍

romseygeek
romseygeek reviewed Nov 7, 2018
View reviewed changes
server/src/test/java/org/elasticsearch/search/query/ExistsIT.java Outdated
Copy link
Copy Markdown
Contributor

@romseygeek romseygeek Nov 7, 2018

Choose a reason for hiding this comment

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

Not especially related to the current PR, but how is case normalization being handled? The normal HTTP header here is Content-type with a capital C; is there a lowercase filter being applied anywhere, or is that being ignored for the moment?

Copy link
Copy Markdown
Contributor Author

@jtibshirani jtibshirani Nov 7, 2018

Choose a reason for hiding this comment

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

We don't apply any lowercasing to the object keys. This is consistent with the way normal field names are handled in the mapping, which are not lowercased (you could have two distinct fields for example, one named Content-Type and the other content-type). Do you think an option to automatically case-fold the keys would be useful?

Separately for field values (not keys), keyword fields provide a normalizer option. This is something we could consider, although it might need to be designed differently as json fields can contain non-string values.

romseygeek
romseygeek approved these changes Nov 7, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@romseygeek romseygeek left a comment

Choose a reason for hiding this comment

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

LGTM. I have one completely unrelated comment to do with case normalization, but feel free to answer that elsewhere...

@jtibshirani
Copy link
Copy Markdown
Contributor Author

jtibshirani commented Nov 7, 2018

Thank you both for reviewing, I plan to fix the failing test and merge.

@jtibshirani
Make sure MappedFieldType#hasDocValues always returns false.
4e7765a 
This affects some of the checks in tests, but doesn't have an actual effect
on indexing (because there we rely on FieldType#docValuesType).
@jtibshirani jtibshirani merged commit e7613b7 into elastic:object-fields Nov 8, 2018
@jtibshirani jtibshirani deleted the json-field-tests branch November 8, 2018 00:42
jtibshirani added a commit to jtibshirani/elasticsearch that referenced this pull request Nov 8, 2018
@jtibshirani
Add tests for the supported query types. (elastic#35319)
4c46b07 
* Add tests for the supported query types.
* Disallow unbounded range queries on keyed JSON fields.
* Make sure MappedFieldType#hasDocValues always returns false.
jtibshirani added a commit to jtibshirani/elasticsearch that referenced this pull request Nov 9, 2018
@jtibshirani
Add tests for the supported query types. (elastic#35319)
b8a4ca1 
* Add tests for the supported query types.
* Disallow unbounded range queries on keyed JSON fields.
* Make sure MappedFieldType#hasDocValues always returns false.
jtibshirani added a commit that referenced this pull request Nov 30, 2018
@jtibshirani
Add tests for the supported query types. (#35319)
977025d 
* Add tests for the supported query types.
* Disallow unbounded range queries on keyed JSON fields.
* Make sure MappedFieldType#hasDocValues always returns false.
jtibshirani added a commit that referenced this pull request Dec 5, 2018
@jtibshirani
Add tests for the supported query types. (#35319)
cf2bcb6 
* Add tests for the supported query types.
* Disallow unbounded range queries on keyed JSON fields.
* Make sure MappedFieldType#hasDocValues always returns false.
jtibshirani added a commit that referenced this pull request Dec 13, 2018
@jtibshirani
Add tests for the supported query types. (#35319)
86c3761 
* Add tests for the supported query types.
* Disallow unbounded range queries on keyed JSON fields.
* Make sure MappedFieldType#hasDocValues always returns false.
jtibshirani added a commit that referenced this pull request Mar 6, 2019
@jtibshirani
Add tests for the supported query types. (#35319)
daf6c00 
* Add tests for the supported query types.
* Disallow unbounded range queries on keyed JSON fields.
* Make sure MappedFieldType#hasDocValues always returns false.
jtibshirani added a commit that referenced this pull request Mar 7, 2019
@jtibshirani
Add tests for the supported query types. (#35319)
7fefdac 
* Add tests for the supported query types.
* Disallow unbounded range queries on keyed JSON fields.
* Make sure MappedFieldType#hasDocValues always returns false.
jtibshirani added a commit that referenced this pull request Mar 9, 2019
@jtibshirani
Add tests for the supported query types. (#35319)
575f249 
* Add tests for the supported query types.
* Disallow unbounded range queries on keyed JSON fields.
* Make sure MappedFieldType#hasDocValues always returns false.
jtibshirani added a commit that referenced this pull request Mar 11, 2019
@jtibshirani
Add tests for the supported query types. (#35319)
1c8dab2 
* Add tests for the supported query types.
* Disallow unbounded range queries on keyed JSON fields.
* Make sure MappedFieldType#hasDocValues always returns false.
jtibshirani added a commit that referenced this pull request Mar 14, 2019
@jtibshirani
Add tests for the supported query types. (#35319)
2fd6cc9 
* Add tests for the supported query types.
* Disallow unbounded range queries on keyed JSON fields.
* Make sure MappedFieldType#hasDocValues always returns false.
jtibshirani added a commit that referenced this pull request Mar 19, 2019
@jtibshirani
Add tests for the supported query types. (#35319)
963d6c8 
* Add tests for the supported query types.
* Disallow unbounded range queries on keyed JSON fields.
* Make sure MappedFieldType#hasDocValues always returns false.
jtibshirani added a commit to jtibshirani/elasticsearch that referenced this pull request Mar 19, 2019
@jtibshirani
Add tests for the supported query types. (elastic#35319)
bc73a90 
* Add tests for the supported query types.
* Disallow unbounded range queries on keyed JSON fields.
* Make sure MappedFieldType#hasDocValues always returns false.
jtibshirani added a commit that referenced this pull request Mar 28, 2019
@jtibshirani
Add tests for the supported query types. (#35319)
727d50d 
* Add tests for the supported query types.
* Disallow unbounded range queries on keyed JSON fields.
* Make sure MappedFieldType#hasDocValues always returns false.
jtibshirani added a commit that referenced this pull request Apr 10, 2019
@jtibshirani
Add tests for the supported query types. (#35319)
fd73dcc 
* Add tests for the supported query types.
* Disallow unbounded range queries on keyed JSON fields.
* Make sure MappedFieldType#hasDocValues always returns false.
jtibshirani added a commit that referenced this pull request Apr 17, 2019
@jtibshirani
Add tests for the supported query types. (#35319)
f4bcf16 
* Add tests for the supported query types.
* Disallow unbounded range queries on keyed JSON fields.
* Make sure MappedFieldType#hasDocValues always returns false.
jtibshirani added a commit that referenced this pull request May 1, 2019
@jtibshirani
Add tests for the supported query types. (#35319)
93f7e31 
* Add tests for the supported query types.
* Disallow unbounded range queries on keyed JSON fields.
* Make sure MappedFieldType#hasDocValues always returns false.
jtibshirani added a commit that referenced this pull request May 24, 2019
@jtibshirani
Add tests for the supported query types. (#35319)
e957be0 
* Add tests for the supported query types.
* Disallow unbounded range queries on keyed JSON fields.
* Make sure MappedFieldType#hasDocValues always returns false.
jtibshirani added a commit that referenced this pull request May 29, 2019
@jtibshirani
Add tests for the supported query types. (#35319)
4507b85 
* Add tests for the supported query types.
* Disallow unbounded range queries on keyed JSON fields.
* Make sure MappedFieldType#hasDocValues always returns false.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@romseygeek romseygeek romseygeek approved these changes

+1 more reviewer

@colings86 colings86 colings86 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

:Search Foundations/Mapping Index mappings, including merging and defining field types >test Issues or PRs that are addressing/adding tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jtibshirani @elasticmachine @colings86 @romseygeek