Functions: fix json valid and json extract in conditions - #11036
Conversation
Signed-off-by: yongman <yming0221@gmail.com>
ti-chi-bot
Bot
added
do-not-merge/needs-triage-completed
release-note-none
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughChangesThe filter analyzer tracks Guarded JSON filtering
Estimated code review effort: 3 (Moderate) | ~25 minutes Mergeability Score: ⚪ Minimal · up to The PR is merge-ready after normal checks and review; no actionable merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant FilterBuilder
participant DAGExpressionAnalyzer
participant FunctionCastStringAsJson
participant JSONParser
FilterBuilder->>DAGExpressionAnalyzer: build filter with JSON_VALID
DAGExpressionAnalyzer->>DAGExpressionAnalyzer: record guard for string expression
DAGExpressionAnalyzer->>FunctionCastStringAsJson: set ignore_invalid_json
FunctionCastStringAsJson->>JSONParser: parse guarded input
JSONParser-->>FunctionCastStringAsJson: JSON value or JSON null
DAGExpressionAnalyzer-->>FilterBuilder: guarded filter expression
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
dbms/src/Functions/FunctionsJson.h (1)
1741-1751: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a comment that explains the
checkJsonValidre-check.The condition
!ignore_invalid_json || checkJsonValid(...)reads as inverted at first glance. It throws in tolerant mode when TiFlash's own validator accepts the input.The intent is correct: a simdjson error on input that
checkJsonValidaccepts is not an invalidity error. It indicates a different failure, for example a depth or capacity limit. Such an error must not be converted into a filtered-out row.State that intent inline so a later reader does not "simplify" the condition.
📝 Proposed comment
const auto & json_elem = parser.parse(slice.data, slice.size); if (unlikely(json_elem.error())) { + // In tolerant mode, only true invalidity may become a JSON null placeholder. + // If checkJsonValid accepts the input, simdjson failed for another reason + // (for example a depth or capacity limit), so keep throwing. if (!ignore_invalid_json || checkJsonValid(reinterpret_cast<const char *>(slice.data), slice.size)) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dbms/src/Functions/FunctionsJson.h` around lines 1741 - 1751, Add an inline comment immediately above the condition in the JSON error-handling block explaining that checkJsonValid distinguishes true invalid JSON from simdjson failures such as depth or capacity limits; in tolerant mode, throw when TiFlash accepts the input so non-invalidity errors are not converted into filtered rows. Do not change the condition or surrounding behavior.dbms/src/Functions/tests/gtest_json_valid.cpp (1)
156-199: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the exception message in the negative cases.
ASSERT_THROW(..., Exception)passes for anyDB::Exception. These four cases are the core negative assertions of this PR. Each one must fail because JSON parsing stayed strict, not because of an unrelated setup error such as a bad field type.Use a matcher on the message so the test proves the intended cause.
♻️ Proposed change
+ auto assert_invalid_json_throw = [&](const google::protobuf::RepeatedPtrField<tipb::Expr> & conditions) { + try + { + execute_filter(conditions); + FAIL() << "expected an Invalid JSON text exception"; + } + catch (const Exception & e) + { + ASSERT_TRUE(e.message().find("Invalid JSON text") != String::npos) << e.message(); + } + }; + google::protobuf::RepeatedPtrField<tipb::Expr> reversed_conditions; *reversed_conditions.Add() = is_not_null; *reversed_conditions.Add() = json_valid; - ASSERT_THROW(execute_filter(reversed_conditions), Exception); + assert_invalid_json_throw(reversed_conditions);Apply the same replacement to
unguarded_conditions,or_conditions, andwrapped_conditions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dbms/src/Functions/tests/gtest_json_valid.cpp` around lines 156 - 199, Update the four negative assertions in the test around reversed_conditions, unguarded_conditions, or_conditions, and wrapped_conditions to verify that execute_filter throws DB::Exception with a message matching the expected strict JSON parsing failure. Apply the same message matcher consistently to each ASSERT_THROW-style check, preserving the existing condition setup.dbms/src/Flash/Coprocessor/DAGExpressionAnalyzerHelper.cpp (1)
205-218: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffExtract the guard snapshot-and-restore into an RAII scope on
DAGExpressionAnalyzer. The copy-build-restore sequence forjson_valid_guarded_exprsis written by hand in two translation units, and the helper reaches directly into a private analyzer member to do it. One small RAII type removes both the duplication and the cross-class member access, and it also makes the restore exception-safe at each site.
dbms/src/Flash/Coprocessor/DAGExpressionAnalyzerHelper.cpp#L205-L218: replace the three manualanalyzer->json_valid_guarded_exprscopies and moves with two nested scope objects, one for the whole function and one per child.dbms/src/Flash/Coprocessor/DAGExpressionAnalyzer.cpp#L1033-L1038: replaceguards_before_conditioncopy and move-back with the same scope object around thegetActionscall.dbms/src/Flash/Coprocessor/DAGExpressionAnalyzer.h#L323-L333: declare the scope type, for exampleclass JsonValidGuardScope, that savesjson_valid_guarded_exprson construction and restores it on destruction, and expose arecordJsonValidGuardsentry point so the helper no longer touches the member directly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dbms/src/Flash/Coprocessor/DAGExpressionAnalyzerHelper.cpp` around lines 205 - 218, Introduce DAGExpressionAnalyzer::JsonValidGuardScope in dbms/src/Flash/Coprocessor/DAGExpressionAnalyzer.h (lines 323-333) to snapshot json_valid_guarded_exprs on construction and restore it on destruction, and expose recordJsonValidGuards for helper use. In dbms/src/Flash/Coprocessor/DAGExpressionAnalyzerHelper.cpp (lines 205-218), replace the manual whole-function and per-child copies/moves with nested scope objects, routing guard recording through the public entry point. In dbms/src/Flash/Coprocessor/DAGExpressionAnalyzer.cpp (lines 1033-1038), replace the guards_before_condition copy and restoration with the same scope around getActions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/fullstack-test/expr/json_valid.test`:
- Around line 31-44: Update both regression queries around the
late-materialization settings to also set tidb_allow_mpp=1, tidb_enforce_mpp=1,
and tidb_isolation_read_engines='tiflash' in each independent mysql session,
ensuring both queries exercise the TiFlash MPP path.
---
Nitpick comments:
In `@dbms/src/Flash/Coprocessor/DAGExpressionAnalyzerHelper.cpp`:
- Around line 205-218: Introduce DAGExpressionAnalyzer::JsonValidGuardScope in
dbms/src/Flash/Coprocessor/DAGExpressionAnalyzer.h (lines 323-333) to snapshot
json_valid_guarded_exprs on construction and restore it on destruction, and
expose recordJsonValidGuards for helper use. In
dbms/src/Flash/Coprocessor/DAGExpressionAnalyzerHelper.cpp (lines 205-218),
replace the manual whole-function and per-child copies/moves with nested scope
objects, routing guard recording through the public entry point. In
dbms/src/Flash/Coprocessor/DAGExpressionAnalyzer.cpp (lines 1033-1038), replace
the guards_before_condition copy and restoration with the same scope around
getActions.
In `@dbms/src/Functions/FunctionsJson.h`:
- Around line 1741-1751: Add an inline comment immediately above the condition
in the JSON error-handling block explaining that checkJsonValid distinguishes
true invalid JSON from simdjson failures such as depth or capacity limits; in
tolerant mode, throw when TiFlash accepts the input so non-invalidity errors are
not converted into filtered rows. Do not change the condition or surrounding
behavior.
In `@dbms/src/Functions/tests/gtest_json_valid.cpp`:
- Around line 156-199: Update the four negative assertions in the test around
reversed_conditions, unguarded_conditions, or_conditions, and wrapped_conditions
to verify that execute_filter throws DB::Exception with a message matching the
expected strict JSON parsing failure. Apply the same message matcher
consistently to each ASSERT_THROW-style check, preserving the existing condition
setup.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 30c06a7c-b05b-4054-86d2-600052930fc0
📒 Files selected for processing (6)
dbms/src/Flash/Coprocessor/DAGExpressionAnalyzer.cppdbms/src/Flash/Coprocessor/DAGExpressionAnalyzer.hdbms/src/Flash/Coprocessor/DAGExpressionAnalyzerHelper.cppdbms/src/Functions/FunctionsJson.hdbms/src/Functions/tests/gtest_json_valid.cpptests/fullstack-test/expr/json_valid.test
|
/cc @windtalker |
JaySon-Huang
left a comment
JaySon-Huang
left a comment
There was a problem hiding this comment.
lgtm
@windtalker PTAL
ti-chi-bot
Bot
added
approved
needs-1-more-lgtm
|
/test pull-unit-next-gen |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JaySon-Huang, windtalker The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
|
/cherry-pick release-nextgen-202603 |
|
@yongman: new pull request created to branch DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
4 participants
What problem does this PR solve?
Issue Number: close #11011
Problem Summary:
JSON_EXTRACT error even with JSON_VALID in conditions.
What is changed and how it works?
Check List
Tests
Side effects
Documentation
Release note
Summary by CodeRabbit
Bug Fixes
Tests