Zachmu/index or bug fix #22
Conversation
… that cannot be merged together. This partial index usage resulted in pushdown getting an IndexableTable for only one side of the OR expression, returning incorrect results. Signed-off-by: Zach Musgrave <[email protected]>
Signed-off-by: Zach Musgrave <[email protected]>
… using them in engine_test.go to reproduce index-related bugs in SELECT statements Signed-off-by: Zach Musgrave <[email protected]>
…mergeable indexes. Signed-off-by: Zach Musgrave <[email protected]>
Signed-off-by: Zach Musgrave <[email protected]>
…d for DB / table setup. Signed-off-by: Zach Musgrave <[email protected]>
…ementations necessarily need to be coupled, so starting down that path. Signed-off-by: Zach Musgrave <[email protected]>
…un queries with every combination of indexed / parallel / partitioned Signed-off-by: Zach Musgrave <[email protected]>
…s over each in the parent table looking for matches -- not here to make queries faster, just to test if they give correct results when indexes are involved. Signed-off-by: Zach Musgrave <[email protected]>
Signed-off-by: Zach Musgrave <[email protected]>
…test catches the bug Signed-off-by: Zach Musgrave <[email protected]>
Signed-off-by: Zach Musgrave <[email protected]>
…tering the number of partitions for the test setup Signed-off-by: Zach Musgrave <[email protected]>
Signed-off-by: Zach Musgrave <[email protected]>
Signed-off-by: Zach Musgrave <[email protected]>
Signed-off-by: Zach Musgrave <[email protected]>
Signed-off-by: Zach Musgrave <[email protected]>
… and intersect functions Signed-off-by: Zach Musgrave <[email protected]>
Signed-off-by: Zach Musgrave <[email protected]>
Signed-off-by: Zach Musgrave <[email protected]>
Signed-off-by: Zach Musgrave <[email protected]>
Signed-off-by: Zach Musgrave <[email protected]>
Signed-off-by: Zach Musgrave <[email protected]>
Signed-off-by: Zach Musgrave <[email protected]>
…ion for between operation) Signed-off-by: Zach Musgrave <[email protected]>
Signed-off-by: Zach Musgrave <[email protected]>
…l the pieces together Signed-off-by: Zach Musgrave <[email protected]>
Signed-off-by: Zach Musgrave <[email protected]>
…ilures (probably a mistake in the index Values() implementation) Signed-off-by: Zach Musgrave <[email protected]>
… test failures Signed-off-by: Zach Musgrave <[email protected]>
Signed-off-by: Zach Musgrave <[email protected]>
… to index structs, introduced some new helper functions to cut down on verbosity of test literals Signed-off-by: Zach Musgrave <[email protected]>
Signed-off-by: Zach Musgrave <[email protected]>
Signed-off-by: Zach Musgrave <[email protected]>
Signed-off-by: Zach Musgrave <[email protected]>
Signed-off-by: Zach Musgrave <[email protected]>
Signed-off-by: Zach Musgrave <[email protected]>
Signed-off-by: Zach Musgrave <[email protected]>
|
@Hydrocharged , please give this a quick sanity check. I know it's big, so don't worry about verifying every line. But let me know if there are any glaring errors I should fix before sending this to src-d. |
…ummy from the name. Moved a few interface assertions left over from assignindex_test into the type files. Signed-off-by: Zach Musgrave <[email protected]>
memory/mergeable_index.go
Outdated
| } | ||
|
|
||
| func (m *MergedIndexLookup) Indexes() []string { | ||
| panic("implement me") |
There was a problem hiding this comment.
Assuming we aren't implementing these just yet?
There was a problem hiding this comment.
Never going to be implemented -- this method only matters for a primary index, so that the engine can match columns to indexes. For this synthetic index, that never happens.
There was a problem hiding this comment.
Ah gotcha. I'd change the panic message then in that case.
| } | ||
|
|
||
| func (m *MergedIndexLookup) Difference(...sql.IndexLookup) sql.IndexLookup { | ||
| panic("not implemented") |
There was a problem hiding this comment.
Further down we have panic("negateIndexLookup.Difference is not implemented"), would seem a bit more consistent if this was also panic("mergedIndexLookup.Difference is not implemented"), but this is about a 0.1% on the importance scale lol
There was a problem hiding this comment.
Good call, I'll fix these.
| return float64(val), sql.Float64 | ||
| case string: | ||
| return val, sql.Text | ||
| default:panic(fmt.Sprintf("Unsupported type for %v of type %T", val, val)) |
There was a problem hiding this comment.
Hard to see from GitHub where this is all used and the impact it would have, but this is converting everything to 64-bit precision and returning the associated SQL type. I think it will but I wanna make sure: this will only be used from the index driver right? As in any value passed in won't make it's way into an insertion?
I ask in the case of a column expecting an int16 but receiving an int64 for storage. There may be a benefit to working with specific types here that I'm not seeing since it's hard to gather the bigger picture from here (like that comparing two ints of different sizes thing you ran into before), but I wanna check.
There was a problem hiding this comment.
Yeah, this is just for comparison purposes for SELECT statements. Using the widest possible numeric type makes the comparison more straightforward.
There was a problem hiding this comment.
Okay, just wanted to double check!
| @@ -368,7 +368,7 @@ func TestDate(t *testing.T) { | |||
| {"null date", sql.NewRow(nil), nil, false}, | |||
| {"invalid type", sql.NewRow([]byte{0, 1, 2}), nil, false}, | |||
| {"date as string", sql.NewRow(stringDate), "2007-01-02", false}, | |||
| {"date as time", sql.NewRow(time.Now()), time.Now().Format("2006-01-02"), false}, | |||
| {"date as time", sql.NewRow(time.Now().UTC()), time.Now().UTC().Format("2006-01-02"), false}, | |||
Signed-off-by: Zach Musgrave <[email protected]>
Fixed a bug in indexing code that would cause queries to return incorrect results. Along the way, implemented index capabilities for the in-memory tables (only for correctness verification), and expanded engine_test to test every combination of indexes, partitions, and parallelism.