fix MultiValuesSourceFieldConfig toXContent #36525

talevy · 2018-12-12T02:05:22Z

This commit turns MultiValuesSourceFieldConfig into a proper
ToXContentObject for easy testing and verification of its
to/from XContent methods.

Closes #36474.

elasticmachine · 2018-12-12T02:05:23Z

Pinging @elastic/es-analytics-geo

talevy · 2018-12-12T02:13:32Z

@polyfractal I was interested in adding actual tests for the aggregation as well since I couldn't find any. Do we have to/from xcontent tests somewhere for the weighted_avg aggregation?

I tried my hand, but
ran into issues where it isn't clear how to parse the toXContent version of things. Looks like things are inconsistent?

For example, here is a snippet of the generated xcontent:

{“vOdlQXAnYr”:{“weighted_avg”:{“fields”:{“weight”:{“field”:“weight_field”},“value”:{“field”:“value_field”}}}}}

There is an extra fields field that differs from the vanilla query-dsl format.

talevy · 2018-12-12T02:23:55Z

I assume I am missing some context, but I went ahead and added a second commit that demonstrates what I meant by my previous comment

polyfractal

Thanks @talevy! Left a few comments, otherwise LGTM.

Should this target master, not just 6.x?

.../java/org/elasticsearch/search/aggregations/support/MultiValuesSourceAggregationBuilder.java

polyfractal · 2018-12-12T17:35:56Z

...asticsearch/search/aggregations/metrics/weighted_avg/WeightedAvgAggregationBuilderTests.java

+
+    @Override
+    protected WeightedAvgAggregationBuilder doParseInstance(XContentParser parser) throws IOException {
+        parser.nextToken();


Should we snag the code from BaseAggregatorTestCase#parse() so that we have a similar set of assertions here?

oh woah, yeah, I'll lift that...

thanks for the pointer, was looking for something like that to re-use

polyfractal · 2018-12-12T17:43:02Z

...rc/main/java/org/elasticsearch/search/aggregations/support/MultiValuesSourceFieldConfig.java

@@ -78,7 +79,7 @@ private MultiValuesSourceFieldConfig(String fieldName, Object missing, Script sc
    }

    public MultiValuesSourceFieldConfig(StreamInput in) throws IOException {
-        this.fieldName = in.readString();
+        this.fieldName = in.readOptionalString();


I think that if we want to change this, we might need to wrap this in version checks? readOptionalString() encodes an extra byte for the boolean

good catch. yes.

OK. maybe we can do this in a separate PR. I think Docs need clean up too. We claim that field is required, but it can be replaced with script.

I see this as a bug, but want to attack this separately since the original issue was only with XContent.

++ makes sense to me

This commit turns MultiValuesSourceFieldConfig into a proper ToXContentObject for easy testing and verification of its to/from XContent methods. Closes elastic#36474.

talevy · 2018-12-13T02:32:49Z

Hey @polyfractal, I know you approved this, but I've reverted the serialization changes, mind taking another look?

As I said previously. the bug in the serialization is not the concern of this PR.

polyfractal

👍 looks good! ++ to doing the serialization stuff in a followup PR (agree it's a bug and the docs should be updated to match)

talevy · 2018-12-13T16:19:56Z

thanks for the review @polyfractal!

This commit turns MultiValuesSourceFieldConfig into a proper ToXContentObject for easy testing and verification of its to/from XContent methods. Closes #36474.

* elastic/master: Remove deprecated `useDisMax` from MultiMatchQuery (elastic#36488) HLRC: Add get users action (elastic#36332) fix MultiValuesSourceFieldConfig toXContent (elastic#36525) Add ILM-specific security privileges (elastic#36493) Remove usages of `MockTcpTransport` from zen tests (elastic#36579)

RussellSpitzer · 2019-04-05T21:37:08Z

This is also broken on the 6.4 branch, would it be possible to back port the fix there as well?

talevy added >bug :Analytics/Aggregations Aggregations labels Dec 12, 2018

talevy requested a review from polyfractal December 12, 2018 02:05

talevy force-pushed the fix-36474 branch from aa639fa to 2cfe5e2 Compare December 12, 2018 02:08

$polyfractal$

polyfractal approved these changes Dec 12, 2018

View reviewed changes

talevy added 3 commits December 12, 2018 17:30

fix MultiValuesSourceFieldConfig toXContent

449d4bd

This commit turns MultiValuesSourceFieldConfig into a proper ToXContentObject for easy testing and verification of its to/from XContent methods. Closes elastic#36474.

fix extranious

da1072e

clean up

d8ef49b

talevy force-pushed the fix-36474 branch from 0b218fb to d8ef49b Compare December 13, 2018 02:19

talevy changed the base branch from 6.x to master December 13, 2018 02:20

talevy requested a review from polyfractal December 13, 2018 05:52

$polyfractal$

polyfractal approved these changes Dec 13, 2018

View reviewed changes

talevy merged commit b820d7c into elastic:master Dec 13, 2018

talevy deleted the fix-36474 branch December 13, 2018 16:17

talevy added v7.0.0 v6.6.0 labels Dec 13, 2018

talevy added a commit that referenced this pull request Dec 13, 2018

fix MultiValuesSourceFieldConfig toXContent (#36525)

6539c22

This commit turns MultiValuesSourceFieldConfig into a proper ToXContentObject for easy testing and verification of its to/from XContent methods. Closes #36474.

talevy added a commit that referenced this pull request Dec 13, 2018

fix MultiValuesSourceFieldConfig toXContent (#36525)

40f458b

This commit turns MultiValuesSourceFieldConfig into a proper ToXContentObject for easy testing and verification of its to/from XContent methods. Closes #36474.

talevy added the v6.5.4 label Dec 13, 2018

$@polyfractal$ polyfractal mentioned this pull request Dec 19, 2018

Java High Level Rest Client cannot parse weighted_avg result #36861

Closed

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix MultiValuesSourceFieldConfig toXContent #36525

fix MultiValuesSourceFieldConfig toXContent #36525

talevy commented Dec 12, 2018

elasticmachine commented Dec 12, 2018

talevy commented Dec 12, 2018 •

edited

talevy commented Dec 12, 2018

$@polyfractal$ polyfractal left a comment

$@polyfractal$ polyfractal Dec 12, 2018

talevy Dec 12, 2018

$@polyfractal$ polyfractal Dec 12, 2018

talevy Dec 12, 2018

talevy Dec 13, 2018

$@polyfractal$ polyfractal Dec 13, 2018

talevy commented Dec 13, 2018 •

edited

$@polyfractal$ polyfractal left a comment

talevy commented Dec 13, 2018

RussellSpitzer commented Apr 5, 2019

fix MultiValuesSourceFieldConfig toXContent #36525

fix MultiValuesSourceFieldConfig toXContent #36525

Conversation

talevy commented Dec 12, 2018

elasticmachine commented Dec 12, 2018

talevy commented Dec 12, 2018 • edited

talevy commented Dec 12, 2018

polyfractal left a comment

Choose a reason for hiding this comment

polyfractal Dec 12, 2018

Choose a reason for hiding this comment

talevy Dec 12, 2018

Choose a reason for hiding this comment

polyfractal Dec 12, 2018

Choose a reason for hiding this comment

talevy Dec 12, 2018

Choose a reason for hiding this comment

talevy Dec 13, 2018

Choose a reason for hiding this comment

polyfractal Dec 13, 2018

Choose a reason for hiding this comment

talevy commented Dec 13, 2018 • edited

polyfractal left a comment

Choose a reason for hiding this comment

talevy commented Dec 13, 2018

RussellSpitzer commented Apr 5, 2019

talevy commented Dec 12, 2018 •

edited

$@polyfractal$ polyfractal left a comment

$@polyfractal$ polyfractal Dec 12, 2018

$@polyfractal$ polyfractal Dec 12, 2018

$@polyfractal$ polyfractal Dec 13, 2018

talevy commented Dec 13, 2018 •

edited

$@polyfractal$ polyfractal left a comment