-
Notifications
You must be signed in to change notification settings - Fork 468
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
Equivalence propagation #24155
Equivalence propagation #24155
Conversation
903d306
to
0f00775
Compare
da98e1e
to
b7b45ca
Compare
This PR introduces an `Analysis` trait that is meant to be a simplification of the `Attribute` trait/framework. It is simpler in many ways, and less expressive, but seemingly the restrictions are not yet limiting and are potentially helpful in broadening the uptake of the framework. The main idea is that an `Analysis` implementor specifies an output type it will produce for each expression, and the logic that produces this output given an expression, analysis results for child expressions, and analysis results for other depended-upon analyses. All analysis state is indexed by "offset": the post-order visitation numbering of the expressions. The framework runs the analyses in dependency order, establishes links between `Let`-bound identifiers and offsets, but otherwise has very few opinions. By contrast, the Attribute framework allowed its hosted attributes to propose various bits of logic that would run in addition as it traverses an expression, used to set up `Let`-binding information but also potentially much more. There is a demonstration of how to use the framework in #24155, but we are looking at this PR in isolation to break up the moving parts of that PR into manageable pieces. 1. The main annoyance in using the output is that one must keep notes as one descends into an expression about the current expression offsets, as very little help is currently offered here (though, a `Slice` variant of the results that scopes itself down to expressions, and their child expressions, would probably be helpful). 2. There is a fair amount of cloning of results, even though many expressions only wish to say "whatever my only child says". One could imagine an analysis result type of `Result<AnalysisType, usize>` where the `Err(offset)` variant might indicate an offset to consult for the state (and expressions that don't modify the analysis result could have `Err(offset)` for some appropriate `offset` (their child, or whereever their child references). The main reviewing question is whether this simplification is net valuable, in that it makes things easier to use and adopt, without ruling out too much expressivity or potentially performance. Additionally, I have not yet begun to integrate it further than the previous PR, but I'm happy to follow up with commits that do this if we identify where we could most fruitfully slice out the existing uses. I put together prototype versions for all existing attributes other than `Cardinality` and `ColumnNames`, as they each had a fair bit of inherent complexity. ### Motivation <!-- Which of the following best describes the motivation behind this PR? * This PR fixes a recognized bug. [Ensure issue is linked somewhere.] * This PR adds a known-desirable feature. [Ensure issue is linked somewhere.] * This PR fixes a previously unreported bug. [Describe the bug in detail, as if you were filing a bug report.] * This PR adds a feature that has not yet been specified. [Write a brief specification for the feature, including justification for its inclusion in Materialize, as if you were writing the original feature specification.] * This PR refactors existing code. [Describe what was wrong with the existing code, if it is not obvious.] --> ### Tips for reviewer <!-- Leave some tips for your reviewer, like: * The diff is much smaller if viewed with whitespace hidden. * [Some function/module/file] deserves extra attention. * [Some function/module/file] is pure code movement and only needs a skim. Delete this section if no tips. --> ### Checklist - [ ] This PR has adequate test coverage / QA involvement has been duly considered. - [ ] This PR has an associated up-to-date [design doc](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/design/README.md), is a design doc ([template](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/design/00000000_template.md)), or is sufficiently small to not require a design. <!-- Reference the design in the description. --> - [ ] If this PR evolves [an existing `$T ⇔ Proto$T` mapping](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/command-and-response-binary-encoding.md) (possibly in a backwards-incompatible way), then it is tagged with a `T-proto` label. - [ ] If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label ([example](MaterializeInc/cloud#5021)). <!-- Ask in #team-cloud on Slack if you need help preparing the cloud PR. --> - [ ] This PR includes the following [user-facing behavior changes](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/guide-changes.md#what-changes-require-a-release-note): - <!-- Add release notes here or explicitly state that there are no user-facing behavior changes. -->
fd8fc76
to
e7e33ec
Compare
f63ad17
to
779a6d9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll continue reading the code tomorrow, but I'm sending my comments so far.
classes: Vec::new(), | ||
}; | ||
for class1 in self.classes.iter() { | ||
for class2 in other.classes.iter() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add some limit here that just gives up if self.classes.len() * other.classes.len()
is too large? (I've already seen a join in a customer's MIR plan with 196 equivalences...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm conflicted on this, given the status of the codebase and the struggles with predictability of transforms. If we can figure out a principled way to do this that doesn't fight with idempotence, that doesn't make the outer fixpoint iteration brittle, etc., then yes! But if it's "don't do something quadratic in the input" then .. we have larger problems. :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, looking at the code there is a fine linear time implementation (move to HashSet
s for intersection). Does that work for you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we'll never run into the limit. How about you add a soft_assert_or_log
? And then CI would fail if we run into the limit, and in production we'd get a Sentry error. And then if it never happens then everyone is happy, and if it happens then we can figure out the next steps (increasing the limit, optimizing the code, ignoring the problem if it doesn't look dangerous that the optimization was not applied in the specific situation, ...).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I've only now seen your comment about using HashSet
s. I'm not sure how to do it with HashSet
s, but a possible approach would be to have a HashMap
that maps from expressions to indexes in classes
(relying on the fact that minimize
makes it so that each expression can appear in only one class). And a similar thing should also work for my other similar comment.
// They stop being sorted as soon as we make any modification, though. | ||
// But, it would be a fast rejection when faced with lots of data. | ||
for index1 in 0..self.classes.len() { | ||
for index2 in 0..index1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as in union
: could you please add a safety limit?
Even with the above E.g.:
|
This PR introduces an `Analysis` trait that is meant to be a simplification of the `Attribute` trait/framework. It is simpler in many ways, and less expressive, but seemingly the restrictions are not yet limiting and are potentially helpful in broadening the uptake of the framework. The main idea is that an `Analysis` implementor specifies an output type it will produce for each expression, and the logic that produces this output given an expression, analysis results for child expressions, and analysis results for other depended-upon analyses. All analysis state is indexed by "offset": the post-order visitation numbering of the expressions. The framework runs the analyses in dependency order, establishes links between `Let`-bound identifiers and offsets, but otherwise has very few opinions. By contrast, the Attribute framework allowed its hosted attributes to propose various bits of logic that would run in addition as it traverses an expression, used to set up `Let`-binding information but also potentially much more. There is a demonstration of how to use the framework in #24155, but we are looking at this PR in isolation to break up the moving parts of that PR into manageable pieces. 1. The main annoyance in using the output is that one must keep notes as one descends into an expression about the current expression offsets, as very little help is currently offered here (though, a `Slice` variant of the results that scopes itself down to expressions, and their child expressions, would probably be helpful). 2. There is a fair amount of cloning of results, even though many expressions only wish to say "whatever my only child says". One could imagine an analysis result type of `Result<AnalysisType, usize>` where the `Err(offset)` variant might indicate an offset to consult for the state (and expressions that don't modify the analysis result could have `Err(offset)` for some appropriate `offset` (their child, or whereever their child references). The main reviewing question is whether this simplification is net valuable, in that it makes things easier to use and adopt, without ruling out too much expressivity or potentially performance. Additionally, I have not yet begun to integrate it further than the previous PR, but I'm happy to follow up with commits that do this if we identify where we could most fruitfully slice out the existing uses. I put together prototype versions for all existing attributes other than `Cardinality` and `ColumnNames`, as they each had a fair bit of inherent complexity. ### Motivation <!-- Which of the following best describes the motivation behind this PR? * This PR fixes a recognized bug. [Ensure issue is linked somewhere.] * This PR adds a known-desirable feature. [Ensure issue is linked somewhere.] * This PR fixes a previously unreported bug. [Describe the bug in detail, as if you were filing a bug report.] * This PR adds a feature that has not yet been specified. [Write a brief specification for the feature, including justification for its inclusion in Materialize, as if you were writing the original feature specification.] * This PR refactors existing code. [Describe what was wrong with the existing code, if it is not obvious.] --> ### Tips for reviewer <!-- Leave some tips for your reviewer, like: * The diff is much smaller if viewed with whitespace hidden. * [Some function/module/file] deserves extra attention. * [Some function/module/file] is pure code movement and only needs a skim. Delete this section if no tips. --> ### Checklist - [ ] This PR has adequate test coverage / QA involvement has been duly considered. - [ ] This PR has an associated up-to-date [design doc](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/design/README.md), is a design doc ([template](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/design/00000000_template.md)), or is sufficiently small to not require a design. <!-- Reference the design in the description. --> - [ ] If this PR evolves [an existing `$T ⇔ Proto$T` mapping](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/command-and-response-binary-encoding.md) (possibly in a backwards-incompatible way), then it is tagged with a `T-proto` label. - [ ] If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label ([example](MaterializeInc/cloud#5021)). <!-- Ask in #team-cloud on Slack if you need help preparing the cloud PR. --> - [ ] This PR includes the following [user-facing behavior changes](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/guide-changes.md#what-changes-require-a-release-note): - <!-- Add release notes here or explicitly state that there are no user-facing behavior changes. -->
8eac2db
to
5aeedfc
Compare
(_, MirScalarExpr::Literal(_, _)) => std::cmp::Ordering::Greater, | ||
(MirScalarExpr::Column(_), MirScalarExpr::Column(_)) => e1.cmp(e2), | ||
(MirScalarExpr::Column(_), _) => std::cmp::Ordering::Less, | ||
(_, MirScalarExpr::Column(_)) => std::cmp::Ordering::Greater, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a comment here mentioning that the first elements of the classes are the representatives, and these ordering considerations are important because we'll be simplifying expressions to these representatives?
self.classes.dedup(); | ||
} | ||
|
||
/// Update `self` to maintain the same equivalences which potentially reducing along `Ord::le`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whose Ord::le
is this sentence referring to?
// TODO: remove these measures once we are more confident about idempotence. | ||
let prev = self.clone(); | ||
self.minimize_once(columns); | ||
assert_eq!(self, &prev); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copying here from Slack:
I'd suggest to leave the idempotence assertion in the code (as a soft_assert_or_log
) until we test this on customer queries.
I've started a Nightly CI run on the current version: https://buildkite.com/materialize/nightlies/builds/6753 Alex says he'll run RQG tomorrow. He will also add a feature flag as discussed here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After the latest fix I couldn't find any more correctness issues.
I suggest to wait for me to rebase and push a commit that add the transform behind a feature flag and enables it in CI before we merge this PR. This can be done immediate after #25628 and #25812 are merged, so probably later today.
@def-: you'll have to ping me next week if the SQLSmith / RQG failures due to OOMs become too frequent. Once we have good example queries I can try tracing with and without the feature flag turned on to see what (if anything) is causing the extra memory consumtion.
/// The mutations should never invalidate an equivalence the operator has been reported as providing, as that | ||
/// information may have already been acted upon by others. | ||
/// | ||
/// The `expr_index` argument must equal `expr`s position in post-order, so that it can be used as a reference |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no expr_index
mentioned else in this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to fix, but maybe after it's in a state that you like / it gets merged. Mostly want to avoid randomizing things with my primitive git
skills.
e36b409
to
35ea714
Compare
MitigationsCompleting required mitigations increases Resilience Coverage.
Risk Summary:The risk associated with this pull request is high, with a score of 83, indicating a significant likelihood of introducing bugs. This assessment is informed by factors such as the average line count in files and the number of executable lines within files, which historically have made pull requests 141% more likely to cause a bug compared to the baseline. Additionally, the pull request modifies files that have a recent history of bug fixes. The repository's observed bug trend is currently decreasing, which is separate from the risk score but suggests an improvement in the overall code stability. Note: The risk score is not based on semantic analysis but on historical predictors of bug occurrence in the repository. The attributes above were deemed the strongest predictors based on that history. Predictors and the score may change as the PR evolves in code, time, and review activity. Bug Hotspots:
|
We discussed with Gábor that further change requests can be addressed in a follow up PR since we are merging this behind a feature flag that is off by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adapter code LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a parser test we can add for these changes in https://github.com/MaterializeInc/materialize/tree/main/src/sql-parser/tests/testdata
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jkosh44 I think you are flagged when I force-pushed my feature flag commits, because in those I extended the parser in order to allow overriding the new feature flag at the SQL level in EXPLAIN WITH(...)
and CREATE CLUSTER ... FEATURES(...)
.
We already have parser tests for this syntax. I don't think it's worth it to add specific specific tests for each new feature flag that we define.
We might want to improve the ergonomics of building a vector of transforms with optional feature flags with a macro.
Export the public HTTP ports of the compared MZ containers in order to allow running https://optimizer-trace.dev.materialize.com/ against them.
35ea714
to
90575e5
Compare
Temporary set the default value to `true` in order to make all of the failing `*.td` tests happy.
Transformation that propagates "equivalence" information, of the form
expr1 = expr2
under Rust's=
. This allows us to communicate arbitrary predicates (expr = true
) but also compactly represent normalizing equivalences that equate classes of expressions (and allow substitution by the simplest representatives).At the moment, it optimizes away queries like
but should be more fully explored to see what it does to expressions, and whether it mis-optimizes any. This PR is meant to read out any test failures in that regard.
The longer term goal is to consolidate places where we try and reason about "equivalence" in ad-hoc manners. There are several transforms that overlap with this reasoning, and several of them could be simplified, improved, or potentially just removed. They include:
Get
nodes to house predicates, and provide guarantees for users)typ()
provides)Filter
-local predicate simplification that now extends further)The main gotcha at the moment is that several of the above perform light physical plan modification as they go. For example
LiteralLifting
projects away any lifted columns, in addition to highlighting the information that the column is a literal. To do this it goes through a bunch of permutation pain, but it ends up with an expression that for example removes unit join terms, where without the projection they are a column containing unread data that we don't know how to prune (the code was never written).Motivation
Tips for reviewer
Checklist
$T ⇔ Proto$T
mapping (possibly in a backwards-incompatible way), then it is tagged with aT-proto
label.