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.

Sign up for GitHub

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

Jump to bottom

Construct the significant terms aggregation builder's parser statically #25519

Closed
nik9000 opened this issue Jul 3, 2017 · 3 comments · Fixed by #50623
Closed

Construct the significant terms aggregation builder's parser statically #25519

nik9000 opened this issue Jul 3, 2017 · 3 comments · Fixed by #50623
Assignees
@nik9000
Labels
:Analytics/Aggregations Aggregations >non-issue

Comments

@nik9000
Copy link
Member

nik9000 commented Jul 3, 2017

Right now we build a new ObjectParser every time we want get a significant terms aggregation. We do this because we had to pass in ParseFieldRegistry<SignificanceHeuristicParser>. I think we can avoid this by moving ParseFieldRegistry<SignificanceHeuristicParser> to NamedXContentRegistry style parsing.
@nik9000 nik9000 added :Analytics/Aggregations Aggregations help wanted adoptme good first issue low hanging fruit labels Jul 3, 2017
@nik9000
Copy link
Member Author

nik9000 commented Jul 3, 2017

Marked a low hanging fruit because it should be fairly simple to do but big enough that it certainly deserves its own PR.

@nik9000 nik9000 mentioned this issue Jul 3, 2017
Merged
@cbuescher
Copy link
Member

Same is true for SignificantTextAggregationBuilder which follows the same pattern. I took a quick first look at it, changing the way the heuristics are registered is only part of this. I quickly got stuck when trying to convert the part around

elasticsearch/core/src/main/java/org/elasticsearch/search/aggregations/bucket/significant/SignificantTermsAggregationBuilder.java

Line 98 in f576c98

new ParseField(name));
because currently each potential heuristic is declared on the ObjectParser using its name. I think we might need something like what I started in #24020 to be able to do this with ObjectParser, most other places we parse named things like this we still do it in the "classic" manual parsing style. Just adding this note here since this issue might be a little more involved than estimated.

@nik9000 nik9000 removed the good first issue low hanging fruit label Jul 3, 2017
@colings86
Copy link
Contributor

@elastic/es-search-aggs

@not-napoleon not-napoleon mentioned this issue Dec 3, 2019
Closed
@nik9000 nik9000 self-assigned this Dec 23, 2019
@nik9000 nik9000 removed the help wanted adoptme label Dec 23, 2019
nik9000 added a commit to nik9000/elasticsearch that referenced this issue Jan 2, 2020
@nik9000
x-content: Support collapsed named objects
c94a888 
This adds support for "collapsed" named object to `ObjectParser`. In
particular, this supports the sort of xcontent that we use to specify
significance heuristics. See elastic#25519 and this example:

```
GET /_search
{
    "query" : {
        "terms" : {"force" : [ "British Transport Police" ]}
    },
    "aggregations" : {
        "significant_crime_types" : {
            "significant_terms" : {
                "field" : "crime_type",
                "mutual_information" : { <<------- This is the name
                    "include_negatives": true
                }
            }
        }
    }
}
```

I believe there are a couple of things that work this way.

I've held off on moving the actual parsing of the significant heuristics
to this code to kep the review more compact. The moving is pretty
mechanical stuff in the aggs framework.
@nik9000 nik9000 mentioned this issue Jan 2, 2020
Merged
nik9000 added a commit that referenced this issue Jan 3, 2020
@nik9000
x-content: Support collapsed named objects (#50564)
4ccbd3e 
This adds support for "collapsed" named object to `ObjectParser`. In
particular, this supports the sort of xcontent that we use to specify
significance heuristics. See #25519 and this example:

```
GET /_search
{
    "query" : {
        "terms" : {"force" : [ "British Transport Police" ]}
    },
    "aggregations" : {
        "significant_crime_types" : {
            "significant_terms" : {
                "field" : "crime_type",
                "mutual_information" : { <<------- This is the name
                    "include_negatives": true
                }
            }
        }
    }
}
```

I believe there are a couple of things that work this way.

I've held off on moving the actual parsing of the significant heuristics
to this code to keep the review more compact. The moving is pretty
mechanical stuff in the aggs framework.
nik9000 added a commit to nik9000/elasticsearch that referenced this issue Jan 3, 2020
@nik9000
x-content: Support collapsed named objects (elastic#50564)
213e502 
This adds support for "collapsed" named object to `ObjectParser`. In
particular, this supports the sort of xcontent that we use to specify
significance heuristics. See elastic#25519 and this example:

```
GET /_search
{
    "query" : {
        "terms" : {"force" : [ "British Transport Police" ]}
    },
    "aggregations" : {
        "significant_crime_types" : {
            "significant_terms" : {
                "field" : "crime_type",
                "mutual_information" : { <<------- This is the name
                    "include_negatives": true
                }
            }
        }
    }
}
```

I believe there are a couple of things that work this way.

I've held off on moving the actual parsing of the significant heuristics
to this code to keep the review more compact. The moving is pretty
mechanical stuff in the aggs framework.
@nik9000 nik9000 mentioned this issue Jan 3, 2020
Merged
nik9000 added a commit to nik9000/elasticsearch that referenced this issue Jan 3, 2020
@nik9000
Replace bespoke parser for significance heuristics
806eb61 
This replaces the hand written xcontent parsers for significance
heristics with `ObjectParser` and parsing named xcontent.

As a happy accident, this was the last user of `ParseFieldRegistry` so
this PR entirely removes that class.

Closes elastic#25519
@nik9000 nik9000 mentioned this issue Jan 3, 2020
Merged
nik9000 added a commit that referenced this issue Jan 3, 2020
@nik9000
x-content: Support collapsed named objects (#50564) (#50619)
a45de8a 
This adds support for "collapsed" named object to `ObjectParser`. In
particular, this supports the sort of xcontent that we use to specify
significance heuristics. See #25519 and this example:

```
GET /_search
{
    "query" : {
        "terms" : {"force" : [ "British Transport Police" ]}
    },
    "aggregations" : {
        "significant_crime_types" : {
            "significant_terms" : {
                "field" : "crime_type",
                "mutual_information" : { <<------- This is the name
                    "include_negatives": true
                }
            }
        }
    }
}
```

I believe there are a couple of things that work this way.

I've held off on moving the actual parsing of the significant heuristics
to this code to keep the review more compact. The moving is pretty
mechanical stuff in the aggs framework.
nik9000 added a commit that referenced this issue Jan 6, 2020
@nik9000
Replace bespoke parser for significance heuristics (#50623)
92503ca 
This replaces the hand written xcontent parsers for significance
heristics with `ObjectParser` and parsing named xcontent.

As a happy accident, this was the last user of `ParseFieldRegistry` so
this PR entirely removes that class.

Closes #25519
nik9000 added a commit to nik9000/elasticsearch that referenced this issue Jan 6, 2020
@nik9000
Replace bespoke parser for significance heuristics (elastic#50623)
f721121 
This replaces the hand written xcontent parsers for significance
heristics with `ObjectParser` and parsing named xcontent.

As a happy accident, this was the last user of `ParseFieldRegistry` so
this PR entirely removes that class.

Closes elastic#25519
@nik9000 nik9000 mentioned this issue Jan 6, 2020
Merged
nik9000 added a commit that referenced this issue Jan 6, 2020
@nik9000
Replace bespoke parser for significance heuristics (#50623) (#50659)
f576aef 
This replaces the hand written xcontent parsers for significance
heristics with `ObjectParser` and parsing named xcontent.

As a happy accident, this was the last user of `ParseFieldRegistry` so
this PR entirely removes that class.

Closes #25519
SivagurunathanV pushed a commit to SivagurunathanV/elasticsearch that referenced this issue Jan 23, 2020
@nik9000 @SivagurunathanV
x-content: Support collapsed named objects (elastic#50564)
bafbc23 
This adds support for "collapsed" named object to `ObjectParser`. In
particular, this supports the sort of xcontent that we use to specify
significance heuristics. See elastic#25519 and this example:

```
GET /_search
{
    "query" : {
        "terms" : {"force" : [ "British Transport Police" ]}
    },
    "aggregations" : {
        "significant_crime_types" : {
            "significant_terms" : {
                "field" : "crime_type",
                "mutual_information" : { <<------- This is the name
                    "include_negatives": true
                }
            }
        }
    }
}
```

I believe there are a couple of things that work this way.

I've held off on moving the actual parsing of the significant heuristics
to this code to keep the review more compact. The moving is pretty
mechanical stuff in the aggs framework.
SivagurunathanV pushed a commit to SivagurunathanV/elasticsearch that referenced this issue Jan 23, 2020
@nik9000 @SivagurunathanV
Replace bespoke parser for significance heuristics (elastic#50623)
0158b15 
This replaces the hand written xcontent parsers for significance
heristics with `ObjectParser` and parsing named xcontent.

As a happy accident, this was the last user of `ParseFieldRegistry` so
this PR entirely removes that class.

Closes elastic#25519
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@nik9000 nik9000

Labels
:Analytics/Aggregations Aggregations >non-issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Replace bespoke parser for significance heuristics nik9000/elasticsearch
3 participants
@nik9000 @colings86 @cbuescher