raghav-reglobe commented on code in PR #69222:
URL: https://github.com/apache/doris/pull/69222#discussion_r4561345828
##########
be/src/core/data_type_serde/data_type_variant_v2_serde.cpp:
##########
@@ -643,10 +645,16 @@ JsonToVariantOptions serde_json_options() {
} // namespace
+JsonToVariantOptions DataTypeVariantV2SerDe::json_options() const {
+ JsonToVariantOptions options = serde_json_options();
+ options.template_paths = _template_paths;
Review Comment:
@eldenmoon the finding is correct, and I would like your call on the fix
before I build it, because it is a choice about aggregate state and not about
this PR's parsing.
The state of a generic `collect_list` is JSON text per element
(`aggregate_function_collect.h`, `write` / `read`), read back with the argument
type's serde. JSON text cannot say whether `1.005` was a DOUBLE or a DECIMAL,
so the serde has to pick one way to read it:
1. **Read the state without the template** (drop it from
`TAggregateExpr.param_types`, as the CAST node already does). The case in this
thread then gives 1.00 in every phase. The cost: a value this PR stores exactly
on a templated column, for example `0.123456789012345678` on `decimal(38,18)`
or `18446744073709551616` on a LARGEINT path, comes back as a DOUBLE after a
two-phase `collect_list`. The suite pins those two values today
(`collect_one_phase` / `collect_two_phases`), and I expect the two-phase result
to change.
2. **Keep the value's own encoding in the state** (write the element in the
column's binary form instead of JSON text). Both cases come back as they went
in. It changes the state format of `collect_list` / `array_agg` over a VARIANT
argument, so it needs a `be_exec_version` step, or your word that VARIANT V2
does not need one yet.
I think 2 is the correct one, but it is the larger change and it touches
aggregate state, which this PR did not set out to change. If you prefer, I do 1
here, state the limit in the description, and send 2 as its own PR.
P2 (the padded buffer outside memory accounting) is a plain fix and comes
with the next push.
Still open from my earlier reply: whether you want the INSERT / UPDATE /
MERGE typing rule in this PR or as a follow-up.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
