MDEV-36986 Support tracing arrays of primitive types

[bsrikanth-mariadb]

Json_writer had two separate code paths: add_unquoted_str() for
numbers/bool/null, and add_escaped_str() which added the surrounding
quotes itself for strings. Single_line_formatting_helper, which
buffers consecutive array/object elements to decide if they fit on
one line, assumed only strings could ever be buffered and always
wrapped the flushed values in quotes. As a result, arrays of numbers
(e.g. "depends_on_map_bits", "rec_per_key") were incorrectly rendered
with their elements quoted as strings.

Unify both paths into add_escaped_quoted_str(): the caller now hands
over bytes that are already in their final on-the-wire form. String
escaping (json_escape_to_string) writes its own surrounding quotes,
while numbers/bool/null are passed through unquoted, so the one-line
helper just concatenates the buffered payloads on flush instead of
adding quotes itself.

Also:

• Fix Json_writer_array::add(ulonglong)/(size_t), which went through
  add_ll() with a cast to longlong and corrupted large unsigned
  values (e.g. ULLONG_MAX); route them through add_ull() instead.
• Fix mysql-test/include/opt_context_schema.inc: "subquery_runs" was
  nested inside the preceding object instead of being a sibling
  member, and "rec_per_key" items are now declared as "number" to
  match the corrected output.
• Update recorded .result files for opt_trace, opt_context_*, and
  subselect_mat_analyze_json to reflect numbers/booleans no longer
  being quoted inside JSON arrays.
• Extend unittest/sql/my_json_writer-t.cc with coverage for arrays
  of primitives: plain integers, mixed types, sizes, multi-line
  arrays, values flushed before a nested object, and strings that
  were already escaped by the one-line buffer.

[spetrunia]

While at it:

Json_writer_array::add(ulonglong) cast to longlong, printing values above LLONG_MAX as negative. Use add_ull(). add(size_t) had the same bug and is compiled out on _WIN64, so fixing only one would have made the output platform-dependent

AI says to this part:

add(size_t) sibling left unfixed (code-reviewer) — sql/my_json_writer.h:518: Json_writer_object::add(const char*, size_t) still binds to the integral template on _WIN64 and reaches add_ull() vs add_ll() elsewhere — the same platform-dependent signedness bug the commit claims to eliminate, just on the object-class sibling instead of the array-class one that got fixed.

Is this meaningful?

If yes, let's also split this change into separate commit from the quoting fix (this is trivial, right?)

[spetrunia]

What if we fix it the other way:

void Json_writer::add_escaped_str(const char* str, size_t num_bytes)

Make this add_escaped_quoted_str. That is, the caller guarantees that the string is both escaped and quoted.

Make json_escape_to_string also do quoting. It copies the data anyway, so it can add quotes too.

void Json_writer::add_unquoted_str(const char* str)
{
  size_t len= strlen(str);
  add_unquoted_str(str, len);
}

This one has only a few users all of which know the string length. Remove it.

void Json_writer::add_unquoted_str(const char* str, size_t len)
{

Do we need this or it's the same as add_escaped_quoted_str ?

Then, Single_line_formatting_helper::quoted will not be needed.

[bsrikanth-mariadb]

While at it:

Json_writer_array::add(ulonglong) cast to longlong, printing values above LLONG_MAX as negative. Use add_ull(). add(size_t) had the same bug and is compiled out on _WIN64, so fixing only one would have made the output platform-dependent

AI says to this part:

add(size_t) sibling left unfixed (code-reviewer) — sql/my_json_writer.h:518: Json_writer_object::add(const char*, size_t) still binds to the integral template on _WIN64 and reaches add_ull() vs add_ll() elsewhere — the same platform-dependent signedness bug the commit claims to eliminate, just on the object-class sibling instead of the array-class one that got fixed.

Is this meaningful?

If yes, let's also split this change into separate commit from the quoting fix (this is trivial, right?)

Yes Sergei, they seem relevant. sure, will have it in a separate commit. But, I assume we can do it in he same PR, right?

[bsrikanth-mariadb]

What if we fix it the other way:

void Json_writer::add_escaped_str(const char* str, size_t num_bytes)

Make this add_escaped_quoted_str. That is, the caller guarantees that the string is both escaped and quoted.

Make json_escape_to_string also do quoting. It copies the data anyway, so it can add quotes too.

void Json_writer::add_unquoted_str(const char* str)
{
  size_t len= strlen(str);
  add_unquoted_str(str, len);
}

This one has only a few users all of which know the string length. Remove it.

void Json_writer::add_unquoted_str(const char* str, size_t len)
{

Do we need this or it's the same as add_escaped_quoted_str ?

Then, Single_line_formatting_helper::quoted will not be needed.

sure, made these changes.

[spetrunia]

Let-me-run-mreview-for-you:

Blockers (both agents independently found the same root cause)

- sql/my_json_writer.cc:288-304 — Json_writer::add_str()'s escape-failure fallback 
(JSON_ERROR_ILLEGAL_SYMBOL / JSON_ERROR_OUT_OF_SPACE) now passes bare literals like "String with illegal unicode symbol" to add_escaped_quoted_str(), which no longer adds the surrounding quotes. 
Result: on an escaping failure the writer silently emits invalid JSON ("col": String with illegal unicode symbol), 
with invalid_json left unset and nothing logged — the corruption is undetectable except as a downstream parse error at an unrelated offset.
- Same defect, same two lines, flagged again on the OUT_OF_SPACE path.

Important highlights

- add_double() now emits bare nan/inf unquoted on every path — invalid JSON for NaN/Inf costs.
- The fix for the static_cast<longlong> overflow corruption in Json_writer_array::add(ulonglong/size_t) was not mirrored in the sibling Json_writer_object, which still corrupts values ≥ 2^63 the same way.
- add_escaped_quoted_str() is now misnamed — it's a raw passthrough for bools/null/numbers, none of which are escaped or quoted.
- Doc comments in .cc and .h contradict each other about the function's contract — this is the exact misreading that produced the Blocker.
- A new unit test labeled "Multi-line array of integers" actually asserts single-line output (69 ≤ 80 col limit) — doesn't cover the path it claims to.
- No test coverage at all for either add_str() escape-failure branch — which is why the Blocker shipped invisibly.
- Style nit promoted to Important: space-before-= violations, and a whitespace-only unrelated-line edit.

[spetrunia]

Can we also remove this function:

private:
  void add_escaped_quoted_str(const char* val);

I think all callers do know the string length?
Typical usage looks like

  my_snprintf(buf, sizeof(buf), "%lld", val);
  add_escaped_quoted_str(buf);

where my_snprintf returns the length.

[spetrunia]

adding a zero-length string should be invalid. Empty strings should be quoted.
writing a non-quoted empty string would produce invalid json like '{"memb": }'

[spetrunia]

"First and the last bytes of buf are used for double quotes".

[spetrunia]

Please address the above and then it's ok to push.