MDEV-40673 CONCAT silently returns the wrong string, dropping its first argument's value, when that argument is JSON_TYPE() or GET_FORMAT() - #5713
Conversation
|
|
61f4d8a to
27a415d
Compare
gkodinov
added
the
External Contribution
gkodinov
left a comment
gkodinov
left a comment
There was a problem hiding this comment.
Thank you for your contribution! This is a preliminary review.
The fix is valid and good enough. I'd add a comment saying why realloc is needed as it seems counter-intuitive.
However, before I approve, please merge the two commits into a single one.
…st argument's value, when that argument is JSON_TYPE() or GET_FORMAT()
CONCAT can lose its first argument when that argument returns a string
that does not own a heap buffer. For example,
CONCAT(JSON_TYPE('{"a":1}'),'X') should return OBJECTX.
Item_func_concat::realloc_result() uses String::alloc() when the
existing result has no allocated capacity. Growing that kind of string
discards the already-materialized prefix.
Use String::realloc() in the zero-capacity case so growth retains the
current contents before subsequent CONCAT arguments are appended.
The regression concatenates the non-heap-owned OBJECT result from
JSON_TYPE with X and checks the complete OBJECTX result.
Bug report: https://jira.mariadb.org/browse/MDEV-40673
387e995 to
ba81787
Compare
gkodinov
left a comment
gkodinov
left a comment
There was a problem hiding this comment.
LGTM. Please stand by for the final review.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Add regression coverage for the documented GET_FORMAT() case.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Fixes CONCAT dropping prefixes from externally backed string results.
Changes:
- Uses
String::realloc()to preserve existing contents. - Adds
JSON_TYPE()regression coverage.
| File | Description |
|---|---|
sql/item_strfunc.cc |
Preserves external string data during concatenation growth. |
mysql-test/main/func_concat.test |
Adds JSON_TYPE() regression coverage. |
mysql-test/main/func_concat.result |
Records the expected OBJECTX output. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| --echo # | ||
| --echo # MDEV-40673: Preserve the result of JSON_TYPE in CONCAT | ||
| --echo # | ||
| SELECT CONCAT(JSON_TYPE('{"a":1}'),'X') AS r; |
sanja-byelkin
left a comment
sanja-byelkin
left a comment
There was a problem hiding this comment.
IMHO it is quite fundamental problem so should be fixed in 10.11.
| # | ||
| SELECT CONCAT(JSON_TYPE('{"a":1}'),'X') AS r; | ||
| r | ||
| OBJECTX |
There was a problem hiding this comment.
add
--echo # End of 10.11 tests
| did not limit the result to max_alloced_packet. But it's not harmful, | ||
| "str" will be reallocated exactly to "length" bytes in case of overflow. | ||
| */ | ||
| uint new_length= MY_MAX(str->alloced_length() * 2, length); |
There was a problem hiding this comment.
having realloc in both cases we can just remove if (str->alloced_length() == 0) return str->realloc(length);at all and the code will be the same. In this case
uint new_length= MY_MAX(str->alloced_length() * 2, length); turns to new_length= MY_MAX(0 * 2, length) and so it will be new_length= length and then realloc...
CONCAT can lose its first argument when that argument returns a string that does not own a heap buffer. For example, CONCAT(JSON_TYPE('{"a":1}'),'X') should return OBJECTX.
Bug report: https://jira.mariadb.org/browse/MDEV-40673
Root cause
Item_func_concat::realloc_result() uses String::alloc() when the existing result has no allocated capacity. Growing that kind of string discards the already-materialized prefix.
Changes
Regression coverage
The regression is integrated into the existing
main.func_concattest.The regression concatenates the non-heap-owned OBJECT result from JSON_TYPE with X and checks the complete OBJECTX result.
mysql-test/main/func_concat.testmysql-test/main/func_concat.resultValidation
On
11.4atd10e5d726799b1cd57cc866f40aad68c720803da:main.func_concattest, including the integrated regression, failed on the unchanged target branch at the regression case and passed with this fix.main.func_json,main.select.