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.

Sign up for GitHub

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

[optimizer] Reduce MRE::typ() calls in SemijoinIdempotence #30944

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[optimizer] Reduce MRE::typ() calls in SemijoinIdempotence #30944

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fe773bf
save some MRE::typ() calls
mgree Jan 3, 2025
0a3a1a0
push through more savings on MRE::typ()
mgree Jan 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 21 additions & 18 deletions src/transform/src/semijoin_idempotence.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
//! which we will transfer to the columns of `D` thereby forming `C`.

use itertools::Itertools;
use mz_repr::RelationType;
use std::collections::BTreeMap;

use mz_expr::{Id, JoinInputMapper, LocalId, MirRelationExpr, MirScalarExpr, RECURSION_LIMIT};
Expand Down Expand Up @@ -198,6 +199,9 @@ fn attempt_join_simplification(
let input_mapper = JoinInputMapper::new(inputs);

if let Some((ltr, rtl)) = semijoin_bijection(inputs, equivalences) {
// If semijoin_bijection returns `Some(...)`, then `inputs.len() == 2`.
assert_eq!(inputs.len(), 2);

// Collect the `Get` identifiers each input might present as.
let ids0 = as_filtered_get(&inputs[0], gets_behind_gets)
.iter()
Expand All @@ -208,10 +212,12 @@ fn attempt_join_simplification(
.map(|(id, _)| *id)
.collect::<Vec<_>>();

// Record the types of the inputs, for use in both loops below.
let typ0 = inputs[0].typ();
let typ1 = inputs[1].typ();

// Consider replacing the second input for the benefit of the first.
if distinct_on_keys_of(&inputs[1], &rtl)
&& input_mapper.input_arity(1) == equivalences.len()
{
if distinct_on_keys_of(&typ1, &rtl) && input_mapper.input_arity(1) == equivalences.len() {
for mut candidate in list_replacements(&inputs[1], let_replacements, gets_behind_gets) {
if ids0.contains(&candidate.id) {
if let Some(permutation) = validate_replacement(&ltr, &mut candidate) {
Expand All @@ -222,11 +228,11 @@ fn attempt_join_simplification(
// The pushdown is for the benefit of CSE on the `A` expressions,
// in the not uncommon case of nullable foreign keys in outer joins.
// TODO: Discover the transform that would not require this code.
let typ0 = inputs[0].typ().column_types;
let typ1 = inputs[1].typ().column_types;
let mut is_not_nulls = Vec::new();
for (col0, col1) in ltr.iter() {
if !typ1[*col1].nullable && typ0[*col0].nullable {
if !typ1.column_types[*col1].nullable
&& typ0.column_types[*col0].nullable
{
Copy link
Contributor

Choose a reason for hiding this comment

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

In theory, when doing the inputs[1] = candidate.replacement.project(permutation); above, the types might change (e.g., whether we can determine nullability). But this does not seem to be happening in practice: I added an assert comparing the saved type with a fresh typ() call, and ran a bunch of tests (including RQG left-join-stacks), and the assert never fired. So this seems fine!

is_not_nulls.push(MirScalarExpr::Column(*col0).call_is_null().not())
}
}
Expand All @@ -243,9 +249,7 @@ fn attempt_join_simplification(
}
}
// Consider replacing the first input for the benefit of the second.
if distinct_on_keys_of(&inputs[0], &ltr)
&& input_mapper.input_arity(0) == equivalences.len()
{
if distinct_on_keys_of(&typ0, &ltr) && input_mapper.input_arity(0) == equivalences.len() {
for mut candidate in list_replacements(&inputs[0], let_replacements, gets_behind_gets) {
if ids1.contains(&candidate.id) {
if let Some(permutation) = validate_replacement(&rtl, &mut candidate) {
Expand All @@ -256,11 +260,11 @@ fn attempt_join_simplification(
// The pushdown is for the benefit of CSE on the `A` expressions,
// in the not uncommon case of nullable foreign keys in outer joins.
// TODO: Discover the transform that would not require this code.
let typ0 = inputs[0].typ().column_types;
let typ1 = inputs[1].typ().column_types;
let mut is_not_nulls = Vec::new();
for (col1, col0) in rtl.iter() {
if !typ0[*col0].nullable && typ1[*col1].nullable {
if !typ0.column_types[*col0].nullable
&& typ1.column_types[*col1].nullable
{
is_not_nulls.push(MirScalarExpr::Column(*col1).call_is_null().not())
}
}
Expand Down Expand Up @@ -422,7 +426,7 @@ fn list_replacements_join(
// Each unique key could be a semijoin candidate.
// We want to check that the join equivalences exactly match the key,
// and then transcribe the corresponding columns in the other input.
if distinct_on_keys_of(&inputs[1], &rtl) {
if distinct_on_keys_of(&inputs[1].typ(), &rtl) {
let columns = ltr
.iter()
.map(|(k0, k1)| (*k0, *k0, *k1))
Expand Down Expand Up @@ -452,7 +456,7 @@ fn list_replacements_join(
// Each unique key could be a semijoin candidate.
// We want to check that the join equivalences exactly match the key,
// and then transcribe the corresponding columns in the other input.
if distinct_on_keys_of(&inputs[0], &ltr) {
if distinct_on_keys_of(&inputs[0].typ(), &ltr) {
let columns = ltr
.iter()
.map(|(k0, k1)| (*k1, *k0, *k0))
Expand Down Expand Up @@ -484,10 +488,9 @@ fn list_replacements_join(
results
}

/// True iff some unique key of `input` is contained in the keys of `map`.
fn distinct_on_keys_of(expr: &MirRelationExpr, map: &BTreeMap<usize, usize>) -> bool {
expr.typ()
.keys
/// True iff some unique key of `typ` is contained in the keys of `map`.
fn distinct_on_keys_of(typ: &RelationType, map: &BTreeMap<usize, usize>) -> bool {
typ.keys
.iter()
.any(|key| key.iter().all(|k| map.contains_key(k)))
}
Expand Down
Loading