-
Notifications
You must be signed in to change notification settings - Fork 109
Check projection aliases when assigned to index. #640
Conversation
@@ -18,6 +18,7 @@ require ( | |||
github.com/mitchellh/hashstructure v1.0.0 | |||
github.com/oliveagle/jsonpath v0.0.0-20180606110733-2e52cf6e6852 | |||
github.com/opentracing/opentracing-go v1.0.2 | |||
github.com/pbnjay/memory v0.0.0-20190104145345-974d429e7ae4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is missed from the master
sql/index.go
Outdated
expressions := make([]string, len(expr)) | ||
for i, e := range expr { | ||
expressions[i] = e.String() | ||
expressions := make(map[string]string) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we passing expr
and aliases
all the way here instead of just passing the expressions already normalized?
Instead of collecting a map[string]string
of aliases, we could collect a map[string]sql.Expression
and just pass the values of that map as expr
. Then, no API would need to change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to pass aliases from assignIndexes
to getIndexes
, because how we can combine them into map[string]sql.Expression
in an obvious way?
Moreover in func getIndexes(e sql.Expression, a *Analyzer)
we cast expression and pass left/right to getIndexes
again or to IndexByExpression
.
What I can do is to combine expressions and aliases before calling IndexByExpression
, so instead of having IndexByExpression(db string, aliases map[string]string, expr ...Expression)
we will have IndexByExpression(db string, expressions map[string]string)
, so basically move outside the IndexByExpression
:
expressions := make(map[string]string)
for _, e := range expr {
if aliases != nil {
expressions[e.String()] = aliases[e.String()]
} else {
expressions[e.String()] = ""
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I mean is not having to change the api of IndexByExpression
or anything like that so they don't need to know anything about aliases.
Instead, just pass the dealiased expressions.
If you would have to pass mytable.A
and someAlias
to IndexByExpression
, instead pass mytable.A
and foo(mytable.B)
, no need for any kind of mapping.
That way the functions that look for indexes don't need to know anything about aliases of any kind, just looking for those expressions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so I can normalize expressions and aliases and pass to IndexByExpression only expressions (or actually):
IndexByExpression(db string, expr ...string) Index
because we don't need whole Expression
object inside IndexByExpression
and we already have aliases as strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if you got the string
of the expression, means you had the expression. Then instead I would pass the sql.Expression
itself so the API does not need to change. The fact that we use the string
is an implementation detail, it's better to keep passing sql.Expression
s IMHO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@erizocosmico - PTAL (rollback).
Signed-off-by: kuba-- <kuba@sourced.tech>
sql/analyzer/assign_indexes.go
Outdated
@@ -42,8 +42,17 @@ func assignIndexes(a *Analyzer, node sql.Node) (map[string]*indexLookup, error) | |||
return true | |||
} | |||
|
|||
aliases := make(map[string]sql.Expression) | |||
if prj, ok := filter.Child.(*plan.Project); ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two cases that I think we're not handling here but can happen:
- The alias was not defined in this project but in another one. Example:
Filter(a = 5)
|- Project(a, mytable.B) <- a here is still an alias
|- Project(mytable.A as a, mytable.B) <-- it's in another project
- Group Bys can also define aliases because they act like projects
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are aliases local per query?
How about shadowing?
What if I have
SELECT a as foo from (
SELECT b as foo from T
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Subqueries use their own indexes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But how about aliases? Here I have foo
for a and b.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
foo
projected from the subquery should have T
as table, so it's not an alias. Besides, you stop trying to find an alias for foo as soon as you find one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a recursive function to deeply inspect all aliases.
I think group by is not really a case here because we don't use indexes for group by.
Signed-off-by: kuba-- <kuba@sourced.tech>
Signed-off-by: kuba-- kuba@sourced.tech
Fixes #639
Collect projection aliases and pass down to
IndexByExpression