go-mysql-server icon indicating copy to clipboard operation
go-mysql-server copied to clipboard

Ld index bugs

Open zachmu opened this issue 6 years ago • 2 comments

In testing out indexing capability with our product's custom IndexDriver, I found a bug. The following query would incorrectly return only a single result on an indexed column for an IndexLookup that didn't implement the Mergeable interface:

SELECT col1 from table where col1 = 1 or col1 = 2;

When the two index lookups couldn't be merged, the first one was returned as the sole lookup for that table. This lookup then got pushed down to the table by the analyzer.

Verifying that this bug was actually fixed ended up being pretty difficult, because engine_test doesn't use an IndexDriver. So I moved the test index types out of assign_indexes_test.go into the memory package, then implemented the Values() method for them so that they could return matching rows from the memory tables. Then I implemented an IndexDriver for the memory package which can be seeded by hand for tests. Finally, I changed TestQueries to test every combination of partitions, index driver, and parallelism, for a total of 12 runs.

The biggest change here is how MergedIndexLookups are handled in the assign_index_tests.go file. What I have now is (I think) more understandable and correct than the previous method of tracking unions and intersections, but did substantially increase the amount of boilerplate in the test code.

Finally, I fixed an unrelated bug in time_test.go, which would fail in my timezone after 4pm due to timezone differences in date calculation.

zachmu avatar Nov 08 '19 01:11 zachmu

Hi friends at src-d,

Can we get some eyeballs on these outstanding pull requests? We have a bunch of additional changes we've been building on top of these (including work on views based on https://github.com/src-d/go-mysql-server/pull/860) and it's going to become difficult contribute them back if these sit unmerged much longer.

zachmu avatar Nov 21 '19 17:11 zachmu

Any chance we can get this reviewed soon? We are continuing to stack changes on top of this one, and it's going to be really challenging to merge them back to you at this point.

zachmu avatar Dec 05 '19 22:12 zachmu