Distinguish between inner and semijoins in QueryExpr AST.
#969
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
This commit adds a flag `semi: bool` to `JoinExpr`, which signifies a semijoin, as opposed to an inner join. A new optimization pass, `QueryExpr::try_semi_join`, is defined which can detect a certain common case of inner joins and rewrite them into semijoins. The punchline here is that `core::vm::join_inner` used to accept a flag `semi: bool` which it could use to avoid some expensive `Header` mutations, but that flag was always passed as `false` because we had no way to distinguish semijoins. With this commit, the flag is actually used, so evaluating non-indexed semijoins should avoid allocating a new `Header`.
| let mut sources = SourceSet::default(); | ||
| let rhs_source_expr = sources.add_mem_table(data); | ||
|
|
||
| let q = query(&schema).with_join_inner(rhs_source_expr, FieldName::positional("inventory", 0), rhs, true); |
There was a problem hiding this comment.
I think I would prefer a slightly higher level test that goes through the entire optimizer.
There was a problem hiding this comment.
As you pointed out, we already have test coverage of this part, so these are redundant. Feel free to remove.
| let source_expr = sources.add_mem_table(table.clone()); | ||
| let second_source_expr = sources.add_mem_table(table); | ||
|
|
||
| let q = query(source_expr).with_join_inner(second_source_expr, field.clone(), field, true); |
There was a problem hiding this comment.
Again, I think I would prefer a higher level test that goes from sql to result set. You can use RelationalDB::create_table_for_test along with sql::execute::run.
joshua-spacetime
requested changes
Mar 14, 2024
crates/vm/src/expr.rs
Outdated
|
|
||
| let q = QueryExpr { | ||
| source: lhs_source, | ||
| // Build the query manually, because `.with_select` will attempt to push selections before the join. |
There was a problem hiding this comment.
This is unfortunate. All of this should be included as part of optimize. But of course this is a preexisting issue.
|
But I should add that this change does improve the performance of incremental join exactly as expected. |
- Remove a test that was silly and backwards, and intentionally thwarted the optimizer in a way that will hopefully stop working soon. - Add a test that an `IncrementalJoin`'s `virtual_plan` looks like we expect. - Rename the `JoinExpr` argument to `core::vm::join_inner` for clarity. - Sprinkle comments around about how we compile and optimize joins.
joshua-spacetime
approved these changes
Mar 14, 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
This commit adds a flag
semi: booltoJoinExpr, which signifies a semijoin, as opposed to an inner join.A new optimization pass,
QueryExpr::try_semi_join, is defined which can detect a certain common case of inner joins and rewrite them into semijoins.The punchline here is that
core::vm::join_innerused to accept a flagsemi: boolwhich it could use to avoid some expensiveHeadermutations, but that flag was always passed asfalsebecause we had no way to distinguish semijoins. With this commit, the flag is actually used,so evaluating non-indexed semijoins should avoid allocating a new
Header.API and ABI breaking changes
N/a
Expected complexity level and risk
3 - our query planner and evaluator depend strongly (more strongly than they should) on specific parses and representations in some places. I believe I found all such places and changed them, but am not confident.