functions: save FieldType as value instead of ptr in json function by yongman · Pull Request #10846 · pingcap/tiflash

yongman · 2026-05-15T03:37:17Z

What problem does this PR solve?

Issue Number: close #10845

Problem Summary:

When TiFlash nextgen evaluates JSON_EXTRACT on a TEXT column with IS NULL / IS NOT NULL filters, the result can be inconsistent with JSON columns.

For example, JSON_EXTRACT(action_params, '$.popup_id') IS NULL may return rows whose extracted value is actually non-null, while IS NOT NULL returns no rows.

The root cause is that the disaggregated columnar path builds temporary FilterConditions, and JSON cast functions keep raw pointers to tipb::FieldType. After the temporary object is destroyed, those pointers can become dangling, so FunctionCastStringAsJson may read invalid FieldType metadata.

What is changed and how it works?

functions: save FieldType as value instead of ptr in json function

Store TiDB FieldType metadata by value in JSON cast functions instead of keeping raw pointers to caller-owned FieldType objects.

Use std::optional<tipb::FieldType> for optional FieldType metadata and update the missing-metadata checks accordingly in:
- FunctionCastJsonAsString
- FunctionCastIntAsJson
- FunctionCastStringAsJson
- FunctionCastTimeAsJson

This avoids dangling FieldType references when JSON cast functions are created from temporary filter conditions, and keeps TEXT-to-JSON cast behavior stable for pushed-down JSON_EXTRACT filters.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Manual test:

Use the SQL in #10845 to create event_log1 with action_params TEXT and event_log2 with action_params JSON, then run with:

SET SESSION tidb_isolation_read_engines='tiflash';

Verify that both TEXT and JSON columns return consistent results:

WHERE JSON_EXTRACT(action_params, '$.popup_id') IS NULL
-- returns 0 rows

WHERE JSON_EXTRACT(action_params, '$.popup_id') IS NOT NULL
-- returns 5 rows

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

None

Summary by CodeRabbit

Release Notes

Bug Fixes
- Fixed issue #10845: Corrected inconsistencies in JSON extraction and null filtering operations when using TiFlash. The fix ensures consistent behavior when working with various data types and null values. Added comprehensive regression tests to validate the corrections across multiple scenarios.
Refactor
- Refactored internal field type handling mechanisms in JSON casting functions for improved code stability and maintainability.

Signed-off-by: yongman <yming0221@gmail.com>

pantheon-ai · 2026-05-15T03:37:22Z

@yongman I've received your pull request and will start the review. I'll conduct a thorough review covering code quality, potential issues, and implementation details.

⏳ This process typically takes 10-30 minutes depending on the complexity of the changes.

_{ℹ️ Learn more details on Pantheon AI.}

coderabbitai · 2026-05-15T03:37:34Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: f3c2e079-4376-4724-8efc-25eacefa10d4

📥 Commits

Reviewing files that changed from the base of the PR and between db8d62f and 5e9d5ee.

📒 Files selected for processing (1)

dbms/src/Functions/FunctionsTiDBConversion.h

📝 Walkthrough

Walkthrough

This PR refactors TiDB field type storage in JSON casting functions from raw pointers to std::optional, updates reference-to-value field storage in TiDBConversion classes, and adds a fullstack regression test validating consistent JSON extraction behavior on TEXT versus JSON column types under TiFlash/MPP settings.

Changes

JSON FieldType Optional Refactoring and Regression Test

Layer / File(s)	Summary
Header include and FunctionCastJsonAsString refactor `dbms/src/Functions/FunctionsJson.h`	Add `<optional>` header and refactor `FunctionCastJsonAsString::tidb_tp` from `const tipb::FieldType *` to `std::optional<tipb::FieldType>`; update `setOutputTiDBFieldType` setter and execution condition to use `has_value()` and optional access.
FunctionCastIntAsJson optional refactor `dbms/src/Functions/FunctionsJson.h`	Refactor `FunctionCastIntAsJson::input_tidb_tp` to `std::optional<tipb::FieldType>`; update `setInputTiDBFieldType` and execution gating to check `has_value()` instead of `nullptr`.
FunctionCastStringAsJson dual optional refactor `dbms/src/Functions/FunctionsJson.h`	Refactor `FunctionCastStringAsJson::input_tidb_tp` and `output_tidb_tp` to `std::optional<tipb::FieldType>`; update dual setters and execution branches to safely check and dereference optionals.
FunctionCastTimeAsJson optional refactor `dbms/src/Functions/FunctionsJson.h`	Refactor `FunctionCastTimeAsJson::input_tidb_tp` to `std::optional<tipb::FieldType>`; update setter and timestamp detection to use optional presence checks.
TiDBConversion field storage refactor `dbms/src/Functions/FunctionsTiDBConversion.h`	Change `tidb_tp` from `const tipb::FieldType &` to `tipb::FieldType` value in `ExecutableFunctionTiDBCast` and `FunctionTiDBCast` classes.
Fullstack regression test for issue `#10845` `tests/fullstack-test/expr/cast_as_json_issue10845.test`	Add end-to-end test validating `json_extract` and `json_unquote` behavior consistency between TEXT and JSON column types under TiFlash/MPP; verify null/non-null filtering and extracted value ordering.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 Pointers fade to optionals bright,
Where nullptrs once cast their blight,
Now optional hugs the TiDB way,
JSON casting works both night and day,
Text and JSON dance in sync—a delight!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 6.25% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The pull request title clearly and specifically summarizes the main change: storing FieldType as a value instead of a pointer in JSON functions.
Description check	✅ Passed	The pull request description follows the template with all critical sections completed: problem statement references issue `#10845`, a detailed commit message explains the changes, manual test steps are provided, and checklist items are properly marked.
Linked Issues check	✅ Passed	The code changes directly address issue `#10845` by storing FieldType metadata by value (std::optional) instead of raw pointers in FunctionCastJsonAsString, FunctionCastIntAsJson, FunctionCastStringAsJson, and FunctionCastTimeAsJson, eliminating dangling references in disaggregated columnar paths.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to fixing the FieldType lifetime issue: modifications to four JSON cast functions in FunctionsJson.h, a reference type update in FunctionsTiDBConversion.h, and a regression test for issue `#10845`. No unrelated changes detected.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

🧹 Nitpick comments (1)

dbms/src/Functions/FunctionsJson.h (1)
439-439: 💤 Low value

Refactoring from pointer to value changes ownership semantics.

The change from const tipb::FieldType* to std::optional<tipb::FieldType> is semantically significant: the function now owns a copy of the FieldType rather than holding a reference to external data. This eliminates potential lifetime issues (dangling pointers), which likely addresses the consistency bug mentioned in issue #10845.

The setter copies tipb::FieldType on each call. If tipb::FieldType (a protobuf message) is large, consider adding a move-enabled overload:
void setOutputTiDBFieldType(tipb::FieldType tidb_tp_) { tidb_tp = std::move(tidb_tp_); }
However, the current implementation is correct, and the copy overhead may be acceptable.

Also applies to: 467-467, 530-530
🤖 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` at line 439, The setter currently copies
a potentially large protobuf (setOutputTiDBFieldType) which can be expensive;
add a move-enabled overload that takes tipb::FieldType by value (or an rvalue
ref) and moves it into the std::optional member (tidb_tp) to avoid the extra
copy, and apply the same change to the other setters flagged in this file (the
other setOutputTiDBFieldType occurrences referenced in the comment).

🤖 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.

Nitpick comments:
In `@dbms/src/Functions/FunctionsJson.h`:
- Line 439: The setter currently copies a potentially large protobuf
(setOutputTiDBFieldType) which can be expensive; add a move-enabled overload
that takes tipb::FieldType by value (or an rvalue ref) and moves it into the
std::optional member (tidb_tp) to avoid the extra copy, and apply the same
change to the other setters flagged in this file (the other
setOutputTiDBFieldType occurrences referenced in the comment).

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: aa141322-ab1a-4623-ab1a-810339dd1046

📥 Commits

Reviewing files that changed from the base of the PR and between ed4e382 and e74426d.

📒 Files selected for processing (1)

dbms/src/Functions/FunctionsJson.h

JaySon-Huang

Verified the fixed in the tiflash-cse columnar branch.

LGTM

JaySon-Huang · 2026-05-15T05:54:56Z

/cc @windtalker @gengliqi

windtalker · 2026-05-21T07:48:13Z

I don't understand why "disaggregated columnar path builds temporary FilterConditions"

yongman · 2026-05-21T14:30:35Z

I don't understand why "disaggregated columnar path builds temporary FilterConditions"

@windtalker The related logic refer to

tiflash/dbms/src/Storages/StorageDisaggregatedRemote.cpp

Lines 126 to 160 in 6f17860

    
           void StorageDisaggregated::filterConditionsWithPushedDownFilters( 
        
               DAGExpressionAnalyzer & analyzer, 
        
               DAGPipeline & pipeline) 
        
           { 
        
           #if ENABLE_NEXT_GEN_COLUMNAR == 0 
        
               filterConditions(analyzer, pipeline); 
        
           #else 
        
               FilterConditions conditions(filter_conditions.executor_id, filter_conditions.conditions); 
        
               conditions.conditions.MergeFrom(table_scan.getPushedDownFilters()); 
        
               if (conditions.hasValue()) 
        
               { 
        
                   ::DB::executePushedDownFilter(conditions, analyzer, log, pipeline); 
        
                   auto & profile_streams = context.getDAGContext()->getProfileStreamsMap()[conditions.executor_id]; 
        
                   pipeline.transform([&profile_streams](auto & stream) { profile_streams.push_back(stream); }); 
        
               } 
        
           #endif 
        
           } 
        
           void StorageDisaggregated::filterConditionsWithPushedDownFilters( 
        
               PipelineExecutorContext & exec_context, 
        
               PipelineExecGroupBuilder & group_builder, 
        
               DAGExpressionAnalyzer & analyzer) 
        
           { 
        
           #if ENABLE_NEXT_GEN_COLUMNAR == 0 
        
               filterConditions(exec_context, group_builder, analyzer); 
        
           #else 
        
               FilterConditions conditions(filter_conditions.executor_id, filter_conditions.conditions); 
        
               conditions.conditions.MergeFrom(table_scan.getPushedDownFilters()); 
        
               if (conditions.hasValue()) 
        
               { 
        
                   ::DB::executePushedDownFilter(exec_context, group_builder, conditions, analyzer, log); 
        
                   context.getDAGContext()->addOperatorProfileInfos(conditions.executor_id, group_builder.getCurProfileInfos()); 
        
               } 
        
           #endif 
        
           }

windtalker · 2026-05-22T05:52:38Z

I don't understand why "disaggregated columnar path builds temporary FilterConditions"

@windtalker The related logic refer to

tiflash/dbms/src/Storages/StorageDisaggregatedRemote.cpp

Lines 126 to 160 in 6f17860

void StorageDisaggregated::filterConditionsWithPushedDownFilters(

DAGExpressionAnalyzer & analyzer,

DAGPipeline & pipeline)

{

#if ENABLE_NEXT_GEN_COLUMNAR == 0

filterConditions(analyzer, pipeline);

#else

FilterConditions conditions(filter_conditions.executor_id, filter_conditions.conditions);

conditions.conditions.MergeFrom(table_scan.getPushedDownFilters());

if (conditions.hasValue())

{

::DB::executePushedDownFilter(conditions, analyzer, log, pipeline);

auto & profile_streams = context.getDAGContext()->getProfileStreamsMap()[conditions.executor_id];

pipeline.transform([&profile_streams](auto & stream) { profile_streams.push_back(stream); });

}

#endif

}

void StorageDisaggregated::filterConditionsWithPushedDownFilters(

PipelineExecutorContext & exec_context,

PipelineExecGroupBuilder & group_builder,

DAGExpressionAnalyzer & analyzer)

{

#if ENABLE_NEXT_GEN_COLUMNAR == 0

filterConditions(exec_context, group_builder, analyzer);

#else

FilterConditions conditions(filter_conditions.executor_id, filter_conditions.conditions);

conditions.conditions.MergeFrom(table_scan.getPushedDownFilters());

if (conditions.hasValue())

{

::DB::executePushedDownFilter(exec_context, group_builder, conditions, analyzer, log);

context.getDAGContext()->addOperatorProfileInfos(conditions.executor_id, group_builder.getCurProfileInfos());

}

#endif

}

can you give some explainations about why disaggregated columnar path is so special?

Signed-off-by: JaySon-Huang <tshent@qq.com>

JaySon-Huang · 2026-05-26T07:51:40Z

@windtalker @yongman

The disaggregated columnar path is special because the columnar reader and the classic StorageDeltaMerge path have different contracts for late-materialization filters.

For a TiDB table scan, pushed_down_filter_conditions are the filters pushed down by late materialization and are expected to be executed in the storage layer:

/// pushed_down_filter_conditions is the filter conditions that are
/// pushed down to table scan by late materialization.
/// They will be executed on Storage layer.

In the classic StorageDeltaMerge path, these filters are passed through DAGQueryInfo and then into DM::PushDownExecutor::build(...). PushDownExecutor builds the filter expression actions from pushed_down_filters, so this path treats them as the storage-layer filter executor.

The disaggregated columnar reader is different. It receives the table scan protobuf with pushed_down_filter_conditions, but the current columnar reader contract is weaker. The code comment in StorageDisaggregatedColumnar.cpp says:

// Copy pushed down filters to filter_conditions to make filterConditions works properly.
// Proxy columnar reader use pushed down filters to reduce packs load from disk and has no
// guarantee to filter all useless data, so we rely on the filterConditions to filter data.

So, for the columnar path, table_scan.getPushedDownFilters() can be used by the proxy columnar reader for pruning / reducing pack reads, but it is not sufficient for correctness because it may still return rows that do not satisfy those filters. TiFlash therefore has to re-apply those late-materialization filters in the local pipeline.

That is why filterConditionsWithPushedDownFilters() creates a temporary FilterConditions, merges table_scan.getPushedDownFilters() into the normal selection conditions, and calls executePushedDownFilter(...) after the columnar reader returns data. This temporary merged FilterConditions is also why the dangling tipb::FieldType* issue can be exposed in this path: JSON cast functions may be built from expressions owned by that temporary object.

Code references

dbms/src/Flash/Coprocessor/TiDBTableScan.h:68: documents that pushed_down_filter_conditions come from late materialization and should be executed on the storage layer.
dbms/src/Storages/StorageDeltaMerge.cpp:878 and dbms/src/Storages/StorageDeltaMerge.cpp:972: classic StorageDeltaMerge builds a PushDownExecutor from the query info.
dbms/src/Storages/DeltaMerge/Filter/PushDownExecutor.cpp:226: PushDownExecutor::build(query_info, ...) reads dag_query->pushed_down_filters.
dbms/src/Storages/DeltaMerge/Filter/PushDownExecutor.cpp:149: PushDownExecutor builds filter expression actions from pushed_down_filters.
dbms/src/Storages/StorageDisaggregatedColumnar.cpp:348: columnar comment states that the proxy columnar reader uses pushed-down filters to reduce pack loads but does not guarantee filtering all useless data.
dbms/src/Storages/StorageDisaggregatedRemote.cpp:133 and dbms/src/Storages/StorageDisaggregatedRemote.cpp:152: columnar-enabled filterConditionsWithPushedDownFilters() merges table_scan.getPushedDownFilters() into a temporary FilterConditions and executes it in TiFlash.

Signed-off-by: JaySon-Huang <tshent@qq.com>

JaySon-Huang · 2026-05-26T09:05:05Z

/test pull-unit-test

JaySon-Huang · 2026-05-27T02:17:03Z

/test pull-unit-test

JaySon-Huang · 2026-05-27T03:08:56Z

@windtalker PTAL again

windtalker · 2026-05-27T08:18:50Z

Besides the json function, there are some other functions that still hold the reference of FieldType, I think you should update those functions as well

JaySon-Huang · 2026-06-08T07:28:00Z

@windtalker Thanks for the reminder.

I checked the other functions under dbms/src/Functions. Besides the JSON functions, the only similar case I found is tidb_cast: FunctionTiDBCast and ExecutableFunctionTiDBCast kept tipb::FieldType as member references in FunctionsTiDBConversion.h.

I updated both of them to own a tipb::FieldType value instead. The builder already copies the input FieldType, but this change also makes the built FunctionBase / ExecutableFunction self-contained and removes the dependency on the builder lifetime.

The remaining const tipb::FieldType & usages I found are function parameters passed synchronously, not stored as object state.

windtalker

lgtm

ti-chi-bot · 2026-06-08T13:00:15Z

[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

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [JaySon-Huang,windtalker]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ti-chi-bot · 2026-06-08T13:00:19Z

[LGTM Timeline notifier]

Timeline:

2026-05-15 05:48:16.988262699 +0000 UTC m=+417465.521042028: ☑️ agreed by JaySon-Huang.
2026-06-08 13:00:18.153770281 +0000 UTC m=+792119.224087661: ☑️ agreed by windtalker.

…ingcap#10846) close pingcap#10845\n\nfunctions: save FieldType as value instead of ptr in json function Store TiDB FieldType metadata by value in JSON cast functions instead of keeping raw pointers to caller-owned FieldType objects. Use std::optional<tipb::FieldType> for optional FieldType metadata and update the missing-metadata checks accordingly in: - FunctionCastJsonAsString - FunctionCastIntAsJson - FunctionCastStringAsJson - FunctionCastTimeAsJson This avoids dangling FieldType references when JSON cast functions are created from temporary filter conditions, and keeps TEXT-to-JSON cast behavior stable for pushed-down JSON_EXTRACT filters.\n\nSigned-off-by: JaySon-Huang <tshent@qq.com>\n\nCo-authored-by: JaySon-Huang <tshent@qq.com>

functions: save FieldType as value instead of ptr in json function

e74426d

Signed-off-by: yongman <yming0221@gmail.com>

ti-chi-bot Bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/needs-triage-completed labels May 15, 2026

ti-chi-bot Bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 15, 2026

coderabbitai Bot reviewed May 15, 2026

View reviewed changes

JaySon-Huang approved these changes May 15, 2026

View reviewed changes

ti-chi-bot Bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels May 15, 2026

ti-chi-bot Bot requested review from gengliqi and windtalker May 15, 2026 05:54

JaySon-Huang added 2 commits May 26, 2026 15:20

Merge remote-tracking branch 'upstream/master' into ym/fix-json-function

99df179

tests: add test case for 10845

7378edc

Signed-off-by: JaySon-Huang <tshent@qq.com>

ti-chi-bot Bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 26, 2026

Only define filterConditionsWithPushedDownFilters with columnar enabled

db8d62f

Signed-off-by: JaySon-Huang <tshent@qq.com>

trigger ci

bd8f8b2

JaySon-Huang added 2 commits June 8, 2026 15:15

Merge remote-tracking branch 'upstream/master' into ym/fix-json-function

6059eff

Fix TiDB cast field type lifetime

5e9d5ee

windtalker reviewed Jun 8, 2026

View reviewed changes

Comment thread dbms/src/Functions/FunctionsJson.h

windtalker approved these changes Jun 8, 2026

View reviewed changes

ti-chi-bot Bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jun 8, 2026

ti-chi-bot Bot merged commit 5e47cac into pingcap:master Jun 8, 2026
10 checks passed

yongman deleted the ym/fix-json-function branch June 8, 2026 13:56

Conversation

yongman commented May 15, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

Summary by CodeRabbit

Release Notes

Uh oh!

pantheon-ai Bot commented May 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented May 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Poem

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

JaySon-Huang left a comment

Choose a reason for hiding this comment

Uh oh!

JaySon-Huang commented May 15, 2026

Uh oh!

windtalker commented May 21, 2026

Uh oh!

yongman commented May 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

windtalker commented May 22, 2026

Uh oh!

JaySon-Huang commented May 26, 2026

Code references

Uh oh!

JaySon-Huang commented May 26, 2026

Uh oh!

JaySon-Huang commented May 27, 2026

Uh oh!

JaySon-Huang commented May 27, 2026

Uh oh!

windtalker commented May 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JaySon-Huang commented Jun 8, 2026

Uh oh!

Uh oh!

windtalker left a comment

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot Bot commented Jun 8, 2026

Uh oh!

ti-chi-bot Bot commented Jun 8, 2026

[LGTM Timeline notifier]

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

yongman commented May 15, 2026 •

edited by coderabbitai Bot

Loading

pantheon-ai Bot commented May 15, 2026 •

edited

Loading

coderabbitai Bot commented May 15, 2026 •

edited

Loading

yongman commented May 21, 2026 •

edited

Loading

windtalker commented May 27, 2026 •

edited

Loading