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

Improve subquery typing #49

Merged
merged 3 commits into from
Dec 28, 2017
Merged

Improve subquery typing #49

merged 3 commits into from
Dec 28, 2017

Conversation

petermattis
Copy link
Owner

@petermattis petermattis commented Dec 28, 2017 •
edited
Loading

  • Remove usage of IndexedVar
  • Use a sentinel for the "empty row"

This change is Reviewable

@petermattis
Copy link
Owner Author

Depends on cockroachdb/cockroach#21076.

@RaduBerinde
Copy link
Collaborator

:lgtm:

Review status: 0 of 7 files reviewed at latest revision, 3 unresolved discussions.

v3/relational_props.go, line 29 at r2 (raw file):

}

type columnProps struct {

[nit] this could use a comment, especially explaining how it can be part of an Expr

v3/relational_props.go, line 36 at r2 (raw file):

	hidden bool
}

add var _ tree.TypedExpr = &columnProps{} and same for VariableExpr

v3/scope.go, line 164 at r1 (raw file):

}

func (s *scope) replaceSubquery(sub *tree.Subquery, multiRow bool, desiredColumns int) *subquery {

[nit] add a comment explaining that desiredColumns = -1 means "any"

Comments from Reviewable

@petermattis
Improve subquery typing
ddab174
@petermattis
Remove usage of IndexedVar
7da255c 
Make `columnProps` implement the `tree.Expr` and `tree.IndexedExpr`
interfaces. This allows removal of the usage of `IndexedVar` and the
O(n) lookup of the column corresponding to an `IndexedVar`.
@petermattis
Use a sentinel for the "empty row"
85e3b7a
@petermattis
Copy link
Owner Author

Review status: 0 of 7 files reviewed at latest revision, 3 unresolved discussions.

v3/relational_props.go, line 29 at r2 (raw file):

Previously, RaduBerinde wrote…

[nit] this could use a comment, especially explaining how it can be part of an Expr

Done.

v3/relational_props.go, line 36 at r2 (raw file):

Previously, RaduBerinde wrote…

add var _ tree.TypedExpr = &columnProps{} and same for VariableExpr

Done.

v3/scope.go, line 164 at r1 (raw file):

Previously, RaduBerinde wrote…

[nit] add a comment explaining that desiredColumns = -1 means "any"

Done.

Comments from Reviewable

@petermattis petermattis merged commit f052c0c into master Dec 28, 2017
@petermattis petermattis deleted the pmattis/subquery branch December 28, 2017 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RaduBerinde RaduBerinde Awaiting requested review from RaduBerinde

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@petermattis @RaduBerinde