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

implement builtin_string/instr_binary #4340

Merged
merged 1 commit into from Mar 11, 2019

Conversation

TigerInYourDream
Copy link
Contributor

Signed-off-by: TigerInYourDreams TigerInYourDream@hi.zy

What have you changed? (mandatory)

Implement the function instr_binary which I found in #3275
Please explain IN DETAIL what the changes are in this PR and why they are needed:
Add instr_binary function.

What are the type of the changes? (mandatory)

Add function。
The currently defined types are listed below, please pick one of the types for this PR by removing the others:

  • New feature (change which adds functionality)

How has this PR been tested? (mandatory)

Unit Test。

Does this PR affect documentation (docs) update? (mandatory)

No.

Does this PR affect tidb-ansible update? (mandatory)

No

Signed-off-by: TigerInYourDreams <TigerInYourDream@hi.zy>
@sre-bot
Copy link
Contributor

sre-bot commented Mar 10, 2019

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

@zhangjinpeng87 zhangjinpeng87 added sig/coprocessor SIG: Coprocessor contribution Type: PR - From contributors labels Mar 10, 2019
Copy link
Member

@zhangjinpeng87 zhangjinpeng87 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@ice1000 ice1000 left a comment

Choose a reason for hiding this comment

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

But LGTM anyway

match s.find(&substr) {
Some(x) => Ok(Some(1 + s[..x].chars().count() as i64)),
None => Ok(Some(0)),
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can extract the Ok(Some( part:

Ok(Some(match s.find(&substr) {
   Some(x) => 1 + s[..x].chars().count() as i64,
   None => 0,
}))

Copy link
Member

Choose a reason for hiding this comment

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

I think no necessary.

@zhangjinpeng87 zhangjinpeng87 merged commit 5a296bc into tikv:master Mar 11, 2019
@TigerInYourDream TigerInYourDream deleted the tid-instr_binary branch March 11, 2019 02:13
("F", "abcdefg", 0),
("cd", "abcdefg", 3),
(" ", "abcdefg", 0),
("", "", 0),
Copy link
Member

Choose a reason for hiding this comment

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

the result is 1 in mysql.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AndreMouche Thanks for the review.The same problem in #4337 .

AndreMouche added a commit that referenced this pull request Mar 11, 2019
row: &'a [Datum],
) -> Result<Option<i64>> {
let s = try_opt!(self.children[0].eval_string_and_decode(ctx, row));
let substr = try_opt!(self.children[1].eval_string_and_decode(ctx, row));
Copy link
Member

Choose a reason for hiding this comment

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

I think we needn't decode it since it is a binary.

@AndreMouche
Copy link
Member

@TigerInYourDream Would you like to fix the bug?

@TigerInYourDream
Copy link
Contributor Author

@TigerInYourDream Would you like to fix the bug?

@AndreMouche My bad.I'll create new pr to fix it. Thanks for your help.

sticnarf pushed a commit to sticnarf/tikv that referenced this pull request Oct 27, 2019
Signed-off-by: TigerInYourDreams <TigerInYourDream@hi.zy>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution Type: PR - From contributors sig/coprocessor SIG: Coprocessor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants