Add Right/left/Cross/Indexed Joins and Implement Table Edit Accumulator #596
Conversation
VinaiRachakonda
commented
Oct 21, 2021
•
VinaiRachakonda
commented
- Add functionality for updates to work with Indexed Joins
- Implement a Table Edit Accumulator that writes changes at Close time
- Fix some undefined test cases
- Add functionality for Update Join w/ Right,Left,Cross joins
VinaiRachakonda
changed the title
|
|
||
| // Get implements the tableEditAccumulator interface. | ||
| func (k *keylessTableEditAccumulator) Get(value sql.Row) (sql.Row, bool, bool, error) { | ||
| // Note: Keyless tables do not have to return an accurate answer here as any given row can be inserted or deleted |
There was a problem hiding this comment.
I don't like this tbh but I also don't like adding conditional logic in table.go for checking this condition. Maybe implement a keyless table implementation as well?
VinaiRachakonda
changed the title
| @@ -261,6 +261,110 @@ var UpdateTests = []WriteQueryTest{ | |||
| sql.NewRow(1, 1, 32, 31, 32, 33, 34), | |||
| }, | |||
| }, | |||
| { | |||
| WriteQuery: `UPDATE one_pk INNER JOIN two_pk on one_pk.pk = two_pk.pk1 INNER JOIN two_pk a1 on one_pk.pk = two_pk.pk2 SET two_pk.c1 = two_pk.c1 + 1`, // cross join | |||
There was a problem hiding this comment.
I'm not verifying these query results by hand, make sure you have (or tested them on mysql)
There was a problem hiding this comment.
This isn't a cross join (a cross join has no join condition)(
| }, | ||
| }, | ||
| { | ||
| WriteQuery: `UPDATE othertable INNER JOIN tabletest on othertable.i2=3 and tabletest.i=3 SET othertable.s2 = 'fourth'`, // cross join |
There was a problem hiding this comment.
This is a cross join but not phrased as on
A cross join has no join condition
This just reduces to a cross join because the analyzer realizes that none of the join predicates are actually join predicates to moves them to the filter.
You should keep this test and add another one that is an explicit cross join
There was a problem hiding this comment.
You should make sure you have tests for the implicit cross join, e.g.
update table1, table2 set...
enginetest/update_queries.go
Outdated
| ExpectedErr: sql.ErrUnsupportedFeature, | ||
| }, | ||
| } | ||
| var UpdateErrorTests = []QueryErrorTest{} |
memory/table.go
Outdated
| return err | ||
| } | ||
|
|
||
| t.table = tbl |
There was a problem hiding this comment.
Shouldn't need to do this, you won't be reusing this object after close is called
memory/table.go
Outdated
| for _, i := range pkColIdxes { | ||
| vals[i] = row[pkColIdxes[i]] | ||
| } | ||
| return sql.NewUniqueKeyErr(fmt.Sprint(vals), true, partitionRow) |
There was a problem hiding this comment.
Do we have engine tests for this?
(We don't, they wouldn't have passed before)
memory/table_editor.go
Outdated
| return nil | ||
| } | ||
|
|
||
| // insertHelper deletes a row from a keyless table, if it exists. |
memory/table_editor.go
Outdated
| // insertHelper deletes a row from a keyless table, if it exists. | ||
| func (k *keylessTableEditAccumulator) insertHelper(ctx *sql.Context, table *Table, row sql.Row) error { | ||
| key := string(table.keys[table.insert]) | ||
| table.insert++ |
There was a problem hiding this comment.
table.insert should be called table.insertPartIdx or similar
memory/table_editor.go
Outdated
| func (k *keylessTableEditAccumulator) insertHelper(ctx *sql.Context, table *Table, row sql.Row) error { | ||
| key := string(table.keys[table.insert]) | ||
| table.insert++ | ||
| if table.insert == len(table.keys) { |
There was a problem hiding this comment.
table.keys should be called table.partitionKeys
sql/analyzer/assign_update_join.go
Outdated
| if jn == nil { | ||
| return n, nil | ||
| if unsupported { | ||
| return nil, sql.ErrUnsupportedFeature.New() |
There was a problem hiding this comment.
Is this still possible? Didn't you remove all the test cases that might trigger this?
|
|
||
| // IndexedJoinSorter is used to sort the output of an IndexedJoin to fit the original join order. Note that this | ||
| // occurs because IndexedJoins use join optimization logic that changes the original given join order. | ||
| type IndexedJoinSorter struct { |
There was a problem hiding this comment.
So I don't like this solution, although you can check it in for the time being as long as you follow up with a fix.
This is basically just a projection (mapping from one column order to another), not a new kind of node that we should have to account for separately everywhere (which will be prone to bugs). I think the only reason this works is that it's applied in the join optimization step, which happens last. Otherwise the new node type would cause issues (lots of case statements to clean up).
So you have two choices: either replace this with a straight projection (easy enough).
Or put the projection into the UpdateJoin node itself (probably better).
But either way, definitely don't introduce a new node type here.
There was a problem hiding this comment.
Yeah 100% makes sense. Will followup with a new pr.
