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
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
the
size/L
|
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 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 |
2 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