MDEV-38591: Implement MEMBER OF and NOT MEMBER OF operators - #5278
MDEV-38591: Implement MEMBER OF and NOT MEMBER OF operators#5278kjarir wants to merge 2 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements the SQL standard MEMBER OF and NOT MEMBER OF operators (MDEV-38591) and updates JSON_QUOTE to support numeric and NULL arguments (MDEV-13645). The review feedback highlights several critical and high-severity issues: a potential double-free or use-after-free vulnerability in shallow_copy due to raw pointer copying, type safety issues from using specific subclass pointers for helper items instead of Item*, double evaluation of non-deterministic arguments in val_bool(), and the need to sync helper arguments during optimization in update_used_tables(). Additionally, minor improvements are suggested to value-initialize the je_val struct and clean up C-style casts.
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.
grooverdan
added
GSoC
External Contribution
There was a problem hiding this comment.
Pull request overview
This PR adds SQL-standard JSON containment predicates value MEMBER OF (json_doc) and value NOT MEMBER OF (json_doc) by introducing a new Item_func_member_of that composes existing JSON items (JSON_QUOTE + JSON_CONTAINS), plus parser/lexer support and dedicated MTR coverage. It also updates JSON_QUOTE behavior for numeric and NULL inputs (and adjusts tests/results accordingly).
Changes:
- Extend the SQL grammar/lexer to parse
MEMBER OF/NOT MEMBER OFand print it back inEXPLAIN EXTENDED. - Add
Item_func_member_ofimplementation with explicit JSON validation/error attribution and optimizer traversal hooks. - Update
JSON_QUOTEto handle numeric andNULLinputs, and add/adjust MTR test coverage and expected results.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| sql/sql_yacc.yy | Adds MEMBER token, precedence, and grammar rules for MEMBER OF / NOT MEMBER OF; adjusts %expect. |
| sql/lex.h | Registers MEMBER keyword in the lexer symbol table. |
| sql/item_jsonfunc.h | Declares Item_func_member_of (new composed JSON predicate item). |
| sql/item_jsonfunc.cc | Implements Item_func_member_of; changes JSON_QUOTE to accept numeric/NULL inputs. |
| sql/item_cmpfunc.h | Adds a 2-argument Item_func_opt_neg constructor used by the new predicate item. |
| mysql-test/suite/json/t/member_of.test | New JSON suite test coverage for MEMBER OF / NOT MEMBER OF semantics. |
| mysql-test/suite/json/r/member_of.result | Expected results for the new MEMBER OF test. |
| mysql-test/suite/json/t/json_no_table.test | Updates expectations for JSON_QUOTE/JSON_UNQUOTE numeric handling. |
| mysql-test/suite/json/r/json_no_table.result | Updates expected results for JSON_QUOTE/JSON_UNQUOTE numeric handling. |
| mysql-test/main/func_json.test | Adds coverage for JSON_QUOTE numeric/NULL behavior and composition. |
| mysql-test/main/func_json.result | Expected results for the new JSON_QUOTE coverage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
53d1e25 to
731ac67
Compare
Resolve review feedback from Gemini, Copilot, grooverdan, and Rucha on PR MariaDB#5278: - Implement robust shallow_copy() to null cloned helper item pointers and zero out je_val, avoiding double-free / use-after-free risks. - Update json_quote_item and json_contains_item from specific subclass pointers to base Item* pointers. - Handle NULL input gracefully in Item_func_json_quote::val_str(). - Safely check args[0]->null_value after delegating containment check to json_contains_item. - Correctly cast to Item_func* (using explicit static_cast) in update_used_tables() to prevent pointer mismatch under multiple inheritance, checking for FUNC_ITEM before accessing arguments.
If args[1] (the JSON container/array) is a compile-time constant,
avoid re-evaluating and re-scanning it on every row.
Follow the a2_constant/a2_parsed pattern established by
Item_func_json_contains:
- a1_constant: set in fix_length_and_dec() via args[1]->const_item()
- a1_parsed: latches to true after first evaluation when a1_constant
is true; stays false for variable args, forcing per-row evaluation
- js_doc_cached: stores the validated String* (NULL if args[1] is
SQL NULL or malformed JSON)
On a constant container: validate once on the first row, cache the
result, skip both val_json() and the full json_scan_next() loop on
subsequent rows. For SQL NULL or invalid JSON, cache as NULL and
return SQL NULL on subsequent rows without repeating the warning.
shallow_copy() resets a1_parsed and js_doc_cached so a cloned item
does not inherit a stale pointer to the original's transient buffers.
Addresses grooverdan's review comment on fix_length_and_dec() in
PR MariaDB#5278: 'Note this function is a good place to preprocess if args
are constant to save expensive processing on each row.'
grooverdan
left a comment
grooverdan
left a comment
There was a problem hiding this comment.
I need to get transformations looked at closer.
Per review on PR MariaDB#5278: - Restore specific subclass pointer types for json_quote_item and json_contains_item (Item_func_json_quote* / Item_func_json_contains*) - Use fix_length_and_dec() instead of fix_fields_if_needed() for helper items in fix_length_and_dec(); use Item_bool_func::fix_length_and_dec() to replace manual max_length/set_maybe_null - Remove pre-validation JSON scan loops from val_bool() — json_contains already validates internally; remove je_val, a1_constant, a1_parsed, js_doc_cached and related state entirely - Remove compile() override; simplify walk() and transform() to delegate through json_contains_item (which naturally covers json_quote_item as its argument); inline propagate_equal_fields() in header - shallow_copy(): revert to one-liner — fix_length_and_dec() always re-initializes state before any evaluation - Remove SHOW WARNINGS from member_of.test (MTR includes warnings in output by default); error messages for malformed JSON now correctly attribute to internal json_contains delegation - Update class comment to reflect current architecture
Fixes all 12 review items from grooverdan's review of PR MariaDB#5278: - Fix stale a2_constant/a2_parsed cache in Item_func_json_contains that caused wrong MEMBER OF results for non-constant candidates (root cause: json_quote_item's const_item_cache was never updated after fix_length_and_dec, so it was read as constant-true by Item_func_json_contains::fix_length_and_dec). Fixed by calling update_used_tables() on both helper items before they're consumed. - Fix null_value check ordering in val_bool() (json_contains_item's null_value must be checked before args[0]'s, since args[0] is only evaluated as a side effect of json_contains_item->val_bool()). - Restructure Item_func_json_quote::val_str() switch to handle ROW_RESULT (single-column unwrap) and TIME_RESULT, replace silent NULL return on unknown types with ER_WRONG_ARGUMENTS, and set maybe_null accordingly. - Simplify null check and numeric-append logic in val_str(). - Name immediate parent class explicitly in fix_length_and_dec()/walk() (Item_func_opt_neg); transform() intentionally kept as direct pointer call to avoid double transform_args() (see PR reply). - Remove redundant is_json_type()/val_json() pre-check in val_bool() and unused tmp_candidate member. - Found and fixed a latent bug in propagate_equal_fields() (was returning json_contains_item instead of this, violating the build_equal_items cond==this invariant) while regression-testing equality propagation for update_used_tables() (see PR reply) - kept update_used_tables() override, it is not redundant. - Added mentor's four null-literal-vs-JSON-null test cases, a full cross-join regression test matching the mentor's original wrong-result table, a ROW_RESULT error-path test, and an equality-propagation regression test. All changes verified against real source (not assumed) and the full suite/json mtr suite (26/26 pass).
|
Re: Item_func_opt_neg does not override transform(), so calling Item_func_opt_neg::transform() resolves to Item_func::transform(), which unconditionally calls transform_args() first. Item_func_member_of::transform() already calls transform_args() earlier in the function (to also transform json_contains_item). Calling Item_func_opt_neg::transform() at the end would run transform_args() a second time, double-transforming args[]. Kept |
|
Re: removing update_used_tables() override on Item_func_member_of: This turned out not to be redundant. It re-syncs json_quote_item's and json_contains_item's internal argument pointers to args[0]/args[1] whenever the optimizer substitutes an equal item during equality propagation. Found this while adding a regression test for a join condition (t1.a = t2.b AND t1.a MEMBER OF (t2.k)) — removing the override, combined with a separate pre-existing bug in propagate_equal_fields() (it was returning json_contains_item instead of this, violating the cond==this invariant in build_equal_items), triggered a DBUG_ASSERT crash in sql_select.cc. Fixed propagate_equal_fields() to correctly return this after delegating to json_contains_item, kept update_used_tables() as originally written, and added the join case as a regression test in member_of.test. |
|
Thanks for the exact repro! Our earlier test used Corrected using your actual example: an ordinary scalar-returning stored function ( Updated |
grooverdan
left a comment
grooverdan
left a comment
There was a problem hiding this comment.
Before final merge:
- rebase/squash
- mtr OPTIMIZER_TRACE to notembedded.test file.
Otherwise 🥳 - well done @kjarir !
There was a problem hiding this comment.
This is a preliminary review. Please squash the commits down to 2:
- the MDEV-13645 one. I would actually prefer that this fix is done in a separate PR, since it's needed down to 10.11.
- the current one.
Also, please make sure the latest main is merged and conflicts are resolved.
I'd also appreciate an explanation on why Item_func_json_contains() cannot be used, but that's optional since it beyond the scope of the preliminary review.
JSON_QUOTE() previously only accepted STRING_RESULT arguments. Extend it to handle all argument types correctly: - Numeric types (INT_RESULT, REAL_RESULT, DECIMAL_RESULT): call val_str() which fills tmp_s with the text representation, then wrap it in JSON double-quotes. - TIME_RESULT: treat the value as a string literal (quoted, not escaped). - ROW_RESULT with a single column: unwrap and re-evaluate as string. - SQL NULL input: return the 4-character JSON literal "null" (not SQL NULL), as required by the MEMBER OF operator (MDEV-38591) which needs json_quote_item to produce a well-formed JSON value to pass to json_contains_item. - Fix fix_length_and_dec() to declare max_char_length as at least 4 characters via MY_MAX(args[0]->max_char_length() * 12 + 2, 4ULL): when the argument is a literal NULL its max_char_length() is 0, which would yield 2, causing the "null" output to be truncated to "nu" under the --view-protocol field-length check. The minimum of 4 ensures "null" is never truncated. Tests in mysql-test/main/func_json.test and mysql-test/suite/json/r/json_no_table are updated to reflect the new behavior.
91d35c3 to
66a9345
Compare
Add support for the SQL/JSON MEMBER OF predicate and its negation
NOT MEMBER OF, which test whether a scalar value is an element of a
JSON array:
SELECT 2 MEMBER OF ('[1, 2, 3]'); -- 1
SELECT 5 NOT MEMBER OF ('[1, 2, 3]'); -- 1
1. Grammar (sql/sql_yacc.yy, sql/lex.h):
- Added MEMBER_SYM keyword to the lexer keyword table.
- Added 'expr MEMBER_SYM OF_SYM ( expr )' production at CMP_PRECEDENCE,
constructing Item_func_member_of(thd, $1, $5).
- Added 'expr NOT_SYM MEMBER_SYM OF_SYM ( expr )' for the negated form,
setting Item_func_opt_neg::negated = true.
2. Item class (sql/item_jsonfunc.h, sql/item_jsonfunc.cc):
- Item_func_member_of inherits from Item_func_opt_neg (which provides the
negated flag and NOT MEMBER OF printing for free).
- Composition model: fix_length_and_dec() constructs two private helper items
outside the normal args[] array:
* json_quote_item (Item_func_json_quote*): wraps the candidate if it is
not already a JSON-typed column, so any scalar SQL value becomes a
well-formed JSON string literal suitable for containment testing.
* json_contains_item (Item_func_json_contains*): does the actual lookup
of the quoted candidate inside the JSON array container.
Both helpers have update_used_tables() called after fix_length_and_dec() to
ensure const_item_cache is correct before json_contains_item reads it;
incorrect const_item_cache caused the stale-cache bug (wrong results on
repeated rows with non-constant candidate).
- val_bool() calls json_contains_item->val_bool() and propagates
json_contains_item->null_value (container NULL gives SQL NULL); negates
the result if negated is set.
- walk(), transform(), update_used_tables(), and propagate_equal_fields() are
overridden to explicitly visit the private helper items, which are invisible
to the default Item_func traversal of args[].
- set_maybe_null() is called in fix_length_and_dec() for cursor protocol
compatibility.
3. Dependency on MDEV-13645 and intentional NULL-candidate semantics:
MDEV-13645 made JSON_QUOTE(NULL) return the string "null" rather than SQL
NULL. MEMBER OF exploits this: when the candidate is SQL NULL, json_quote_item
produces the four-character string "null", which json_contains_item correctly
matches against a JSON null literal inside the array. This means:
- NULL MEMBER OF ('[null]') -> 1 (SQL NULL equals JSON null)
- NULL MEMBER OF ('[1,2,3]') -> 0 (no JSON null in array)
- NULL MEMBER OF (NULL) -> NULL (container IS NULL propagates)
This intentionally diverges from MySQL's documented "If value is NULL,
returns NULL" rule, per reviewer grooverdan's explicit request.
4. Kill-query / interruption support:
Item_func_member_of::val_bool() is interruptible because it delegates
entirely to json_contains_item->val_bool(), which respects THD::killed
checks through the normal JSON traversal path.
5. Test coverage:
- mysql-test/suite/json/t/member_of.test: 21 sections covering simple
membership, type-strict comparison, SQL NULL propagation, malformed JSON,
nested arrays/objects, scalar-as-container, table-column evaluation,
prepared statements, constant-caching regression, EXPLAIN EXTENDED,
equality-propagation regression, ROW_RESULT rejection, stored function
VARCHAR and JSON return types, TIME_RESULT, and RETURNS ROW rejection.
- mysql-test/suite/json/t/member_of_notembedded.test: optimizer trace test
confirming MEMBER OF participates correctly in condition_processing, with
equality_propagation folding 'x MEMBER OF (j) AND x=10' into
'10 MEMBER OF (j)'.
1357fe0 to
7dbc4be
Compare
gkodinov
left a comment
gkodinov
left a comment
There was a problem hiding this comment.
Now the following tests are failing, please re-record: perfschema.start_server_low_digest_sql_length perfschema.digest_view
This PR adds native support for the SQL-standard
value MEMBER OF (json_doc)andvalue NOT MEMBER OF (json_doc)containment predicates. It builds on MDEV-13645 (JSON_QUOTE numeric/NULL fixes) which is included in this branch.Rather than reimplementing a JSON search engine,
Item_func_member_ofcomposes existing items internally. It holdsItem_func_json_quoteandItem_func_json_containsas child items. Infix_length_and_dec(), if the candidate operand is not already JSON-typed, it gets wrapped injson_quote_item. The actual containment check is then delegated to an internaljson_contains_item(json_doc, candidate). AST traversal methods (walk(),transform(),compile(),propagate_equal_fields(),update_used_tables()) explicitly expose these hidden child items to the optimizer, following theItem_in_optimizerprecedent.negation:
Item_func_member_ofderives fromItem_func_opt_neg, the same pattern used byItem_func_betweenandItem_func_in, rather than wrapping in a genericItem_func_not. NULL propagation is handled directly inval_bool()vianegated ? !res : reson non-NULL outcomes.print()emitsvalue not member of (json_doc)to preserve surface syntax in optimizer traces andEXPLAIN EXTENDED.json validation and error attribution:
val_bool()performs explicit JSON validation before delegating tojson_contains. A persistentjson_engine_t je_valmember with a stack buffer initialized at prepare-time runs a full-document scan to catch trailing garbage that a simplejson_read_value()would miss. Syntax errors are intercepted and reported viareport_json_error_ex()attributed to"member of"with correct 0-based argument indices. This prevents warning misattribution to"json_contains"or wrong argument numbers leaking through to the user.Parser:
MEMBER_SYMis registered alphabetically inlex.hand declared via%token. It is added tokeyword_sp_var_and_labelto keep it non-reserved, resolving prior reserved keyword conflicts with MySQL. Grammar rules cover both forms:A state-by-state conflict analysis confirms zero new conflicts introduced. Parser conflict count stays at 71, no
%expectbump needed.Test coverage:
New tests in
mysql-test/suite/json/t/member_of.testcover scalar member checks, type-strict matches, correct NULL propagation under negation, malformed JSON warning attribution, prepared statement re-execution, nested container checks, table column cross-joins, andEXPLAIN EXTENDEDround-trip print verification.Out of scope: Multi-valued index support (MySQL WL#8955/WL#8763) is not part of this implementation.
Please Note: