Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

expression: add builtin function json_valid #12596

Merged
merged 17 commits into from
Oct 15, 2019
Merged

Conversation

Reminiscent
Copy link
Contributor

What problem does this PR solve?

Add json_valid builtin function described #7546.

What is changed and how it works?

See here for details.

Check List

Tests

  • Unit test

@Reminiscent Reminiscent changed the title expression: add builtin function json_valid expression: add builtin function json_valid Oct 10, 2019
Copy link
Contributor

@SunRunAway SunRunAway left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
Could you add some integration test and mysql-test?

expression/builtin_json.go Outdated Show resolved Hide resolved
expression/builtin_json.go Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 12, 2019

Codecov Report

Merging #12596 into master will decrease coverage by 0.0001%.
The diff coverage is 47.0588%.

@@               Coverage Diff                @@
##             master     #12596        +/-   ##
================================================
- Coverage   79.8579%   79.8577%   -0.0002%     
================================================
  Files           462        462                
  Lines        104865     104884        +19     
================================================
+ Hits          83743      83758        +15     
- Misses        14920      14930        +10     
+ Partials       6202       6196         -6

@Reminiscent
Copy link
Contributor Author

@SunRunAway @zz-jason @qw4990 PTAL

Copy link
Contributor

@SunRunAway SunRunAway left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@SunRunAway SunRunAway added the status/LGT1 Indicates that a PR has LGTM 1. label Oct 14, 2019
Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after offline discussion with @Reminiscent, we decided to use 3 signatures to implement this function:

  1. json_valid for json input parameters
  2. json_valid for string input parameters
  3. json_valid for other type input parameters

@qw4990
Copy link
Contributor

qw4990 commented Oct 15, 2019

/run-all-tests

@qw4990 qw4990 merged commit 57327d1 into pingcap:master Oct 15, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Oct 15, 2019

cherry pick to release-2.1 failed

@sre-bot
Copy link
Contributor

sre-bot commented Oct 15, 2019

cherry pick to release-3.1 failed

@sre-bot
Copy link
Contributor

sre-bot commented Oct 15, 2019

cherry pick to release-3.0 failed

@sre-bot
Copy link
Contributor

sre-bot commented Apr 7, 2020

It seems that, not for sure, we failed to cherry-pick this commit to release-2.1 release-3.0. Please comment '/run-cherry-picker' to try to trigger the cherry-picker if we did fail to cherry-pick this commit before. @Reminiscent PTAL.

@Reminiscent
Copy link
Contributor Author

/run-cherry-picker

sre-bot pushed a commit to sre-bot/tidb that referenced this pull request Apr 21, 2020
Signed-off-by: sre-bot <sre-bot@pingcap.com>
@sre-bot

This comment has been minimized.

@Reminiscent
Copy link
Contributor Author

cherry-pick to release-3.0 in PR #13133
cherry-pick to release-3.1 in PR #13138

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/expression status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants