Remove Tables from query plans #912
Merged
Conversation
This file contains hidden or 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
Rework `SourceExpr` to be a logical placeholder for a table, rather than a table itself. Make query eval functions take an additional argument, a set of tables. When evaluating a `SourceExpr` to a table, they will treat the `SourceExpr` as a reference into the set of sources, and use the referred table. This commit modifies only the VM crate; modifications to `core` are forthcoming. It's possible that this commit's scheme for referring to `SourceExpr`s will need to change, as currently it forbids duplicate `SourceExpr`s, which I think might occur during index joins.
gefjon
force-pushed
the
phoebe/remove-memtable-from-plans
branch
from
4281c06 to
8066ac8
Compare
Per review from Joshua, this commit makes `SourceExpr` into an enum similar to the previous definition, with a `DbTable(DbTable)` variant. Indirection to a `SourceSet` is imposed only for the `MemTable` variant. This sould make the PR's overall diff much simpler (assuming I haven't inadvertently made any changes in the process of reverting the `DbTable` code paths). Related to the above, this PR simplifies `SourceSet`. `SourceSet` now holds a `Vec<Option<MemTable>>`, where previously it was a transparent newtype around `[Option<Table>]`. This change eliminates the need for unsafe unsized conversions, removes `SourceBuilder`, and causes `SourceSet` to be uniformly consumed by the high-level query eval operators, where previously `SourceSet`s had to be semi-reusable because they could contain `DbTable`s.
Centril
requested changes
Mar 4, 2024
joshua-spacetime
approved these changes
Mar 4, 2024
Centril
approved these changes
Mar 4, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description of Changes
Alter our representation of query expressions and compiled query plans so that they no longer contain
MemTable. Instead,SourceExpr::MemTablean identifier for aMemTablewithin aSourceSet.Storing
DbTablewithin query plans is unchanged.API and ABI breaking changes
N/a.
Expected complexity level and risk
3 - the query engine has changed substantially, in ways that may inadvertently change behavior.
How complicated do you think these changes are? Grade on a scale from 1 to 5,
where 1 is a trivial change, and 5 is a deep-reaching and complex change.
This complexity rating applies not only to the complexity apparent in the diff,
but also to its interactions with existing and future code.
If you answered more than a 2, explain what is complex about the PR,
and what other components it interacts with in potentially concerning ways.