-
-
Notifications
You must be signed in to change notification settings - Fork 209
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
Fixed bug in field indexes related to pushdown of indexes in subqueries #430
Conversation
…dexed table access (which breaks other things)
…aliased table name
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.
LGTM, seems worth revisiting as the column index mappings are gathering complexity
sql/analyzer/indexed_joins.go
Outdated
@@ -173,29 +174,29 @@ func replaceTableAccessWithIndexedAccess( | |||
} | |||
|
|||
// the condition's field indexes might need adjusting if the order of tables changed | |||
cond, err := FixFieldIndexes(scope, append(schema, append(left.Schema(), right.Schema()...)...), node.Cond) | |||
cond, err := FixFieldIndexes(scope, nil, append(schema, append(left.Schema(), right.Schema()...)...), node.Cond) |
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.
intended nil
?
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.
Nope, good catch
), nil | ||
newIndex := scopeLen - offset + i | ||
if e.Index() != newIndex { | ||
a.Log("Rewriting field %s.%s from index %d to %d", e.Table(), e.Name(), e.Index(), newIndex) |
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.
seems like a lot of threading to get the log statement here
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.
Yeah, eventually we need to put the logger in the session or something
No description provided.