[ML] Prevent fetching multi-field from source #48770
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Aggregatable mutli-fields are at the moment wrongly mapped as normal doc_value fields and thus they support fetching from source. However, they do not exist in the source. This results to failure to extract such fields. This commit fixes this bug. While a fix could be worked out on top of the existing code, it is evident the extraction logic has become difficult to understand and maintain. As we also want to deduplicate multi-fields for data frame analytics, it seemed appropriate to refactor the code to simplify and better handle the extraction of multi-fields. Relates elastic#48756
|
Pinging @elastic/ml-core (:ml) |
| @@ -283,10 +283,12 @@ public void testLookbackOnlyWithSourceDisabled() throws Exception { | |||
| new LookbackOnlyTestHelper("test-lookback-only-with-source-disabled", "airline-data-disabled-source").execute(); | |||
| } | |||
|
|
|||
| @AwaitsFix(bugUrl = "This test uses painless which is not available in the integTest phase") | |||
There was a problem hiding this comment.
It seems that since this test was written the test's environment is different and we have access to painless now. I re-enabled the test.
|
run elasticsearch-ci/1 (unrelated failure in SnapshotIT) |
benwtrent
approved these changes
Oct 31, 2019
| private final T falseValue; | ||
|
|
||
| BooleanMapper(ExtractedField field, T trueValue, T falseValue) { | ||
| super(field.getName(), Collections.singleton(BooleanFieldMapper.CONTENT_TYPE)); |
There was a problem hiding this comment.
seems like Collections.singleton(BooleanFieldMapper.CONTENT_TYPE) could be a private static final field.
| static final String TYPE = "geo_point"; | ||
|
|
||
| public GeoPointField(String name) { | ||
| super(name, Collections.singleton(TYPE)); |
There was a problem hiding this comment.
#nit
this Collections.singleton(TYPE) and the others that are simply passed up to super could be private static finalso that a new HashSet isn't built on every ctor call
|
run elasticsearch-ci/2 (unrelated failure) |
|
run elasticsearch-ci/default-distro |
1 similar comment
|
run elasticsearch-ci/default-distro |
|
@elasticmachine update branch |
dimitris-athanasiou
deleted the
prevent-fetching-multi-field-from-source
branch
dimitris-athanasiou
added a commit
to dimitris-athanasiou/elasticsearch
that referenced
this pull request
Nov 1, 2019
Aggregatable mutli-fields are at the moment wrongly mapped as normal doc_value fields and thus they support fetching from source. However, they do not exist in the source. This results to failure to extract such fields. This commit fixes this bug. While a fix could be worked out on top of the existing code, it is evident the extraction logic has become difficult to understand and maintain. As we also want to deduplicate multi-fields for data frame analytics, it seemed appropriate to refactor the code to simplify and better handle the extraction of multi-fields. Relates elastic#48756 Backport of elastic#48770
dimitris-athanasiou
added a commit
to dimitris-athanasiou/elasticsearch
that referenced
this pull request
Nov 1, 2019
Aggregatable mutli-fields are at the moment wrongly mapped as normal doc_value fields and thus they support fetching from source. However, they do not exist in the source. This results to failure to extract such fields. This commit fixes this bug. While a fix could be worked out on top of the existing code, it is evident the extraction logic has become difficult to understand and maintain. As we also want to deduplicate multi-fields for data frame analytics, it seemed appropriate to refactor the code to simplify and better handle the extraction of multi-fields. Relates elastic#48756 Backport of elastic#48770
dimitris-athanasiou
added a commit
to dimitris-athanasiou/elasticsearch
that referenced
this pull request
Nov 1, 2019
In the case multi-fields exist in the source index, we pick all variants of them in our extracted fields detection for data frame analytics. This means we may have multiple instances of the same feature. The worse consequence of this is when the dependent variable (for regression or classification) is also duplicated which means we train a model on the dependent variable itself. Now that elastic#48770 is merged, this commit is adding logic to only select one variant of multi-fields. Closes elastic#48756
dimitris-athanasiou
added a commit
that referenced
this pull request
Nov 1, 2019
In the case multi-fields exist in the source index, we pick all variants of them in our extracted fields detection for data frame analytics. This means we may have multiple instances of the same feature. The worse consequence of this is when the dependent variable (for regression or classification) is also duplicated which means we train a model on the dependent variable itself. Now that #48770 is merged, this commit is adding logic to only select one variant of multi-fields. Closes #48756
dimitris-athanasiou
added a commit
that referenced
this pull request
Nov 1, 2019
Aggregatable mutli-fields are at the moment wrongly mapped as normal doc_value fields and thus they support fetching from source. However, they do not exist in the source. This results to failure to extract such fields. This commit fixes this bug. While a fix could be worked out on top of the existing code, it is evident the extraction logic has become difficult to understand and maintain. As we also want to deduplicate multi-fields for data frame analytics, it seemed appropriate to refactor the code to simplify and better handle the extraction of multi-fields. Relates #48756 Backport of #48770
dimitris-athanasiou
added a commit
that referenced
this pull request
Nov 1, 2019
Aggregatable mutli-fields are at the moment wrongly mapped as normal doc_value fields and thus they support fetching from source. However, they do not exist in the source. This results to failure to extract such fields. This commit fixes this bug. While a fix could be worked out on top of the existing code, it is evident the extraction logic has become difficult to understand and maintain. As we also want to deduplicate multi-fields for data frame analytics, it seemed appropriate to refactor the code to simplify and better handle the extraction of multi-fields. Relates #48756 Backport of #48770
dimitris-athanasiou
added a commit
to dimitris-athanasiou/elasticsearch
that referenced
this pull request
Nov 1, 2019
…48799) In the case multi-fields exist in the source index, we pick all variants of them in our extracted fields detection for data frame analytics. This means we may have multiple instances of the same feature. The worse consequence of this is when the dependent variable (for regression or classification) is also duplicated which means we train a model on the dependent variable itself. Now that elastic#48770 is merged, this commit is adding logic to only select one variant of multi-fields. Closes elastic#48756 Backport of elastic#48799
dimitris-athanasiou
added a commit
to dimitris-athanasiou/elasticsearch
that referenced
this pull request
Nov 1, 2019
…48799) In the case multi-fields exist in the source index, we pick all variants of them in our extracted fields detection for data frame analytics. This means we may have multiple instances of the same feature. The worse consequence of this is when the dependent variable (for regression or classification) is also duplicated which means we train a model on the dependent variable itself. Now that elastic#48770 is merged, this commit is adding logic to only select one variant of multi-fields. Closes elastic#48756 Backport elastic#48799
dimitris-athanasiou
added a commit
that referenced
this pull request
Nov 1, 2019
…48806) In the case multi-fields exist in the source index, we pick all variants of them in our extracted fields detection for data frame analytics. This means we may have multiple instances of the same feature. The worse consequence of this is when the dependent variable (for regression or classification) is also duplicated which means we train a model on the dependent variable itself. Now that #48770 is merged, this commit is adding logic to only select one variant of multi-fields. Closes #48756 Backport of #48799
dimitris-athanasiou
added a commit
that referenced
this pull request
Nov 1, 2019
…48807) In the case multi-fields exist in the source index, we pick all variants of them in our extracted fields detection for data frame analytics. This means we may have multiple instances of the same feature. The worse consequence of this is when the dependent variable (for regression or classification) is also duplicated which means we train a model on the dependent variable itself. Now that #48770 is merged, this commit is adding logic to only select one variant of multi-fields. Closes #48756 Backport #48799
This was referenced Feb 3, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Aggregatable mutli-fields are at the moment wrongly mapped
as normal doc_value fields and thus they support fetching from
source. However, they do not exist in the source. This results
to failure to extract such fields.
This commit fixes this bug. While a fix could be worked out
on top of the existing code, it is evident the extraction logic
has become difficult to understand and maintain. As we also
want to deduplicate multi-fields for data frame analytics,
it seemed appropriate to refactor the code to simplify and
better handle the extraction of multi-fields.
Relates #48756