Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug fix: IndexedDoltTable didn't fully implement DoltTable's interface #7490

Merged
merged 2 commits into from
Feb 14, 2024

Conversation

fulghum
Copy link
Contributor

@fulghum fulghum commented Feb 14, 2024

IndexedDoltTable wasn't fully compatible with DoltTable's interface, which was causing some queries to execute incorrectly due to using the wrong field indexes. This only happened with queries where the table was used in a read-only context (such as an AS OF query), since in a read-write context WritableIndexedDoltTable is used, which is composed of a WritableDoltTable, so fully inherits the interface. IndexedDoltTable is now consistent with that, and is now composed of a DoltTable instance. This enables the GMS code in assignExecIndexes to use the correct, limited schema of the secondary index and to apply the correct field indexes.

Fixes: #7488

@coffeegoddd
Copy link
Contributor

@fulghum DOLT

comparing_percentages
99.999562 to 99.999562
version result total
fd8fcd5 not ok 26
fd8fcd5 ok 5937431
version total_tests
fd8fcd5 5937457
correctness_percentage
99.999562

Copy link
Contributor

@max-hoffman max-hoffman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, the source of this bug here is that table pruning does not record certain metadata on the GMS side. We currently expect table sources to be able to tell us both (1) all of the logical columns in the table, and (2) what columns will actually be returned (projected) at execution time. Ideally pruning columns would shift some of this burden for execution indexing into the engine. PrimaryKeySchema is maybe a misleading name for the way this interface is used here. We could have something like LogicalSchema as a subinterface separate from the PkOrdinals() method. As long as there are other places where we depend on PrimaryKeySchema for the logical/execution schema difference, having table sources implement the interface for that info will be good.

@fulghum fulghum merged commit 2ff6bc6 into main Feb 14, 2024
21 of 22 checks passed
@fulghum fulghum deleted the fulghum/dolt-7488 branch February 14, 2024 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Field index bug with AS OF query and secondary index
4 participants