fix(schema): model metric aggregation results under aggregations - #25
Open
gabooh wants to merge 1 commit into
Open
fix(schema): model metric aggregation results under aggregations#25gabooh wants to merge 1 commit into
aggregations#25gabooh wants to merge 1 commit into
Conversation
`aggBucketsResult` only declares `buckets` and `after_key`, but half of the
aggregations the schema can request never answer with buckets at all. Verified
against Manticore Search 29.0.2:
terms, histogram, date_histogram, range, date_range, composite
-> {"buckets": [...]}, plus {"after_key": {...}} for composite
min, max, sum, avg
-> {"value": 30.5}
median_absolute_deviation
-> {"value": 10, "value_as_string": "10"}
percentiles, percentile_ranks
-> {"values": [{"key": 25, "value": 18, "value_as_string": "18"}, ...]}
-> {"values": {"25": 18, "50": 30.5}} when `keyed` is true
The request side of these was modelled in 10.1.1 (aggPercentiles,
aggPercentileRanks, aggMedianAbsoluteDeviation, aggMetric); the response side
was not, so the two halves no longer meet.
For the Java client this is fatal rather than lossy. Its ObjectMapper is built
with FAIL_ON_UNKNOWN_PROPERTIES enabled, so a metric result raises
UnrecognizedPropertyException: Unrecognized field "values"
(class com.manticoresearch.client.model.AggBucketsResult),
not marked as ignorable (2 known properties: "buckets", "after_key")
and the whole searchResponse is lost with it — not just the aggregation. Five
of the six metric aggregations above fail this way on 10.2.0; only the bucket
ones deserialize. Any faceted search that also asks for percentiles therefore
cannot be read back by the Java client, on any server version.
Add `value`, `value_as_string` and `values` to `aggBucketsResult`, and say in
the description which aggregation returns which.
`values` is left untyped for the same reason `buckets` already is: its shape
follows the request's `keyed` flag — an array of `{key, value,
value_as_string}` objects when false, a map of requested point to value when
true.
`value` is left untyped as well, deliberately, because its JSON type follows
the aggregation. An integer aggregation returns an exact integer that can leave
the range a double holds exactly — measured on 29.0.2, `sum` over three int64
rows answers 18014398526259203, and `max` answers 9007199254740993 — while
`avg` answers 20.500000. `type: number` would be worse than imprecise in some
clients: this generator maps an unformatted number to a 32-bit float, so the Go
client would round every metric above 16777216 (`aggTDigest.compression` is
`*float32` in the current out/manticoresearch-go for exactly that reason). The
new test pins the exact-integer case so the decision cannot be undone by
accident.
`searchResponse.aggregations` carried its own copy of the pre-fix description
and a buckets-only example; both now cover metric results too.
Existing accessors are untouched and no schema is renamed, so this is additive.
It does add fields to generated response models, which in Rust means a struct
literal that names every field stops compiling — the same cost the previous
release carried when `uuid` was added to `successResponse` and `updateResponse`.
Verified by regenerating the Java, Python and Go clients with OpenAPI Generator
7.17.0 (the version in ./generator-versions) and running the new test against
the regenerated Java output. Response bodies in the test are verbatim from
Manticore Search 29.0.2.
This file contains hidden or 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
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.
The problem
aggBucketsResultdeclares onlybucketsandafter_key, but half of the aggregations the schemacan request never answer with buckets at all. Measured against Manticore Search 29.0.2:
terms,histogram,date_histogram,range,date_range,composite{"buckets": [...]}(+after_keyforcomposite)min,max,sum,avg{"value": 30.5}median_absolute_deviation{"value": 10, "value_as_string": "10"}percentiles,percentile_ranks{"values": [{"key": 25, "value": 18, "value_as_string": "18"}, …]}keyed: true{"values": {"25": 18, "50": 30.5}}The request side of all of these was modelled in 10.1.1 —
aggPercentiles,aggPercentileRanks,aggMedianAbsoluteDeviation,aggMetric, all reachable fromaggregation. The response side wasnot, so the two halves no longer meet.
Why it is fatal on the Java client, not merely lossy
JSON.javabuilds its mapper withFAIL_ON_UNKNOWN_PROPERTIESenabled, so a metric result raisesand the caller loses the whole
searchResponse, not just that one aggregation. Five of the sixmetric shapes above fail this way on the published 10.2.0; only the bucket ones deserialize. A
faceted search that also asks for percentiles — a common shape, the facets and the price
distribution in one round trip — therefore cannot be read back at all, on any server version. I
checked the wire against both 28.6.6 and 29.0.2 to be sure this was the client and not a server too
old for it.
Reproduce with the published client:
The change
value,value_as_stringandvaluesare added toaggBucketsResult, and each description sayswhich aggregation produces it.
searchResponse.aggregationscarried its own copy of the pre-fixprose and a buckets-only example; both now cover metric results too.
Why
valueandvaluesare left untypedvaluesfollowsbuckets: its shape flips with the request'skeyedflag, array or map.valueis the one worth a second look, becausetype: numberlooks obviously right and is not.An integer aggregation returns an exact integer, which can leave the range a double holds
exactly — measured on 29.0.2:
So
format: doublewould round the first two. And a baretype: numberis worse still in someclients: this generator maps an unformatted number to a 32-bit float, so the Go client would
round every metric above 16 777 216 —
aggTDigest.compressionis already*float32in the currentout/manticoresearch-gofor exactly that reason. Untyped is the only shape that is faithful to allof them, and it matches how
keyis handled in the neighbouringaggBucket. The new test pins theexact-integer case so the decision cannot be undone by accident.
Compatibility
Additive: no schema renamed, no existing accessor changed, existing code keeps compiling and new
getters appear. The one cost worth naming is that added fields land in generated response models,
which in Rust means a struct literal naming every field stops compiling — the same cost the previous
release carried when
uuidwas added tosuccessResponseandupdateResponse.How it was verified
Regenerated the Java, Python and Go clients with OpenAPI Generator 7.17.0 (the version in
./generator-versions) and ran the new test against the regenerated Java output: 8 tests, allgreen, and each of them throws on the published 10.2.0. Response bodies in the test are verbatim
from Manticore Search 29.0.2. The three yml stay in lockstep — only
manticore.ymlwas hand-edited,the two derivatives come from
build.shlines 282-283.out/is deliberately not included.