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

*: rewrite builtin function GREATEST, LEAST #4476

Merged
merged 10 commits into from Sep 13, 2017

Conversation

Projects
None yet
4 participants
@XuHuaiyu
Contributor

XuHuaiyu commented Sep 8, 2017

@zz-jason zz-jason referenced this pull request Sep 8, 2017

Closed

rewrite builtin functions #4080

150 of 150 tasks complete
@@ -1348,7 +1348,8 @@ func WrapWithCastAsDecimal(expr Expression, ctx context.Context) Expression {
// of expr is not type string,
// otherwise, returns `expr` directly.
func WrapWithCastAsString(expr Expression, ctx context.Context) Expression {
if expr.GetTypeClass() == types.ClassString {
exprTp := expr.GetType().Tp
if expr.GetTypeClass() == types.ClassString && !types.IsTypeTime(exprTp) && exprTp != mysql.TypeDuration {

This comment has been minimized.

@shenli

shenli Sep 8, 2017

Member

Is this related with greatest/least?

@shenli

shenli Sep 8, 2017

Member

Is this related with greatest/least?

This comment has been minimized.

@jackysp

jackysp Sep 8, 2017

Member

It is better to add a comment here.

@jackysp

jackysp Sep 8, 2017

Member

It is better to add a comment here.

This comment has been minimized.

@XuHuaiyu

XuHuaiyu Sep 8, 2017

Contributor

It's a bug fix,
if we invoke evalString on a TimeExpr, it would raise an error.

@XuHuaiyu

XuHuaiyu Sep 8, 2017

Contributor

It's a bug fix,
if we invoke evalString on a TimeExpr, it would raise an error.

@@ -284,98 +291,404 @@ func (b *builtinCoalesceDurationSig) evalDuration(row []types.Datum) (res types.
return res, isNull, errors.Trace(err)
}
// temporalWithDateAsNumTypeClass makes DATE, DATETIME, TIMESTAMP pretend to be numbers rather than strings.

This comment has been minimized.

@shenli

shenli Sep 8, 2017

Member

Why?

@shenli

shenli Sep 8, 2017

Member

Why?

@@ -348,7 +348,7 @@ func DefaultTypeForValue(value interface{}, tp *FieldType) {
case mysql.TypeDatetime, mysql.TypeTimestamp:
tp.Flen = mysql.MaxDatetimeWidthNoFsp
if x.Fsp > DefaultFsp { // consider point('.') and the fractional part.
tp.Flen = x.Fsp + 1
tp.Flen += x.Fsp + 1

This comment has been minimized.

@shenli

shenli Sep 8, 2017

Member

Is this a bug fix? Any test covers this?

@shenli

shenli Sep 8, 2017

Member

Is this a bug fix? Any test covers this?

@@ -284,98 +291,404 @@ func (b *builtinCoalesceDurationSig) evalDuration(row []types.Datum) (res types.
return res, isNull, errors.Trace(err)
}
// temporalWithDateAsNumTypeClass makes DATE, DATETIME, TIMESTAMP pretend to be numbers rather than strings.
func temporalWithDateAsNumTypeClass(argTp *types.FieldType) (argTypeClass types.TypeClass, isStr bool, isTemporalWithDate bool) {
argTypeClass = argTp.ToClass()

This comment has been minimized.

@zz-jason

zz-jason Sep 10, 2017

Member

can we use evalTp instead of TypeClass ?

@zz-jason

zz-jason Sep 10, 2017

Member

can we use evalTp instead of TypeClass ?

This comment has been minimized.

@XuHuaiyu

XuHuaiyu Sep 11, 2017

Contributor

The modification will influence the implementation of getCmpType,
thus influence the functions which use it.
Better to change this together.

@XuHuaiyu

XuHuaiyu Sep 11, 2017

Contributor

The modification will influence the implementation of getCmpType,
thus influence the functions which use it.
Better to change this together.

@zz-jason

This comment has been minimized.

Show comment
Hide comment
@zz-jason

zz-jason Sep 11, 2017

Member

LGTM

Member

zz-jason commented Sep 11, 2017

LGTM

@zz-jason

This comment has been minimized.

Show comment
Hide comment
@zz-jason

zz-jason Sep 11, 2017

Member

/run-all-test

Member

zz-jason commented Sep 11, 2017

/run-all-test

@XuHuaiyu

This comment has been minimized.

Show comment
Hide comment
@XuHuaiyu

XuHuaiyu Sep 11, 2017

Contributor

/run-integration-common-test
/run-integration-ddl-test

Contributor

XuHuaiyu commented Sep 11, 2017

/run-integration-common-test
/run-integration-ddl-test

@zz-jason zz-jason changed the title from *: rewrite builtin func greatest/least to *: rewrite builtin function GREATEST, LEAST Sep 12, 2017

@zz-jason

This comment has been minimized.

Show comment
Hide comment
@zz-jason
Member

zz-jason commented Sep 12, 2017

XuHuaiyu and others added some commits Sep 13, 2017

@jackysp

LGTM

@jackysp jackysp added status/LGT2 and removed status/LGT1 labels Sep 13, 2017

@XuHuaiyu XuHuaiyu merged commit 959db2b into master Sep 13, 2017

4 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci-tidb/build Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details

@XuHuaiyu XuHuaiyu deleted the xhy/greatest branch Sep 13, 2017

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