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

parser, ast, expression: support TIME/TIMESTAMP literal #4368

Merged
merged 37 commits into from Sep 19, 2017

Conversation

Projects
None yet
6 participants
@spongedu
Contributor

spongedu commented Aug 29, 2017

for #4307

ast.TimeFormat: &timeFormatFunctionClass{baseFunctionClass{ast.TimeFormat, 2, 2}},
ast.TimeToSec: &timeToSecFunctionClass{baseFunctionClass{ast.TimeToSec, 1, 1}},
ast.TimeDiff: &timeDiffFunctionClass{baseFunctionClass{ast.TimeDiff, 2, 2}},
ast.Timestamp: &timestampFunctionClass{baseFunctionClass{ast.Timestamp, 1, 2}},
ast.TimestampLiteral: &timestampLiteralFunctionClass{baseFunctionClass{ast.TimestampLiteral, 1, 2}},

This comment has been minimized.

@zz-jason

zz-jason Aug 29, 2017

Member

how about put the three literal functions together ?

@zz-jason

zz-jason Aug 29, 2017

Member

how about put the three literal functions together ?

This comment has been minimized.

@spongedu

spongedu Aug 29, 2017

Contributor

These three are quite different. I think it's better to seperate them.

@spongedu

spongedu Aug 29, 2017

Contributor

These three are quite different. I think it's better to seperate them.

This comment has been minimized.

@zz-jason
@zz-jason

zz-jason Aug 29, 2017

Member

ok

Show outdated Hide outdated expression/builtin_time.go Outdated
Show outdated Hide outdated expression/builtin_time.go Outdated
Show outdated Hide outdated expression/builtin_time.go Outdated
Show outdated Hide outdated expression/builtin_time.go Outdated
@zz-jason

This comment has been minimized.

Show comment
Hide comment
@zz-jason
Member

zz-jason commented Aug 30, 2017

@breeswish PTAL

@breeswish

You can also add some test cases to verify the field type of these literals in typeinfer_test.

Show outdated Hide outdated parser/parser.y Outdated
Show outdated Hide outdated parser/parser.y Outdated
Show outdated Hide outdated parser/parser_test.go Outdated
Show outdated Hide outdated parser/parser_test.go Outdated

spongedu added some commits Aug 30, 2017

Show outdated Hide outdated expression/builtin_time.go Outdated
Show outdated Hide outdated expression/builtin_time.go Outdated
Show outdated Hide outdated expression/builtin_time.go Outdated
Show outdated Hide outdated parser/parser.y Outdated
Show outdated Hide outdated expression/builtin_time.go Outdated
@breeswish

This comment has been minimized.

Show comment
Hide comment
@breeswish

breeswish Aug 31, 2017

Member

is date evaluated for every row?

Member

breeswish commented Aug 31, 2017

is date evaluated for every row?

spongedu added some commits Aug 31, 2017

@spongedu

This comment has been minimized.

Show comment
Hide comment
@spongedu

spongedu Aug 31, 2017

Contributor

@breeswish now I evaluate the literal values before execution, just like what I've done in #4362. PTAL

Contributor

spongedu commented Aug 31, 2017

@breeswish now I evaluate the literal values before execution, just like what I've done in #4362. PTAL

@breeswish

This comment has been minimized.

Show comment
Hide comment
@breeswish

breeswish Sep 7, 2017

Member

@XuHuaiyu What do you think? Shall we evaluate the date literal in the parsing stage just like other literals (i.e. bit/hex), or in the constant folding stage?

Member

breeswish commented Sep 7, 2017

@XuHuaiyu What do you think? Shall we evaluate the date literal in the parsing stage just like other literals (i.e. bit/hex), or in the constant folding stage?

@sre-bot

This comment has been minimized.

Show comment
Hide comment
@sre-bot

sre-bot Sep 7, 2017

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.

sre-bot commented Sep 7, 2017

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.

@XuHuaiyu

This comment has been minimized.

Show comment
Hide comment
@XuHuaiyu

XuHuaiyu Sep 11, 2017

Contributor

@breeswish
For my part, It's ok to calculate it in the constant-folding stage.

Contributor

XuHuaiyu commented Sep 11, 2017

@breeswish
For my part, It's ok to calculate it in the constant-folding stage.

@breeswish

This comment has been minimized.

Show comment
Hide comment
@breeswish

breeswish Sep 11, 2017

Member

@XuHuaiyu OK. PTAL

Member

breeswish commented Sep 11, 2017

@XuHuaiyu OK. PTAL

Show outdated Hide outdated expression/builtin_time.go Outdated
Show outdated Hide outdated expression/builtin_time.go Outdated
Show outdated Hide outdated expression/builtin_time.go Outdated

spongedu added some commits Sep 15, 2017

Merge remote-tracking branch 'upstream/master' into time_literal
Conflicts:
	expression/builtin_time.go
	plan/typeinfer_test.go
Show outdated Hide outdated expression/builtin_time.go Outdated
Show outdated Hide outdated expression/builtin_time.go Outdated
@zz-jason

This comment has been minimized.

Show comment
Hide comment
@zz-jason

zz-jason Sep 18, 2017

Member

LGTM

Member

zz-jason commented Sep 18, 2017

LGTM

@zz-jason

This comment has been minimized.

Show comment
Hide comment
@zz-jason

zz-jason Sep 18, 2017

Member

/run-all-test

Member

zz-jason commented Sep 18, 2017

/run-all-test

if err := c.verifyArgs(args); err != nil {
return nil, errors.Trace(err)
}
constant, ok := args[0].(*Constant)

This comment has been minimized.

@XuHuaiyu

XuHuaiyu Sep 18, 2017

Contributor

will this happen, args[0] is not a constant?

@XuHuaiyu

XuHuaiyu Sep 18, 2017

Contributor

will this happen, args[0] is not a constant?

This comment has been minimized.

@spongedu

spongedu Sep 18, 2017

Contributor

No. The parser would fail if args[0] is not a constant.

@spongedu

spongedu Sep 18, 2017

Contributor

No. The parser would fail if args[0] is not a constant.

This comment has been minimized.

@XuHuaiyu

XuHuaiyu Sep 19, 2017

Contributor

so why we check whether ok is true here?

@XuHuaiyu

XuHuaiyu Sep 19, 2017

Contributor

so why we check whether ok is true here?

This comment has been minimized.

@spongedu

spongedu Sep 19, 2017

Contributor

This can be deleted, but I think it's better to keep it as defensive code. It something goes wrong, it's better to panic than return an weird result.

@spongedu

spongedu Sep 19, 2017

Contributor

This can be deleted, but I think it's better to keep it as defensive code. It something goes wrong, it's better to panic than return an weird result.

@XuHuaiyu

This comment has been minimized.

Show comment
Hide comment
@XuHuaiyu

XuHuaiyu Sep 19, 2017

Contributor

please resolve the conflicts

Contributor

XuHuaiyu commented Sep 19, 2017

please resolve the conflicts

@XuHuaiyu

LGTM

@zz-jason zz-jason merged commit a239324 into pingcap:master Sep 19, 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

@spongedu spongedu deleted the spongedu:time_literal branch Sep 26, 2017

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