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.

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: support ConstItem() for expression (#10004) #10167

Closed
wants to merge 1 commit into from

Conversation

qw4990
Copy link
Contributor

@qw4990 qw4990 commented Apr 16, 2019

cherry-pick for #10004
some bugs can be fixed by ConstItem() like #9033, #10156

@qw4990
Copy link
Contributor Author

qw4990 commented Apr 16, 2019

/run-all-tests tidb-test=release-2.1 tikv=release-2.1 pd=release-2.1

Copy link
Contributor

@alivxxx alivxxx left a comment

Choose a reason for hiding this comment

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

LGTM

@alivxxx alivxxx added the status/LGT1 Indicates that a PR has LGTM 1. label Apr 16, 2019
@@ -124,6 +124,11 @@ func (col *CorrelatedColumn) IsCorrelated() bool {
return true
}

// ConstItem implements Expression interface.
func (col *CorrelatedColumn) ConstItem() bool {
return false
Copy link
Member

Choose a reason for hiding this comment

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

We just set it to true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm afraid it would cause some other problems.
For example, when building some scalar function like from_unixtime(CorCol), we may use it to decide if we can evaluate the argument at that time.
If it is true, we may call CorCol.Eval(NullRow) and this function call is invalid.

Copy link
Contributor Author

@qw4990 qw4990 Apr 17, 2019

Choose a reason for hiding this comment

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

In my opinion, ConstItem means 1) this expression has no column reference, 2) this expression doesn't include some special scalar functions which behave differently according to time like Rand and SysDate.
CorrelatedColumn breaks the rule 1).
Maybe name ConsistentItem is more closer to its meaning.

@qw4990
Copy link
Contributor Author

qw4990 commented Apr 17, 2019

This function has some problems for CorrelatedColumn.
I will fix these problems in master and then cherry-pick it again.

@qw4990 qw4990 closed this Apr 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/expression status/LGT1 Indicates that a PR has LGTM 1.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants