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

expression: add builtin function tidb_parse_tso #8385

Merged
merged 20 commits into from Nov 28, 2018

Conversation

Projects
None yet
9 participants
@yu34po
Contributor

yu34po commented Nov 21, 2018

What problem does this PR solve?

#8384

What is changed and how it works?

add time builtin function tso

Check List

  • Unit test
  • Integration test

Code changes

  • Has exported function/method change
  • Has exported variable/fields change

Side effects

Related changes


This change is Reviewable

@sre-bot

This comment has been minimized.

sre-bot commented Nov 21, 2018

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.

@jackysp

This comment has been minimized.

Member

jackysp commented Nov 21, 2018

/ok-to-test

@jackysp

This comment has been minimized.

Member

jackysp commented Nov 21, 2018

Thanks for your contribution, @yu34po !
Please fix CI, seems the failure is about the timezone.

@jackysp

This comment has been minimized.

Member

jackysp commented Nov 21, 2018

/run-all-tests

Show resolved Hide resolved expression/builtin_time.go Outdated
// evalTime evals a builtinTsoSig.
func (b *builtinTsoSig) evalTime(row chunk.Row) (types.Time, bool, error) {
arg, isNull, err := b.args[0].EvalInt(b.ctx, row)
if isNull || err != nil || arg <= 0 {

This comment has been minimized.

@dbjoa

dbjoa Nov 22, 2018

Contributor

Why should we consider the case of isNull == true as an error ?

This comment has been minimized.

@yu34po

yu34po Nov 22, 2018

Contributor

function must accept a parameter

This comment has been minimized.

@dbjoa

dbjoa Nov 23, 2018

Contributor

got it.

gregwebs and others added some commits Nov 21, 2018

planner/core,session: fix privilege check for update (#8376)
In the statement: "update t1,t2 set t1.id = t2.id"
TiDB should check update privilege for t1 and select privilege for t2,
Fix a bug that it checks update privilege for both t1 and t2
@dbjoa

This comment has been minimized.

Contributor

dbjoa commented Nov 22, 2018

@yu34po The PR includes vender files, which should not be. Would you check whether or not you set the environment variable, GO111MODULE=on?

@dbjoa

This comment has been minimized.

Contributor

dbjoa commented Nov 22, 2018

@yu34po Would you also add the test for SQL statements having tidb_parse_tso()? expression/integration_test.go can be a candidate place for the test.

@@ -413,6 +413,7 @@ var funcs = map[string]functionClass{
ast.Year: &yearFunctionClass{baseFunctionClass{ast.Year, 1, 1}},
ast.YearWeek: &yearWeekFunctionClass{baseFunctionClass{ast.YearWeek, 1, 2}},
ast.LastDay: &lastDayFunctionClass{baseFunctionClass{ast.LastDay, 1, 1}},
ast.Tso: &tsoFunctionClass{baseFunctionClass{ast.Tso, 1, 1}},

This comment has been minimized.

@XuHuaiyu

XuHuaiyu Nov 22, 2018

Contributor

We'd better change ast.Tso to ast.TidbParseTso

This comment has been minimized.

@yu34po

yu34po Nov 22, 2018

Contributor

ast.Tso already merged in parser master

This comment has been minimized.

@XuHuaiyu

XuHuaiyu Nov 22, 2018

Contributor

We can push another PR to change the name

@XuHuaiyu XuHuaiyu changed the title from add builtin function from_tso to expression: add builtin function tidb_parse_tso Nov 22, 2018

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

LGTM

@zz-jason

This comment has been minimized.

Member

zz-jason commented Nov 22, 2018

@XuHuaiyu

I think we'd better:

  1. change ast.Tso to ast.TiDBParseTso
  2. change tsoXXX to tidbParseTsoXXX
@dbjoa

This comment has been minimized.

Contributor

dbjoa commented Nov 23, 2018

LGTM

@XuHuaiyu

Please address the comment.
Rest LGTM.
Thanks for your contribution.

Show resolved Hide resolved expression/builtin_time.go Outdated
go.mod Outdated
@@ -49,3 +50,5 @@ require (
gopkg.in/natefinch/lumberjack.v2 v2.0.0
sourcegraph.com/sourcegraph/appdash v0.0.0-20180531100431-4c381bd170b4
)
replace github.com/pingcap/parser => github.com/yu34po/parser v0.0.0-20181123030610-1d6c3c585f2d

This comment has been minimized.

@zz-jason

zz-jason Nov 24, 2018

Member

please update go.mod to use the latest pingcap/parser.

@zz-jason

please update go.mod

@zz-jason

This comment has been minimized.

Member

zz-jason commented Nov 26, 2018

@yu34po pingcap/parser#50 has been merged, you can update go.mod to the latest pingcap/parser now.

yu34po added some commits Nov 27, 2018

go.mod Outdated
@@ -41,6 +41,7 @@ require (
github.com/shirou/gopsutil v2.18.10+incompatible
github.com/sirupsen/logrus v1.2.0
github.com/spaolacci/murmur3 v0.0.0-20180118202830-f09979ecbc72
github.com/tiancaiamao/appdash v0.0.0-20181126055449-889f96f722a2

This comment has been minimized.

@zz-jason

zz-jason Nov 27, 2018

Member

why add this?

This comment has been minimized.

@yu34po

yu34po Nov 27, 2018

Contributor

I don't know how this happened.
same as master expect parser version

This comment has been minimized.

@zz-jason

zz-jason Nov 28, 2018

Member

It disappeared after merging the master branch..

zz-jason added some commits Nov 27, 2018

@zz-jason

LGTM

@zz-jason zz-jason added status/LGT2 and removed status/LGT1 labels Nov 28, 2018

@zz-jason

This comment has been minimized.

Member

zz-jason commented Nov 28, 2018

/run-all-tests

@zz-jason zz-jason merged commit 2f6639d into pingcap:master Nov 28, 2018

11 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
jenkins-ci-tidb/common-test Jenkins job succeeded.
Details
jenkins-ci-tidb/integration-common-test Jenkins job succeeded.
Details
jenkins-ci-tidb/integration-compatibility-test Jenkins job succeeded.
Details
jenkins-ci-tidb/integration-ddl-test Jenkins job succeeded.
Details
jenkins-ci-tidb/mybatis-test Jenkins job succeeded.
Details
jenkins-ci-tidb/sqllogic-test Jenkins job succeeded.
Details
jenkins-ci-tidb/unit-test Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment