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

Json unquote extract pushdown #6377

Merged
merged 33 commits into from
Dec 2, 2022

Conversation

yibin87
Copy link
Contributor

@yibin87 yibin87 commented Nov 28, 2022

What problem does this PR solve?

Issue Number: close #6376

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

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

Signed-off-by: yibin <huyibin@pingcap.com>
Signed-off-by: yibin <huyibin@pingcap.com>
Signed-off-by: yibin <huyibin@pingcap.com>
Signed-off-by: yibin <huyibin@pingcap.com>
Signed-off-by: yibin <huyibin@pingcap.com>
Signed-off-by: yibin <huyibin@pingcap.com>
Signed-off-by: yibin <huyibin@pingcap.com>
Signed-off-by: yibin <huyibin@pingcap.com>
Signed-off-by: yibin <huyibin@pingcap.com>
Signed-off-by: yibin <huyibin@pingcap.com>
Signed-off-by: yibin <huyibin@pingcap.com>
@ti-chi-bot
Copy link
Member

ti-chi-bot commented Nov 28, 2022

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • mengxin9014
  • solotzg

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

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

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@ti-chi-bot
Copy link
Member

@yibin87: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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 kubernetes/test-infra repository.

@ti-chi-bot ti-chi-bot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 28, 2022
Signed-off-by: yibin <huyibin@pingcap.com>
Signed-off-by: yibin <huyibin@pingcap.com>
@yibin87 yibin87 removed the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Nov 29, 2022
Signed-off-by: yibin <huyibin@pingcap.com>
@yibin87
Copy link
Contributor Author

yibin87 commented Nov 29, 2022

/run-all-tests

Signed-off-by: yibin <huyibin@pingcap.com>
@yibin87
Copy link
Contributor Author

yibin87 commented Nov 29, 2022

/run-all-tests

Signed-off-by: yibin <huyibin@pingcap.com>
@yibin87
Copy link
Contributor Author

yibin87 commented Dec 1, 2022

/run-all-tests

Copy link
Contributor

@mengxin9014 mengxin9014 left a comment

Choose a reason for hiding this comment

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

Others LGTM

dbms/src/Flash/Coprocessor/TiDBTime.h Show resolved Hide resolved
Comment on lines 32 to 33
static const UInt32 UNICODEMax = 0x0010FFFF; // Maximum valid Unicode code point.
static const UInt32 UTF8Error = UNICODEMax + 1; // the "error" code
Copy link
Contributor

Choose a reason for hiding this comment

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

UNICODEMax -> UNICODE_MAX
UTF8Error -> UTF8_ERROR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice, changed it.

Copy link
Contributor

Choose a reason for hiding this comment

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

still UNICODE_Max

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I didn't notice the upper case, update it now.

Signed-off-by: yibin <huyibin@pingcap.com>
dbms/src/IO/Endian.h Show resolved Hide resolved
dbms/src/IO/Endian.h Show resolved Hide resolved
@solotzg
Copy link
Contributor

solotzg commented Dec 2, 2022

Is there any code coverage report about your modification ?

@yibin87
Copy link
Contributor Author

yibin87 commented Dec 2, 2022

/run-unit-test

@yibin87
Copy link
Contributor Author

yibin87 commented Dec 2, 2022

Is there any code coverage report about your modification ?

I'll run tests to see.

@yibin87
Copy link
Contributor Author

yibin87 commented Dec 2, 2022

/run-all-tests

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 2, 2022
@ti-chi-bot ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Dec 2, 2022
@yibin87
Copy link
Contributor Author

yibin87 commented Dec 2, 2022

/merge

@ti-chi-bot
Copy link
Member

@yibin87: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

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.

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: f18245a

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Dec 2, 2022
@yibin87
Copy link
Contributor Author

yibin87 commented Dec 2, 2022

/re-build

@yibin87
Copy link
Contributor Author

yibin87 commented Dec 2, 2022

/re-builds

@yibin87
Copy link
Contributor Author

yibin87 commented Dec 2, 2022

/rebuild

@wuhuizuo
Copy link
Contributor

wuhuizuo commented Dec 2, 2022

/run-unit-test
/run-integration-test

@yibin87 yibin87 merged commit 4bae244 into pingcap:master Dec 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support 'json_extract', '->', '->>' json functions in TiFlash
6 participants