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

coprocessor, dag: add like signature. #2247

Merged
merged 37 commits into from Sep 7, 2017

Conversation

Projects
None yet
6 participants
@hicqu
Contributor

hicqu commented Sep 2, 2017

No description provided.

@hicqu

This comment has been minimized.

Show comment
Hide comment
@hicqu

hicqu Sep 5, 2017

Contributor

@BusyJay @AndreMouche PTAL again, thanks.

Contributor

hicqu commented Sep 5, 2017

@BusyJay @AndreMouche PTAL again, thanks.

@hicqu

This comment has been minimized.

Show comment
Hide comment
@hicqu

hicqu Sep 5, 2017

Contributor

@XuHuaiyu , both TiKV and TiDB should follow MySQL's behaviour. Please make LIKE in TiDB case-insensitive.

Contributor

hicqu commented Sep 5, 2017

@XuHuaiyu , both TiKV and TiDB should follow MySQL's behaviour. Please make LIKE in TiDB case-insensitive.

@XuHuaiyu

This comment has been minimized.

Show comment
Hide comment
@XuHuaiyu

XuHuaiyu commented Sep 5, 2017

@hicqu got it.

@hanfei1991

This comment has been minimized.

Show comment
Hide comment
@hanfei1991

hanfei1991 Sep 5, 2017

Contributor

@hicqu If we keep consistent with MySQL, the like predicate cannot use index any more.

Contributor

hanfei1991 commented Sep 5, 2017

@hicqu If we keep consistent with MySQL, the like predicate cannot use index any more.

@hicqu

This comment has been minimized.

Show comment
Hide comment
@hicqu

hicqu Sep 5, 2017

Contributor

So, after discuss about this, for now we are going to keep LIKE case-sensitive. @XuHuaiyu @BusyJay .

Contributor

hicqu commented Sep 5, 2017

So, after discuss about this, for now we are going to keep LIKE case-sensitive. @XuHuaiyu @BusyJay .

hicqu added some commits Sep 5, 2017

@hicqu

This comment has been minimized.

Show comment
Hide comment
@hicqu

hicqu Sep 6, 2017

Contributor

@BusyJay @AndreMouche PTAL, thanks.

Contributor

hicqu commented Sep 6, 2017

@BusyJay @AndreMouche PTAL, thanks.

AndreMouche and others added some commits Sep 7, 2017

hicqu added some commits Sep 7, 2017

@hicqu

This comment has been minimized.

Show comment
Hide comment
@hicqu

hicqu Sep 7, 2017

Contributor

@BusyJay , PTAL thanks.

Contributor

hicqu commented Sep 7, 2017

@BusyJay , PTAL thanks.

hicqu added some commits Sep 7, 2017

hicqu added some commits Sep 7, 2017

@BusyJay

I'm quite worry about the inconsistent behavior between pre-ga and ga version caused by this pr.

Show outdated Hide outdated src/coprocessor/dag/expr/compare.rs
row: &'a [Datum],
) -> Result<Option<Cow<'a, str>>> {
let bytes = try_opt!(self.eval_string(ctx, row));
let chrst = self.get_tp().get_charset();

This comment has been minimized.

@BusyJay

BusyJay Sep 7, 2017

Contributor

Is the charset always set?

@BusyJay

BusyJay Sep 7, 2017

Contributor

Is the charset always set?

This comment has been minimized.

@hicqu

hicqu Sep 7, 2017

Contributor

I think so, TiDB will set it to UTF8 by default. See tidb/expression/builtin_cast.go:1342.

@hicqu

hicqu Sep 7, 2017

Contributor

I think so, TiDB will set it to UTF8 by default. See tidb/expression/builtin_cast.go:1342.

@hicqu

This comment has been minimized.

Show comment
Hide comment
@hicqu

hicqu Sep 7, 2017

Contributor

We don't have full collate support. the behavior of LIKE depends on that. So I think for now keeping it case-sensitive is OK, just like we keep 'abc' != 'ABC'. :)

Contributor

hicqu commented Sep 7, 2017

We don't have full collate support. the behavior of LIKE depends on that. So I think for now keeping it case-sensitive is OK, just like we keep 'abc' != 'ABC'. :)

@BusyJay

This comment has been minimized.

Show comment
Hide comment
@BusyJay

BusyJay Sep 7, 2017

Contributor

LGTM

Contributor

BusyJay commented Sep 7, 2017

LGTM

@BusyJay

This comment has been minimized.

Show comment
Hide comment
@BusyJay

BusyJay Sep 7, 2017

Contributor

/run-all-tests

Contributor

BusyJay commented Sep 7, 2017

/run-all-tests

@AndreMouche

LGTM

@hicqu

This comment has been minimized.

Show comment
Hide comment
@hicqu

hicqu Sep 7, 2017

Contributor

/run-all-tests

Contributor

hicqu commented Sep 7, 2017

/run-all-tests

@hicqu hicqu merged commit f7a1389 into master Sep 7, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
jenkins-ci-tikv/build Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details

@hicqu hicqu deleted the qupeng/expr-like branch Sep 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment