support `json_valid` and `json_keys` #8491

SeaRise · 2023-12-11T03:47:24Z

What problem does this PR solve?

Issue Number: close #8490

Problem Summary:

What is changed and how it works?

Check List

Tests

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

Side effects

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

Documentation

Release note

None

SeaRise · 2023-12-11T10:24:56Z

/run-all-tests

dbms/src/Functions/FunctionsJson.h

windtalker · 2023-12-13T07:07:00Z

dbms/src/Functions/FunctionsJson.h

+                            {
+                                auto cur_offset = tmp_buffer.offset();
+                                JsonBinary::appendStringRef(tmp_buffer, key);
+                                auto after_offset = tmp_buffer.offset();


I‘m a little confused that looks like key is already a StringRef/string_view based on the real data in data_from, why here need to deep copy it to tmp_buffer and construct new StringRef based on tmp_buffer?

ok..
add a new function buildKeyArrayInBuffer for std::vector<std::string_view> keys

windtalker · 2023-12-13T07:13:12Z

dbms/src/Functions/FunctionsJson.h

+            ColumnUInt8::Container & vec_null_map = col_null_map->getData();
+
+            {
+                JsonBinary::JsonBinaryWriteBuffer write_buffer(data_to, data_from.size());


I'm a little worried that it will reserve too many memories for the result.

Maybe we can use two loops, first is to extract the keys from orignal json column, then use another loop to do the copy? The intermediate result should not be too large if we keep the key_binaries as StringRef refering the orignal data in data_from?

This could be inconvenient, especially when dealing with nullmap.
Typically, keys are very small, how about automatically resize instead of reserve?
https://github.com/pingcap/tiflash/pull/8491/files#diff-52627c7f14d286918db163406a85acac41a1ad79c22184768379ebe163c1131aR1664-R1665

yibin87

Others LGTM

yibin87 · 2023-12-15T02:04:34Z

dbms/src/Functions/FunctionsJson.h

+            throw Exception(
+                fmt::format("Illegal json path expression of function {}", getName()),
+                ErrorCodes::ILLEGAL_COLUMN);
+        auto path_expr_containor = std::make_unique<JsonPathExprRefContainer>(path_expr);


typo here: path_expr_container

ok, updated.

yibin87 · 2023-12-15T02:32:44Z

dbms/src/Functions/FunctionsJson.h

+                throw Exception(
+                    fmt::format("Illegal json path expression of function {}", getName()),
+                    ErrorCodes::ILLEGAL_COLUMN);
+            auto path_expr_containor = std::make_unique<JsonPathExprRefContainer>(path_expr);


typo, path_expr_container

ok, updated.

yibin87 · 2023-12-15T02:34:36Z

dbms/src/Functions/FunctionsJson.h

+            std::vector<JsonPathExprRefContainerPtr> path_expr_containor_vec(1);
+            path_expr_containor_vec[0] = std::move(path_expr_containor);
+
+            const auto & json_val = json_source->getWhole();


If actual extract codes can be extracted as a common function, it would look better.

good point, updated.

windtalker · 2023-12-18T02:29:49Z

dbms/src/TiDB/Decode/JsonBinary.cpp

@@ -389,12 +391,12 @@ JsonBinary JsonBinary::getArrayElement(size_t index) const
    return getValueEntry(HEADER_SIZE + index * VALUE_ENTRY_SIZE);
 }

-String JsonBinary::getObjectKey(size_t index) const
+std::string_view JsonBinary::getObjectKey(size_t index) const


Since JsonBinary already use StringRef to represent the string reference, and there is no significant difference between StringRef and string_view, why not use StringRef here?

ok, used StringRef instead.

windtalker · 2023-12-18T03:04:37Z

dbms/src/Functions/FunctionsJson.h

+                    offsets_to,
+                    vec_null_map);
+            }
+            else if (path_col->isColumnConst())


If json_col is constant and path_col is not constant, will doExecuteCommon handle it correctly?

yes, ut added here.
https://github.com/pingcap/tiflash/pull/8491/files#diff-f4a535f55a6fc34ac53f3aeaeb0d5df7f0a7a57812bcfb062bf9aad74eb4984aR98-R107

SeaRise · 2023-12-18T04:23:40Z

/run-all-tests

SeaRise · 2023-12-18T06:04:30Z

/run-all-tests

SeaRise · 2023-12-19T02:29:35Z

/run-all-tests

dbms/src/Functions/FunctionsJson.h

windtalker

LGTM

ti-chi-bot · 2023-12-19T03:30:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: windtalker, yibin87

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [windtalker,yibin87]

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 · 2023-12-19T03:30:54Z

[LGTM Timeline notifier]

Timeline:

2023-12-15 03:02:32.037417746 +0000 UTC m=+584443.074644668: ☑️ agreed by yibin87.
2023-12-19 03:30:53.957174791 +0000 UTC m=+931744.994401719: ☑️ agreed by windtalker.

json_valid

f0a37ea

ti-chi-bot bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 11, 2023

SeaRise added 2 commits December 11, 2023 12:19

json_keys

9dd537f

tmp save

a69077a

ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 11, 2023

json_keys_2_args

9d4671d

ti-chi-bot bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 11, 2023

add ut for json_valid

cf7c9df

ti-chi-bot bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 11, 2023

Merge branch 'master' into support_more_json_functions

728cc5d

ti-chi-bot bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 11, 2023

SeaRise added 6 commits December 11, 2023 16:45

fmt

e1b9965

ad ut

c5db596

add ut

449918e

more comment

401737d

add ft

0d3920d

add ut

c0f6d3b

SeaRise changed the title ~~WIP: support json_valid and json_keys~~ support json_valid and json_keys Dec 11, 2023

ti-chi-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 11, 2023

SeaRise commented Dec 11, 2023

View reviewed changes

dbms/src/Functions/FunctionsJson.h Outdated Show resolved Hide resolved

dbms/src/Functions/FunctionsJson.h Outdated Show resolved Hide resolved

dbms/src/Functions/FunctionsJson.h Outdated Show resolved Hide resolved

dbms/src/Functions/FunctionsJson.h Outdated Show resolved Hide resolved

SeaRise added 5 commits December 11, 2023 18:42

Update dbms/src/Functions/FunctionsJson.h

104920f

Update dbms/src/Functions/FunctionsJson.h

8ca7d32

Update dbms/src/Functions/FunctionsJson.h

1cb3180

Update dbms/src/Functions/FunctionsJson.h

0b9cad8

Merge branch 'master' into support_more_json_functions

e5044e4

SeaRise mentioned this pull request Dec 12, 2023

expression: Add json_valid and json_keys push down support for tiflash pingcap/tidb#49347

Merged

13 tasks

SeaRise commented Dec 12, 2023

View reviewed changes

dbms/src/Functions/FunctionsJson.h Show resolved Hide resolved

SeaRise commented Dec 12, 2023

View reviewed changes

dbms/src/Functions/FunctionsJson.h Outdated Show resolved Hide resolved

dbms/src/Functions/FunctionsJson.h Outdated Show resolved Hide resolved

windtalker reviewed Dec 13, 2023

View reviewed changes

address comment and refine

7648ecf

SeaRise force-pushed the support_more_json_functions branch from eea1e6e to 7648ecf Compare December 13, 2023 09:19

SeaRise requested a review from windtalker December 13, 2023 09:25

yibin87 approved these changes Dec 15, 2023

View reviewed changes

ti-chi-bot bot added needs-1-more-lgtm approved labels Dec 15, 2023

SeaRise added 2 commits December 18, 2023 09:00

typo

52f3db2

extract code

d53de9d

windtalker reviewed Dec 18, 2023

View reviewed changes

SeaRise added 2 commits December 18, 2023 12:10

add ut for json_keys(const, column)

8619764

use StringRef

aff767e

SeaRise requested a review from windtalker December 18, 2023 04:17

Merge branch 'master' into support_more_json_functions

3d10f81

Merge branch 'master' into support_more_json_functions

02d8110

windtalker reviewed Dec 19, 2023

View reviewed changes

dbms/src/Functions/FunctionsJson.h Show resolved Hide resolved

SeaRise requested a review from windtalker December 19, 2023 03:21

windtalker approved these changes Dec 19, 2023

View reviewed changes

ti-chi-bot bot added the lgtm label Dec 19, 2023

ti-chi-bot bot removed the needs-1-more-lgtm label Dec 19, 2023

ti-chi-bot bot merged commit 8a19c86 into pingcap:master Dec 19, 2023
6 checks passed

SeaRise deleted the support_more_json_functions branch December 19, 2023 03:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support `json_valid` and `json_keys` #8491

support `json_valid` and `json_keys` #8491

SeaRise commented Dec 11, 2023 •

edited

SeaRise commented Dec 11, 2023

windtalker Dec 13, 2023

SeaRise Dec 13, 2023

windtalker Dec 13, 2023

windtalker Dec 13, 2023

SeaRise Dec 13, 2023

yibin87 left a comment

yibin87 Dec 15, 2023

SeaRise Dec 18, 2023

yibin87 Dec 15, 2023

SeaRise Dec 18, 2023

yibin87 Dec 15, 2023

SeaRise Dec 18, 2023

windtalker Dec 18, 2023

SeaRise Dec 18, 2023

windtalker Dec 18, 2023

SeaRise Dec 18, 2023

SeaRise commented Dec 18, 2023

SeaRise commented Dec 18, 2023

SeaRise commented Dec 19, 2023

windtalker left a comment

ti-chi-bot bot commented Dec 19, 2023

ti-chi-bot bot commented Dec 19, 2023

support json_valid and json_keys #8491

support json_valid and json_keys #8491

Conversation

SeaRise commented Dec 11, 2023 • edited

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

SeaRise commented Dec 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yibin87 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeaRise commented Dec 18, 2023

SeaRise commented Dec 18, 2023

SeaRise commented Dec 19, 2023

windtalker left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Dec 19, 2023

ti-chi-bot bot commented Dec 19, 2023

[LGTM Timeline notifier]

support `json_valid` and `json_keys` #8491

support `json_valid` and `json_keys` #8491

SeaRise commented Dec 11, 2023 •

edited