make SignificantTermsBucket generic on key type #3795
Conversation
kevinbuhmann
force-pushed
the
sig-term-agg-generic-key
branch
2 times, most recently
from
d114ce7 to
399747e
Compare
There was a problem hiding this comment.
Awesome awesome PR thanks for raising it @kevinphelps
Left a couple of nitpicks.
Backporting to 6.x might be a tad harder as SignificantTermsAggregate SignificantTermsBucket are now generic, we could look into keeping both the non generic and generic versions of those in that branch though
| Meta = bucket.Meta | ||
| }; | ||
| } | ||
|
|
||
| public SignificantTermsAggregate SignificantText(string key) | ||
| public SignificantTermsAggregate<TKey> SignificantText<TKey>(string key) |
There was a problem hiding this comment.
I think having a non generic overload defaulting to TKey as string would be a nicer experience for the most common case.
There was a problem hiding this comment.
I can add that.
| { | ||
| Key = (string)key, |
There was a problem hiding this comment.
key is known to be only string or double from where its called:
private IBucket GetKeyedBucket(ref JsonReader reader, IJsonFormatterResolver formatterResolver)perhaps we can use this to our advantage and instantiate either a SignificantTermBucket<string> or SignificantTermBucket<double> and then use this type information in
private IEnumerable<SignificantTermsBucket<TKey>> GetSignificantTermsBuckets<TKey>(IEnumerable<IBucket> items) To return the items (casted) directly instead.
There was a problem hiding this comment.
You could do the same thing in GetKeyedBucket. Should that one be refactored as well?
Thanks! I'll try to make the requested changes tomorrow! :)
That sounds like it could work, but if not it's alright. I've already forked 6.x internally, so it's no big deal for me at least. |
kevinbuhmann
force-pushed
the
sig-term-agg-generic-key
branch
from
399747e to
fc59906
Compare
Supersedes: #3795 Key of SignificantTermsBucket is generic to allow for returning both string and numeric key values. Co-authored-by: kevinphelps <KevinPhelps11@gmail.com>
|
@kevinphelps I'm going to close this PR in favour of #3890, as we're pulling it into 7.x and retargeting introduces a few merge conflicts to resolve. I've added you as co-author 👍 |
Supersedes: #3795 Key of SignificantTermsBucket is generic to allow for returning both string and numeric key values. Co-authored-by: kevinphelps <KevinPhelps11@gmail.com>
The problem is similar to #2336 except with
SignificantTermsBucketinstead ofKeyedBucket. My use case is significant terms aggregation over numeric terms. ButSignificantTermsBucket.Keyis always null due to theKey = (string)keyline inAggregateFormatter.This solution is modeled after the solution for
KeyedBucketin 91f1f81.I have based this PR on
masterbut if this could also be targeted for 6.x, I would be very appreciative. I can open a separate PR targeting the 6.x branch if necessary.Let me know if there are any changes needed. I would be happy to comply.