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

builtin: add instr built-in function #2857

Merged
merged 8 commits into from
Mar 22, 2017
Merged

Conversation

cosmtrek
Copy link
Contributor

Add INSTR(str, substr) built-in function

Copy link
Contributor

@XuHuaiyu XuHuaiyu left a comment

Choose a reason for hiding this comment

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

Thanks for your PR ^_^
Rest LGTM

return d, errors.Trace(err)
}
// INSTR(str, substr)
if args[0].IsNull() {
Copy link
Contributor

Choose a reason for hiding this comment

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

if args[0].IsNull() || args[1].IsNull(){
    return
}

{[]interface{}{123456, 567}, 0},
{[]interface{}{1e10, 1e2}, 1},
{[]interface{}{1.234, ".234"}, 2},

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a test case like SELECT INSTR('foobarbar', "'');.

return d, errors.Trace(err)
}
pos := strings.Index(str, substr) + 1 // index starts from 1
d.SetInt64(int64(pos))
Copy link
Contributor

Choose a reason for hiding this comment

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

works wrong with Chinese characters.
Try INSTR("你好世界", "世界")

@XuHuaiyu
Copy link
Contributor

@cosmtrek
If possible, please check and register the builtin function you decide to implement in issue to let others know, thank you!

@cosmtrek
Copy link
Contributor Author

cosmtrek commented Mar 17, 2017

@XuHuaiyu @zimulala PTAL

First thanks @spongedu

I've just read the INSTR() carefully and realized another mistake except for the problem of ignoring utf8 specified by @spongedu

This function is multibyte safe, and is case sensitive only if at least one argument is a binary string.

We need to consider the type of arguments passed to INSTR() if binary or not. If two arguments are string, substring search should be case-insensitive, otherwise we perform case sensitive search.

Unfortunately, the implementation of LOCATE() function in master branch made this mistake as well...

@zimulala zimulala added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 20, 2017
@coocood coocood added the contribution This PR is from a community contributor. label Mar 20, 2017
Copy link
Contributor

@XuHuaiyu XuHuaiyu left a comment

Choose a reason for hiding this comment

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

Please address the comments, and rest LGTM.

}

var caseSensitive bool
// INSTR performs case **insensitive** search by default
Copy link
Contributor

Choose a reason for hiding this comment

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

add . at the end of this comment.


var caseSensitive bool
// INSTR performs case **insensitive** search by default
// While at least one argument is binary string we do case sensitive search
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

caseSensitive = true
}

var pos int
Copy link
Contributor

Choose a reason for hiding this comment

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

var pos, idx int

return d, nil
}

var str string
Copy link
Contributor

Choose a reason for hiding this comment

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

var str, substr string

@coocood
Copy link
Member

coocood commented Mar 22, 2017

LGTM
Thank you for your contribution.

@coocood coocood added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Mar 22, 2017
@zimulala zimulala merged commit 2c3e731 into pingcap:master Mar 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution This PR is from a community contributor. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants