Skip to content
This repository has been archived by the owner on Jan 28, 2021. It is now read-only.

Ld index bugs #865

Open
wants to merge 44 commits into
base: master
Choose a base branch
from
Open

Ld index bugs #865

wants to merge 44 commits into from

Conversation

zachmu
Copy link
Contributor

@zachmu zachmu commented Nov 8, 2019

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 and others added 30 commits October 28, 2019 13:59
… 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 <zach@liquidata.co>
Signed-off-by: Zach Musgrave <zach@liquidata.co>
… using them in engine_test.go to reproduce index-related bugs in SELECT statements

Signed-off-by: Zach Musgrave <zach@liquidata.co>
…mergeable indexes.

Signed-off-by: Zach Musgrave <zach@liquidata.co>
Signed-off-by: Zach Musgrave <zach@liquidata.co>
…d for DB / table setup.

Signed-off-by: Zach Musgrave <zach@liquidata.co>
…ementations necessarily need to be coupled, so starting down that path.

Signed-off-by: Zach Musgrave <zach@liquidata.co>
…un queries with every combination of indexed / parallel / partitioned

Signed-off-by: Zach Musgrave <zach@liquidata.co>
…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 <zach@liquidata.co>
Signed-off-by: Zach Musgrave <zach@liquidata.co>
…test catches the bug

Signed-off-by: Zach Musgrave <zach@liquidata.co>
Signed-off-by: Zach Musgrave <zach@liquidata.co>
…tering the number of partitions for the test setup

Signed-off-by: Zach Musgrave <zach@liquidata.co>
Signed-off-by: Zach Musgrave <zach@liquidata.co>
Signed-off-by: Zach Musgrave <zach@liquidata.co>
Signed-off-by: Zach Musgrave <zach@liquidata.co>
Signed-off-by: Zach Musgrave <zach@liquidata.co>
… and intersect functions

Signed-off-by: Zach Musgrave <zach@liquidata.co>
Signed-off-by: Daylon Wilkins <daylon@liquidata.co>
Signed-off-by: Zach Musgrave <zach@liquidata.co>
Signed-off-by: Zach Musgrave <zach@liquidata.co>
Signed-off-by: Zach Musgrave <zach@liquidata.co>
Signed-off-by: Zach Musgrave <zach@liquidata.co>
Signed-off-by: Zach Musgrave <zach@liquidata.co>
Signed-off-by: Zach Musgrave <zach@liquidata.co>
…ion for between operation)

Signed-off-by: Zach Musgrave <zach@liquidata.co>
Signed-off-by: Zach Musgrave <zach@liquidata.co>
…l the pieces together

Signed-off-by: Zach Musgrave <zach@liquidata.co>
Signed-off-by: Zach Musgrave <zach@liquidata.co>
…ilures (probably a mistake in the index Values() implementation)

Signed-off-by: Zach Musgrave <zach@liquidata.co>
zachmu added 14 commits November 4, 2019 16:33
… test failures

Signed-off-by: Zach Musgrave <zach@liquidata.co>
Signed-off-by: Zach Musgrave <zach@liquidata.co>
… to index structs, introduced some new helper functions to cut down on verbosity of test literals

Signed-off-by: Zach Musgrave <zach@liquidata.co>
Signed-off-by: Zach Musgrave <zach@liquidata.co>
Signed-off-by: Zach Musgrave <zach@liquidata.co>
Signed-off-by: Zach Musgrave <zach@liquidata.co>
Signed-off-by: Zach Musgrave <zach@liquidata.co>
Signed-off-by: Zach Musgrave <zach@liquidata.co>
Signed-off-by: Zach Musgrave <zach@liquidata.co>
…ummy from the name. Moved a few interface assertions left over from assignindex_test into the type files.

Signed-off-by: Zach Musgrave <zach@liquidata.co>
Signed-off-by: Zach Musgrave <zach@liquidata.co>
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.
Signed-off-by: Zach Musgrave <zach@liquidata.co>
@zachmu
Copy link
Contributor Author

zachmu commented Nov 21, 2019

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 #860) and it's going to become difficult contribute them back if these sit unmerged much longer.

@zachmu
Copy link
Contributor Author

zachmu commented Dec 5, 2019

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.

@Hydrocharged Hydrocharged deleted the ld-index-bugs branch December 20, 2019 22:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants