-
Notifications
You must be signed in to change notification settings - Fork 15
REP-6503 Avoid $type in partition queries unless needed #129
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
REP-6503 Avoid $type in partition queries unless needed #129
Conversation
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.
Seems that some comments still need to be updated. LGTM other than those.
@@ -91,7 +91,7 @@ var bsonTypeString = map[bsontype.Type]string{ | |||
// This is kind of like strings.Cut() but against the sort-ordered list of BSON | |||
// types, except that if the given value is a number or string-like, then other | |||
// “like” types will not be in the returned slices. | |||
func getTypeBracketExcludedBSONTypes(val any) ([]string, []string, error) { | |||
func getTypeBracketExcludedBSONTypes(val any) ([]bsontype.Type, []bsontype.Type, error) { |
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 comment for this function still says it returns string slices.
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.
oops! fixed
internal/partitions/partition.go
Outdated
} | ||
|
||
// filterWithExplicitTypeChecks compensates for the server’s type bracketing | ||
// getExplicitTypeCheckPredicates compensates for the server’s type bracketing | ||
// by matching _id types between the partition’s min & max boundaries. | ||
// It should yield the same result as filterWithExpr. |
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.
Note that this comment needs to be updated.
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.
fixed
// < 5.0, will not use indexes and thus will be very slow | ||
func (p *Partition) filterWithExpr() bson.D { | ||
// < 5.0, will not use indexes and thus will be very slow. | ||
func getExprPredicates(lower, upper any) ([]bson.D, error) { |
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.
We're okay with $expr because the query cannot use PROJECTION_COVERED IXSCAN and would need to FETCH anyway?
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.
This changeset addresses a performance problem that surfaced specifically with the non-$expr queries.
If there are improvements to make to the $expr queries we could probably do those separately.
The $type checks that REP-6129 (PR #118) added to fix type bracketing have impeded performance against 4.4 clusters.
This changeset makes partition queries use $type only when the range requires it, which should only be true for a handful of partitions anyhow. The queries are simplified, too, so that instead of ANDing together predicates like
_id >= 12 OR $type in ["string", "symbol", ...])
andid <= 99 OR $type in ["null", "minkey"]
we just have a single $type invocation (if it’s even there, which it usually won’t be now).The existing checks for type-bracketing accommodation are expanded a bit here as well.