This repository was archived by the owner on Jan 28, 2021. It is now read-only.
Fix a bug in index assignment that led to incorrect query results, and add index driver to engine_test #864
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… 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: Daylon Wilkins <[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]>
…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]>
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.
golangcibot
reviewed
Nov 6, 2019
sql/analyzer/assign_indexes_test.go
Outdated
| result, err = assignIndexes(a, node) | ||
| require.NoError(err) | ||
|
|
||
| lookupIdxs, ok = result["t1"] |
There was a problem hiding this comment.
SA4006: this value of lookupIdxs is never used (from staticcheck)
sql/analyzer/assign_indexes_test.go
Outdated
| result, err = assignIndexes(a, node) | ||
| require.NoError(err) | ||
|
|
||
| lookupIdxs, ok = result["t1"] |
There was a problem hiding this comment.
SA4006: this value of lookupIdxs is never used (from staticcheck)
Signed-off-by: Zach Musgrave <[email protected]>
Removed unused variable assignments
…or checking for compatibility Signed-off-by: Zach Musgrave <[email protected]>
… bug: select col1, col2 from table gets mapped as a ResolvedTable, rather than a Project. This means the columns come back in the wrong order. Signed-off-by: Zach Musgrave <[email protected]>
…ect were being inappropriately pruned (a Project turned into a ResolvedTable because none of the columns were referenced by a return). 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]>
Support for "INSERT INTO table1 SELECT * FROM table2" statements.
|
Closing in favor of #865 -- we have continued development on this branch, and this PR is big enough without adding additional features. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.