BREAKING CHANGE: split aggregate arguments by space rather than \s+#16145
BREAKING CHANGE: split aggregate arguments by space rather than \s+#16145
Conversation
There was a problem hiding this comment.
Pull request overview
This PR optimizes Aggregate.prototype.project() and Aggregate.prototype.sort() string argument parsing by replacing regex-based splitting (/\s+/) with plain space splitting (' '), consistent with the projection parsing optimization discussed in #16126.
Changes:
- Update
project()to split string arguments using' 'with a no-space fast path. - Update
sort()to split string arguments using' 'with a no-space fast path. - Refactor object-argument iteration in
project()to use afor...ofloop.
| } else { | ||
| const include = arg[0] === '-' ? 0 : 1; | ||
| if (include === 0) { | ||
| field = field.substring(1); | ||
| arg = arg.substring(1); | ||
| } | ||
| fields[field] = include; | ||
| }); | ||
| fields[arg] = include; | ||
| } |
There was a problem hiding this comment.
The new no-space fast path treats an empty string ('') as a field name and produces { $project: { '': 1 } }, whereas the previous split(/\s+/) logic would ignore empty tokens and result in an empty projection object. This is a behavior change that will generate invalid pipeline stages for project('') (and similarly for strings that become empty after trimming). Consider normalizing to the same tokenization approach as parseProjection() (wrap in an array when there are no spaces, then skip falsy tokens) or explicitly handling arg === '' before computing include.
There was a problem hiding this comment.
I agree, this should likely have a !arg check as the other branch.
AbdelrahmanHafez
left a comment
AbdelrahmanHafez
left a comment
There was a problem hiding this comment.
Nitpicky suggestion:
Add a test that asserts other types of whitespace aren't used to split.
hasezoey
left a comment
hasezoey
left a comment
There was a problem hiding this comment.
Mostly looks good to me, but Copilout points out that empty strings are treated as a field, which likely should not be the case.
Also consider adding "other whitespace character" tests as Hafez pointed out.
| } else { | ||
| const include = arg[0] === '-' ? 0 : 1; | ||
| if (include === 0) { | ||
| field = field.substring(1); | ||
| arg = arg.substring(1); | ||
| } | ||
| fields[field] = include; | ||
| }); | ||
| fields[arg] = include; | ||
| } |
There was a problem hiding this comment.
I agree, this should likely have a !arg check as the other branch.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Fix #16126
Summary
Splitting by \s+ is considerably slower than just plain
' 'so we should switch to that.Examples