-
Notifications
You must be signed in to change notification settings - Fork 70
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
MCOL-5852: GROUP_CONCAT memory accounting and disk based version #3425
base: develop
Are you sure you want to change the base?
Conversation
Use a boolean flag and a couple of `if`s
cfb6748
to
467bf27
Compare
I'll add some tests a bit later |
6f495c5
to
d328a94
Compare
/* buf.fCurOutPtr points to the data to send; ByteStream guarantees that there | ||
are at least 12 bytes before that for the magic & length fields */ | ||
realBuf = (uint32_t*)msg.buf(); | ||
realBuf -= 3; | ||
realBuf[0] = magic; | ||
realBuf[1] = msglen; | ||
realBuf[2] = longStrings.size(); |
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 suppose you can remove this.
bs >> fTimeZone; | ||
} | ||
|
||
class OrderByRow |
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.
Name overlap with the class that is used by ORDER BY, so you should name it differently IMHO.
After this patch we can move both this sorting structs together with idborderby* into a specialized dir for a healty housekeeping.
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 need a call on this. I am missing how did you replace LongStrings functionality removed from BS.
No description provided.