-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Rust: Type inference refactor and improve join orders #20076
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
Rust: Type inference refactor and improve join orders #20076
Conversation
…` are in `relevantConstraint`
… and factor out `multipleConstraintImplementations`
c00cf2f to
43b2977
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.
Pull Request Overview
This PR refactors the Rust type inference library to improve join orders and reduce computational overhead. The changes focus on optimizing predicate evaluation and streamlining constraint satisfaction logic.
Key changes:
- Extracted
multipleConstraintImplementationspredicate to improve reusability and join ordering - Moved
hasTypeConstraintpredicate definition earlier for better join optimization - Simplified
typeParameterConstraintHasTypeParameterpredicate by reducing unnecessary joins and intermediate variables
| ) | ||
| or | ||
| tt.getTypeAt(TypePath::nil()) = constraint and | ||
| hasTypeConstraint(tt, constraint, constraint) and |
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.
The crux here is hat hasTypeConstraint includes relevantConstraint. Before that this disjunct allowed constraint to be anything tt.getTypeAt(TypePath::nil()) which caused the relation to contain a lot of junk.
geoffw0
left a comment
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.
LGTM, one question.
| Access a, Declaration target, TypePath path, Type t, TypeParameter tp | ||
| ) { | ||
| not exists(getTypeArgument(a, target, tp, _)) and | ||
| target = a.getTarget() and |
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.
What has happened to this constraint?
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.
Good question. It's implied by AccessConstraint::satisfiesConstraintType(a, target, ...) so including it here didn't add anything.
This PR makes a few refactors to the type inference library.
I think these changes make sense on their own, but on
databendthey also reduce the total number of tuples (observed using "Compare Performance" in VSCode) and the following bad join reported by VScode (also observed ondatabend) disappears:Bad join
DCA is uneventful and don't show any change in analysis time 😢