-
Notifications
You must be signed in to change notification settings - Fork 110
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
perf: Use type ids for type checking #1725
Conversation
|
||
/// Add a type to the context and return a [TyId] for it. | ||
/// The [TyId] is not guaranteed to be unique to the type. | ||
/// However for primitive types it will be. |
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 any problem with making it unique?
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.
No problem, it just requires more memory and it becomes a little more expensive to generate type ids. But the benefit is that we only need type ids to test for equality.
crates/planner/src/logical/ty.rs
Outdated
(BinOp::And | BinOp::Or, Type::Alg(AlgebraicType::Bool)) => true, | ||
// Comparison operators take integers or floats | ||
(BinOp::Lt | BinOp::Gt | BinOp::Lte | BinOp::Gte, Type::Alg(t)) => t.is_integer() || t.is_float(), | ||
// Equality supports numerics, strings, and bytes |
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.
Why can't the others?
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.
Mainly to avoid inconsistencies with other type systems. In the case of Rust, sums and products correspond to enum and struct types. Those types could have custom Eq and Ord implementations and as of right now, we don't have a way of representing that in SpacetimeDB.
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
bb87e04
to
7a35760
Compare
7a35760
to
206c618
Compare
A `RelExpr` now holds type ids instead of types. This reduces the size of `RelExpr`. It avoids copying and cloning types during type checking. Comparisons are also cheaper for primitive types.
206c618
to
6233a33
Compare
A
RelExpr
now holds type ids instead of types.This reduces the size of
RelExpr
.It avoids copying and cloning types during type checking. Comparisons are also cheaper for primitive types.