Skip to content

MDEV-16462: explain format=json produces illegal json text#5361

Open
bsrikanth-mariadb wants to merge 1 commit into
mainfrom
13.2-MDEV-16462-explain-format-json-produces-illegal-text
Open

MDEV-16462: explain format=json produces illegal json text#5361
bsrikanth-mariadb wants to merge 1 commit into
mainfrom
13.2-MDEV-16462-explain-format-json-produces-illegal-text

Conversation

@bsrikanth-mariadb

@bsrikanth-mariadb bsrikanth-mariadb commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

The attached_condition field of the produced json text had "String with illegal unicode symbol" instead of the proper unicode value.

The reason is that, the item field when being written to the json writer in write_item(), used a String with charset my_charset_bin, and that prevented the string from being escaped.

Solution is to change: -

  1. charset to my_charset_utf8mb4_bin in sql_explain.cc. and in Item::print_value()
  2. handle string type specially if the charset is my_charset_bin i.e. to append hex value.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request fixes MDEV-16462, an issue where EXPLAIN FORMAT=JSON produced illegal JSON in the attached_condition field. The fix modifies sql/sql_explain.cc to use system_charset_info instead of &my_charset_bin for the item string buffer and passes the string directly to writer->add_str(). It also adds a corresponding test case. There are no review comments, so I have no feedback to provide.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

@spetrunia spetrunia left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bsrikanth-mariadb bsrikanth-mariadb force-pushed the 13.2-MDEV-16462-explain-format-json-produces-illegal-text branch from ea83463 to 5a01ebb Compare July 8, 2026 12:25
@bsrikanth-mariadb bsrikanth-mariadb marked this pull request as draft July 8, 2026 12:30
@bsrikanth-mariadb bsrikanth-mariadb force-pushed the 13.2-MDEV-16462-explain-format-json-produces-illegal-text branch 2 times, most recently from 3041efc to 4b1548e Compare July 9, 2026 03:53
Comment thread sql/sql_explain.cc
@spetrunia

Copy link
Copy Markdown
Member

Let's try with these changes:

diff --git a/sql/item.cc b/sql/item.cc
index 3bdd7f82da3..4d7e3c9a2fe 100644
--- a/sql/item.cc
+++ b/sql/item.cc
@@ -556,7 +556,19 @@ void Item::print_value(String *str)
     switch (cmp_type()) {
     case STRING_RESULT:
     case TIME_RESULT:
-      append_unescaped(str, ptr->ptr(), ptr->length());
+      if (ptr->charset() == &my_charset_bin)
+      {
+        //str->append('\'');
+        //TODO: but this won't escape the "'" or "\" characters!
+        //str->append_semi_hex(ptr->ptr(), ptr->length(), ptr->charset());
+        //str->append('\'');
+        StringBuffer<64> escaped_val;
+        escaped_val.set_hex(ptr->ptr(), ptr->length());
+        str->append(STRING_WITH_LEN("0x"));
+        str->append(escaped_val);
+      }
+      else
+        append_unescaped(str, ptr->ptr(), ptr->length());
       break;
     case DECIMAL_RESULT:
     case REAL_RESULT:
diff --git a/sql/sql_explain.cc b/sql/sql_explain.cc
index a212f86fe18..288dd0637c7 100644
--- a/sql/sql_explain.cc
+++ b/sql/sql_explain.cc
@@ -1784,7 +1784,7 @@ static void write_item(Json_writer *writer, Item *item)
   item->print(&str, QT_EXPLAIN);
 
   thd->variables.option_bits= save_option_bits;
-  writer->add_escaped_str(str.c_ptr_safe(), str.length());
+  writer->add_str(str.c_ptr_safe(), str.length());
 }
 
 static void append_item_to_str(String *out, Item *item)

@bsrikanth-mariadb bsrikanth-mariadb force-pushed the 13.2-MDEV-16462-explain-format-json-produces-illegal-text branch 2 times, most recently from 07a8e7e to 0145a80 Compare July 10, 2026 03:14
@bsrikanth-mariadb bsrikanth-mariadb marked this pull request as ready for review July 10, 2026 03:16
Comment thread mysql-test/main/explain_json.result Outdated
@spetrunia

Copy link
Copy Markdown
Member

What will happen in Item::print_value for TIME_RESULT?
I've tried this and I've got a TIME value with my_charset_numeric=latin1.
But just for code clarity purposes, please change the code structure to be :

    case STRING_RESULT:
      if (ptr->charset() == &my_charset_bin)
      {
        ... 
        break;
      }
    case TIME_RESULT:
        append_unescaped(str, ptr->ptr(), ptr->length());
      break;

@bsrikanth-mariadb bsrikanth-mariadb force-pushed the 13.2-MDEV-16462-explain-format-json-produces-illegal-text branch from 0145a80 to a619e2c Compare July 11, 2026 04:12
The attached_condition field of the produced json text had
"String with illegal unicode symbol" instead of the proper unicode
value.

The reason is that, the item field when being written to the json
writer in write_item(), used a String with charset my_charset_bin,
and that prevented the string from being escaped.

Solution is to change: -
1. charset to my_charset_utf8mb4_bin in sql_explain.cc.
2. Item::print_value(), handle string type specially if
   the charset is my_charset_bin by appending hex value.
@bsrikanth-mariadb bsrikanth-mariadb force-pushed the 13.2-MDEV-16462-explain-format-json-produces-illegal-text branch from a619e2c to 16020a0 Compare July 11, 2026 04:50
@bsrikanth-mariadb

Copy link
Copy Markdown
Contributor Author

What will happen in Item::print_value for TIME_RESULT? I've tried this and I've got a TIME value with my_charset_numeric=latin1. But just for code clarity purposes, please change the code structure to be :

    case STRING_RESULT:
      if (ptr->charset() == &my_charset_bin)
      {
        ... 
        break;
      }
    case TIME_RESULT:
        append_unescaped(str, ptr->ptr(), ptr->length());
      break;

yes, changed it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

2 participants