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: NOW() should be folded in constant folding stage #4385

Merged
merged 3 commits into from Aug 31, 2017

Conversation

Projects
None yet
3 participants
@zz-jason
Member

zz-jason commented Aug 31, 2017

fix #4384
to #4080

@XuHuaiyu

This comment has been minimized.

Show comment
Hide comment
@XuHuaiyu

XuHuaiyu Aug 31, 2017

Contributor

LGTM

Contributor

XuHuaiyu commented Aug 31, 2017

LGTM

@zz-jason

This comment has been minimized.

Show comment
Hide comment
@zz-jason
Member

zz-jason commented Aug 31, 2017

@breeswish PTAL

@@ -657,8 +657,8 @@ type builtinFunc interface {
// getArgs returns the arguments expressions.
getArgs() []Expression
// isDeterministic checks if a function is deterministic.
// A function is deterministic if it returns same results for same inputs.
// e.g. random is non-deterministic.
// A function is deterministic if it returns the same result whenever it's called with the same inputs.

This comment has been minimized.

@breeswish

breeswish Aug 31, 2017

Member

The comment needs changes. We set isDeterministic=true for NOW() is not because of "it returns the same result whenever it's called with the same inputs.".

@breeswish

breeswish Aug 31, 2017

Member

The comment needs changes. We set isDeterministic=true for NOW() is not because of "it returns the same result whenever it's called with the same inputs.".

This comment has been minimized.

@zz-jason

zz-jason Aug 31, 2017

Member

already discussed with @XuHuaiyu before.

isDeterministic() is only used in constant folding, my point is we can change isDeterministic() to canBeFolded() and change baseBuiltinFunc.deterministic to baseBuiltinFunc.canBeFolded

@zz-jason

zz-jason Aug 31, 2017

Member

already discussed with @XuHuaiyu before.

isDeterministic() is only used in constant folding, my point is we can change isDeterministic() to canBeFolded() and change baseBuiltinFunc.deterministic to baseBuiltinFunc.canBeFolded

This comment has been minimized.

@breeswish

breeswish Aug 31, 2017

Member

@zz-jason Agree with this naming change (defer to another PR).

@breeswish

breeswish Aug 31, 2017

Member

@zz-jason Agree with this naming change (defer to another PR).

@breeswish

This comment has been minimized.

Show comment
Hide comment
@breeswish

breeswish Aug 31, 2017

Member

LGTM

Member

breeswish commented Aug 31, 2017

LGTM

@breeswish breeswish added status/LGT2 and removed status/LGT1 labels Aug 31, 2017

@zz-jason zz-jason merged commit 9ae2639 into master Aug 31, 2017

3 checks passed

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

@zz-jason zz-jason deleted the 4384-now branch Aug 31, 2017

winkyao added a commit that referenced this pull request Sep 19, 2017

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