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.

Sign up for GitHub

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

Jump to bottom

expression, executor: rewrite built-in func makeDate using new expression evaluation architecture #3533

Merged
merged 15 commits into from
Jun 23, 2017
Merged

expression, executor: rewrite built-in func makeDate using new expression evaluation architecture #3533

merged 15 commits into from
Jun 23, 2017

Conversation

dawxy
Copy link
Contributor

@dawxy dawxy commented Jun 22, 2017

Rewrite built-in func makeDate using new expression evaluation architecture

@dawxy dawxy changed the title Rewrite built-in func makeDate using new expression evaluation architecture expression, executor: rewrite built-in func makeDate using new expression evaluation architecture Jun 22, 2017
@dawxy dawxy changed the title expression, executor: rewrite built-in func makeDate using new expression evaluation architecture expression rewrite built-in func makeDate using new expression evaluation architecture Jun 22, 2017
@dawxy dawxy changed the title expression rewrite built-in func makeDate using new expression evaluation architecture expression: rewrite built-in func makeDate using new expression evaluation architecture Jun 22, 2017
@dawxy dawxy changed the title expression: rewrite built-in func makeDate using new expression evaluation architecture expression, executor: rewrite built-in func makeDate using new expression evaluation architecture Jun 22, 2017
@hanfei1991 hanfei1991 added the contribution This PR is from a community contributor. label Jun 22, 2017
dawxy
dawxy commented Jun 22, 2017
View reviewed changes
expression/builtin_time.go Outdated
@@ -2023,35 +2023,36 @@ type makeDateFunctionClass struct {
}

func (c *makeDateFunctionClass) getFunction(args []Expression, ctx context.Context) (builtinFunc, error) {
sig := &builtinMakeDateSig{newBaseBuiltinFunc(args, ctx)}
tp := types.NewFieldType(mysql.TypeDate)
tp.Flen = 10
Copy link
Contributor Author

@dawxy dawxy Jun 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

No field length?
https://github.com/mysql/mysql-server/blob/5.7/sql/item_timefunc.h#L1581

Copy link
Contributor

Choose a reason for hiding this comment

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

@dawxy
you may refer to this

Clion can be used to check MySQL source code.

Copy link
Contributor Author

@dawxy dawxy Jun 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@XuHuaiyu
Thank you, I found it,and the MAX_DATE_WIDTH is 10.
refer https://github.com/mysql/mysql-server/blob/5.7/sql/sql_const.h#L59

Copy link
Contributor

Choose a reason for hiding this comment

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

You may define MAX_DATE_WIDTH as a constant in mysql/const.go as MySQL does

@CLAassistant
Copy link

CLAassistant commented Jun 23, 2017 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@dawxy dawxy force-pushed the dawxy/rewrite_makedate branch 4 times, most recently from 6cf38ec to 8078486 Compare June 23, 2017 02:02
@dawxy
Copy link
Contributor Author

dawxy commented Jun 23, 2017

@hanfei1991 @XuHuaiyu , PTAL, thanks.

XuHuaiyu
XuHuaiyu reviewed Jun 23, 2017
View reviewed changes
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.

Rest LGTM

expression/builtin_time.go Outdated
func (b *builtinMakeDateSig) evalTime(row []types.Datum) (d types.Time, isNull bool, err error) {
args := b.getArgs()
var year, dayOfYear int64
year, isNull, err = args[0].EvalInt(row, b.ctx.GetSessionVars().StmtCtx)
Copy link
Contributor

Choose a reason for hiding this comment

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

sc := b.ctx.GetSessionVars().StmtCtx
so we can use it at line 2050

expression/builtin_time_test.go
c.Assert(err, IsNil)
if t["Want"][0].Kind() == types.KindNull {
c.Assert(got.Kind(), Equals, types.KindNull, Commentf("[%v] - args:%v", idx, t["Args"]))
{[]interface{}{71, 1}, "1971-01-01", false, false},
Copy link
Contributor

Choose a reason for hiding this comment

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

add test cases for float64 value

mysql/const.go Outdated
@@ -193,6 +193,11 @@ const (
// AllPrivMask is the mask for PrivilegeType with all bits set to 1.
const AllPrivMask = AllPriv - 1

const (
Copy link
Contributor

Choose a reason for hiding this comment

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

add comments for these const.

@winoros
Copy link
Member

winoros commented Jun 23, 2017

You can run make dev to make sure that you can pass CI.
And make check to format your code.

@dawxy
Copy link
Contributor Author

dawxy commented Jun 23, 2017

@XuHuaiyu
fixed

XuHuaiyu
XuHuaiyu reviewed Jun 23, 2017
View reviewed changes
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.

Rest LGTM

mysql/const.go Outdated
@@ -193,6 +193,12 @@ const (
// AllPrivMask is the mask for PrivilegeType with all bits set to 1.
const AllPrivMask = AllPriv - 1

// MYSQL type maximum length.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/MYSQL/MySQL

hanfei1991
hanfei1991 reviewed Jun 23, 2017
View reviewed changes
expression/builtin_time.go Outdated
var year, dayOfYear int64
year, isNull, err = args[0].EvalInt(row, sc)
if isNull || err != nil {
return d, isNull, errors.Trace(err)
Copy link
Member

Choose a reason for hiding this comment

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

return d, true is clearer

expression/builtin_time.go Outdated
return d, errors.Trace(err)
dayOfYear, isNull, err = args[1].EvalInt(row, sc)
if isNull || err != nil {
return d, isNull, errors.Trace(err)
Copy link
Member

Choose a reason for hiding this comment

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

ditto.

@hanfei1991
Copy link
Member

LGTM

@XuHuaiyu XuHuaiyu added the status/LGT2 Indicates that a PR has LGTM 2. label Jun 23, 2017
@hanfei1991 hanfei1991 merged commit 4db09e0 into pingcap:master Jun 23, 2017
@dawxy dawxy deleted the dawxy/rewrite_makedate branch June 24, 2017 06:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@XuHuaiyu XuHuaiyu XuHuaiyu left review comments

@hanfei1991 hanfei1991 hanfei1991 approved these changes

Assignees
No one assigned
Labels
contribution This PR is from a community contributor. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dawxy @CLAassistant @winoros @hanfei1991 @XuHuaiyu