github-actions[bot] commented on code in PR #69222:
URL: https://github.com/apache/doris/pull/69222#discussion_r4558762446
##########
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:
[P1] Preserve Variant values when reading aggregate intermediate state
The template also reaches `collect_list` through
`TAggregateExpr.param_types`, even when `dropVariantTemplates` removed it from
the executed CAST. Consider this reduced ordinary SELECT plan with one source
row `j = '{"a":1.005}'`:
```text
Aggregate(collect_list(v) AS xs)
Project(CAST(parse_to_variant(j) AS VARIANT<'a':DECIMAL(10,2)>) AS v)
Scan(src)
```
With one aggregation phase, the VARIANT-to-VARIANT cast preserves native
DOUBLE 1.005, so `CAST(xs[1]['a'] AS DECIMAL(10,2))` is 1.00. With two phases,
`ExpressionTranslator.visitAggregateExpression` sends the original templated
parameter type, `AggFnEvaluator` prefers that type over the stripped CAST
node's type, and `AggregateFunctionCollectListData::read` uses its serde to
reparse the intermediate JSON. This assignment therefore turns that same value
into exact DECIMAL 1.005 and the result becomes 1.01. The existing
phase-equivalence test starts from already typed stored values and misses this
case. Aggregate state must preserve the input's numeric representation rather
than reapply an ingestion hint to CAST-produced values.
##########
be/src/exprs/function/parse/variant_string_parse.cpp:
##########
@@ -397,9 +685,91 @@ struct JsonStringToVariantEncoder::Impl {
row.finish();
}
+ // Whether the first byte of the document opens an object or an array,
past what simdjson
+ // itself passes over before a document: a UTF-8 byte order mark and JSON
whitespace.
+ static bool starts_a_container(StringRef json) {
+ size_t pos = 0;
+ if (json.size >= 3 && std::memcmp(json.data, "\xEF\xBB\xBF", 3) == 0) {
+ pos = 3;
+ }
+ for (; pos < json.size; ++pos) {
+ const char c = json.data[pos];
+ if (c != ' ' && c != '\t' && c != '\n' && c != '\r') {
+ return c == '{' || c == '[';
+ }
+ }
+ return false;
+ }
+
+ // Adds the document as the ondemand pass reads it, or nothing: false
means the row it began
+ // was rolled back, and the caller gives the text the DOM parser's verdict.
+ bool add_row_with_template_paths(StringRef json) {
+ auto row = builder.begin_row();
+ bool collected = false;
+ // The DOM parser reads the whole text before any depth or key check
runs, and the ondemand
+ // pass checks as it reads. So on a document whose prefix is over-deep
or duplicate-keyed
+ // AND whose tail is malformed, {"o":{"a":1,"a":2},"k":x}, the
ondemand pass meets the
+ // builder's check where the DOM parser meets the syntax error. It
would throw where the
+ // lenient serde stored the text as a string row, and the strict
functions would report
+ // the prefix's defect where they reported the syntax error. Let the
DOM parser give the
+ // verdict in both: a well-formed document meets the same check there
and fails as it
+ // always did.
+ try {
+ collected = collect_with_template_paths(row, json);
+ } catch (const Exception& exception) {
+ if (exception.code() != ErrorCode::INVALID_ARGUMENT) {
+ throw;
+ }
+ }
+ if (!collected) {
+ // `row` ends here and rolls its partial content back
+ return false;
+ }
+ row.finish();
+ return true;
+ }
+
+ // A root object or array read with the ondemand parser; false on any
simdjson error, and for a
+ // root scalar, which has no path.
+ bool collect_with_template_paths(VariantBatchBuilder::Row& row, StringRef
json) {
+ // One padded copy per encoder, grown to the largest document seen:
ondemand values point
+ // into it, and the builder copies a row's bytes before the next
document is read.
+ const size_t padded_size = json.size + simdjson::SIMDJSON_PADDING;
+ if (padding_buffer.size() < padded_size) {
+ padding_buffer.resize(padded_size);
Review Comment:
[P2] Charge the retained padded JSON buffer to the memory tracker
`padding_buffer` is a `std::string`, so this resize allocates an additional
full-document copy without Doris allocator accounting. The encoder retains its
capacity for the largest document until the whole batch ends. Every
object/array under a DECIMAL template reaches this allocation, as does the
LARGEINT fallback; parallel encoders each retain their own copy. The raw malloc
hook only forwards to jemalloc, so neither query/load consumption nor
allocation-time limit checks include these bytes. The existing third-party DOM
parser buffer does not account for this newly added copy. Use a Doris
allocator-backed byte buffer such as `DorisVector<char>`, or explicitly check,
consume and release its allocated capacity.
--
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]
