Update INNER JOIN Alpha #583
Conversation
… and get the data types correct
…-server into vinai/update-inner-join
sql/analyzer/assign_update_join.go
Outdated
| "github.com/dolthub/go-mysql-server/sql/plan" | ||
| ) | ||
|
|
||
| var ErrNonUpdateInnerJoinNotSupports = errors.NewKind("error: Only INNER JOINs are support for Update currently") |
There was a problem hiding this comment.
Just use sql.ErrUnsupportedFeature
sql/analyzer/assign_update_join.go
Outdated
| func getResolvedTableFromJoin(node plan.JoinNode) map[string]*plan.ResolvedTable { | ||
| toProcess := []sql.Node{node} | ||
| ret := make(map[string]*plan.ResolvedTable) | ||
| for len(toProcess) > 0 { |
There was a problem hiding this comment.
You want plan.Inspect here, don't roll your own
Also generalize this to any node (not just a join node), call it getTablesByName, and move it into tables.go
sql/analyzer/assign_update_join.go
Outdated
| } | ||
|
|
||
| // getRowUpdaterMap maps a set of tables to their RowUpdater objects. | ||
| func getRowUpdaterMap(ctx *sql.Context, node sql.Node, ij plan.JoinNode) map[string]sql.RowUpdater { |
sql/analyzer/assign_update_join.go
Outdated
| } | ||
|
|
||
| // getTablesToBeUpdated takes a node and looks for the tables to modified by a SetField. | ||
| func getTablesToBeUpdated(node sql.Node) map[string]interface{} { |
There was a problem hiding this comment.
Return type should be map[string]struct{}{}
sql/plan/update_join.go
Outdated
|
|
||
| // Resolved implements the sql.Node interface. | ||
| func (u *UpdateJoin) Resolved() bool { | ||
| return true |
There was a problem hiding this comment.
Don't override this, leave it to UnaryNode
sql/plan/update_join.go
Outdated
|
|
||
| // Children implements the sql.Node interface. | ||
| func (u *UpdateJoin) Children() []sql.Node { | ||
| return []sql.Node{u.Child} |
sql/plan/update_join.go
Outdated
|
|
||
| // Schema implements the sql.Node interface. | ||
| func (u *UpdateJoin) Schema() sql.Schema { | ||
| return u.Child.Schema() |
sql/plan/update_join.go
Outdated
|
|
||
| // Check if the row has already been updated | ||
| cache := u.getOrCreateCache(ctx, tableName) | ||
| hash, err := sql.HashOf(oldRow) |
There was a problem hiding this comment.
This only works if the table has primary keys. Otherwise the table could contain duplicate rows. We need some deeper integration into the table iterator on a second pass. For now, you need to error out in the analyzer if the table is keyless.
sql/plan/update_join.go
Outdated
|
|
||
| // Check if the row has already been updated | ||
| cache := u.getOrCreateCache(ctx, tableName) | ||
| hash, err := sql.HashOf(oldRow) |
There was a problem hiding this comment.
This only works if the table has primary keys. Otherwise the table could contain duplicate rows. We need some deeper integration into the table iterator on a second pass. For now, you need to error out in the analyzer if the table is keyless.
sql/plan/update_join.go
Outdated
| var _ sql.RowUpdater = (*updatableJoinUpdater)(nil) | ||
|
|
||
| // StatementBegin implements the sql.TableEditor interface. | ||
| func (u *updatableJoinUpdater) StatementBegin(ctx *sql.Context) {} |
There was a problem hiding this comment.
This probably isn't going to cut it. Why aren't you pushing this to the child updaters?
sql/plan/update_join.go
Outdated
|
|
||
| // RowIter implements the sql.Node interface. | ||
| func (u *UpdateJoin) RowIter(ctx *sql.Context, row sql.Row) (sql.RowIter, error) { | ||
| return u.Child.RowIter(ctx, row) |
There was a problem hiding this comment.
A better way to do this would be to wrap this iterator and only return a row when an update occurs (i.e. the first time one of the two rows was updated)
sql/plan/update_join.go
Outdated
|
|
||
| // DiscardChanges implements the sql.TableEditor interface. | ||
| func (u *updatableJoinUpdater) DiscardChanges(ctx *sql.Context, errorEncountered error) error { | ||
| u.updatedUpdaterMap = u.initialUpdaterMap |
| @@ -177,6 +177,69 @@ var UpdateTests = []WriteQueryTest{ | |||
| nil, | |||
| nil}}, | |||
| }, | |||
| { | |||
There was a problem hiding this comment.
Need more test cases:
- Updating more than one column in the same table
- Updating a column in both tables
- Updating a column using the value of a column from the other table
There was a problem hiding this comment.
It's good you have these in the script tests, but they should be test cases here as well
VinaiRachakonda
changed the title
sql/analyzer/tables.go
Outdated
| @@ -145,6 +145,26 @@ func getResolvedTable(node sql.Node) *plan.ResolvedTable { | |||
| return table | |||
| } | |||
|
|
|||
| // getTablesByNames takes a node and returns all found resolved tables in a map. | |||
| func getTablesByNames(node sql.Node) map[string]*plan.ResolvedTable { | |||
sql/plan/row_update_accumulator.go
Outdated
| }) | ||
|
|
||
| if schema == nil { | ||
| return nil, fmt.Errorf("UpdateJoin accumulator was requested about no join node found") |
sql/plan/update_join.go
Outdated
| _ = pr.WriteNode("Update Join") | ||
| _ = pr.WriteChildren(u.Child.String()) | ||
| return pr.String() | ||
|
|
No description provided.