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

Sort Processor does not have proper behavior with targetField #25237

Merged
merged 1 commit into from Jun 15, 2017

Conversation

Projects
None yet
5 participants
@talevy
Copy link
Contributor

commented Jun 15, 2017

#24133 added the ability to specify a targetField in SortProcessor. This results in some interesting behavior that was missed in the review.
This processor sorts in-place, so there is a side-effect in both the original field and the target field.
Another bug was that the targetField was not being set if the list being sorted was fewer than two elements.

The new behavior works like this: If targetField and fieldName are not the same, we copy the list.

to reproduce original test failure:

gradle :modules:ingest-common:test -Dtests.seed=812BA08DE627020C -Dtests.class=org.elasticsearch.ingest.common.SortProcessorTests -Dtests.method="testSortWithTargetField" -Dtests.security.manager=true -Dtests.jvm.argline="-XX:+AggressiveOpts" -Dtests.locale=bg -Dtests.timezone=America/Pangnirtung
@rjernst

This comment has been minimized.

Copy link
Member

commented Jun 15, 2017

Can you just always copy the list? Modifying a list from somewhere else is sneaky. Also, should be no need for the size check.

@talevy

This comment has been minimized.

Copy link
Contributor Author

commented Jun 15, 2017

@rjernst guess I was just preserving that "optimizing" logic, but yeah, super minimal. I'll simplify

@talevy talevy force-pushed the talevy:fix-24133 branch Jun 15, 2017

@talevy

This comment has been minimized.

Copy link
Contributor Author

commented Jun 15, 2017

updated. thanks for looking @rjernst

modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/SortProcessorTests.java Outdated
String targetFieldName;
do {
targetFieldName = RandomDocumentPicks.randomFieldName(random());
} while (fieldName.equals(targetFieldName));

This comment has been minimized.

Copy link
@rjernst

rjernst Jun 15, 2017

Member

We really should not have random logic like this anywhere in ES. Do we really need random field names to test whether sorting works? I would rather see explicit fields names "field1" and "field2" and actually test both sort orders than always have these random field names. They have their place, but for things where the field name matters.

This comment has been minimized.

Copy link
@talevy

talevy Jun 15, 2017

Author Contributor

nope

@talevy talevy force-pushed the talevy:fix-24133 branch Jun 15, 2017

@rjernst
Copy link
Member

left a comment

Thanks @talevy. Looks good. I would still have an explicit test for both sort orders, so as not to rely on randomization since it is easy to cover these 2 cases.

@davidkyle

This comment has been minimized.

Copy link
Member

commented Jun 15, 2017

There was another occurrence of this failure on 5.x

https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+5.x+multijob-darwin-compatibility/572/console

gradle :modules:ingest-common:test -Dtests.seed=5FE7727E3E91839F -Dtests.class=org.elasticsearch.ingest.common.SortProcessorTests -Dtests.method="testSortWithTargetField" -Dtests.security.manager=true -Dtests.locale=fi-FI -Dtests.timezone=Asia/Omsk

And master:
https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+master+multijob-intake/1424/console

gradle :modules:ingest-common:test -Dtests.seed=14E3C30AB5E521B6 -Dtests.class=org.elasticsearch.ingest.common.SortProcessorTests -Dtests.method="testSortWithTargetField" -Dtests.security.manager=true -Dtests.locale=tr-TR -Dtests.timezone=Asia/Damascus

Both reproduced for me

@martijnvg
Copy link
Member

left a comment

LGTM, +1 to Ryan's test suggestion.

fix: Sort Processor does not have proper behavior with targetField
to specify a `targetField`. This results in some interesting behavior that was missed in the review.
This processor sorts in-place, so there is a side-effect in both the original field and the target field.
Another bug was that the targetField was not being set if the list being sorted was fewer than two elements.

The new behavior works like this: If targetField and fieldName are not the same, we copy the list.

@talevy talevy force-pushed the talevy:fix-24133 branch to 393279a Jun 15, 2017

@talevy talevy merged commit 2cd771a into elastic:master Jun 15, 2017

1 of 2 checks passed

elasticsearch-ci Build triggered. sha1 is merged.
Details
CLA Commit author is a member of Elasticsearch
Details

@talevy talevy deleted the talevy:fix-24133 branch Jun 15, 2017

talevy added a commit that referenced this pull request Jun 15, 2017

fix: Sort Processor does not have proper behavior with targetField (#…
…25237)

to specify a `targetField`. This results in some interesting behavior that was missed in the review.
This processor sorts in-place, so there is a side-effect in both the original field and the target field.
Another bug was that the targetField was not being set if the list being sorted was fewer than two elements.

The new behavior works like this: If targetField and fieldName are not the same, we copy the list.

jasontedor added a commit to glefloch/elasticsearch that referenced this pull request Jun 15, 2017

Merge branch 'master' into fix/24581
* master:
  [Test] restore BWC for parent-join now that the new mapping format is in 5.x
  Add a section named "relations" in the ParentJoinFieldMapper (elastic#25248)
  test: Ported more OldIndexBackwardsCompatibilityIT tests to full cluster restart qa tests. (elastic#25173)
  fix: Sort Processor does not have proper behavior with targetField (elastic#25237)
  Allow reader wrappers to have different live docs but the same cache key.

jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jun 16, 2017

Merge branch 'master' of github.com:elastic/elasticsearch into primar…
…y-context

* 'master' of github.com:elastic/elasticsearch: (21 commits)
  [DOCS] Clarify expected availability of HDFS for the HDFS Repository (elastic#25220)
  Remove some redundant 140 character checkstyle suppressions
  [Docs] more fix for the parent-join docs
  [Docs] Fix cross reference for parent-join field
  More advices around search speed and disk usage. (elastic#25252)
  Add documentation for the new parent-join field (elastic#25227)
  [analysis-icu] Allow setting unicodeSetFilter (elastic#20814)
  Introduce translog size and age based retention policies (elastic#25147)
  Add needs methods for specific variables to Painless script context factories. (elastic#25267)
  Improves snapshot logging and snapshoth deletion error handling (elastic#25264)
  Add unit test for PathHierarchyTokenizerFactory (elastic#24984)
  Deprecate tribe service
  Moved more token filters to analysis-common module.
  [Test] Make sure that SearchAfterSortedDocQueryTests uses a single threaded searcher
  [DOCS] Defined es-test-dir and plugins-examples-dir in index.asciidoc.  (elastic#25232)
  Test fix - removed superfluous assertion (elastic#25247)
  [Test] restore BWC for parent-join now that the new mapping format is in 5.x
  Add a section named "relations" in the ParentJoinFieldMapper (elastic#25248)
  test: Ported more OldIndexBackwardsCompatibilityIT tests to full cluster restart qa tests. (elastic#25173)
  fix: Sort Processor does not have proper behavior with targetField (elastic#25237)
  ...

@clintongormley clintongormley added v6.0.0-beta1 and removed v6.0.0 labels Jul 25, 2017

@clintongormley clintongormley changed the title fix: Sort Processor does not have proper behavior with targetField Sort Processor does not have proper behavior with targetField Jul 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.