stats: truncate large datums when sampling for histogram #39418
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @justinj, @RaduBerinde, and @rytaft)
pkg/sql/stats/row_sampling.go, line 153 at r1 (raw file):
// truncateDatum truncates large datums to avoid using excessive memory or disk // space.
[nit] Explain what "truncate" means.. Is it the closest datum of at most that size?
pkg/sql/stats/row_sampling.go, line 155 at r1 (raw file):
// space. func truncateDatum(evalCtx *tree.EvalContext, d tree.Datum, maxBytes int) tree.Datum { if d.Size() <= uintptr(maxBytes) {
if we move this chceck outside of the call, we can avoid calling Size() twice in the common case (where we don't truncate)
pkg/sql/stats/row_sampling.go, line 171 at r1 (raw file):
case *tree.DString: var r rune maxLen := uintptr(maxBytes) / unsafe.Sizeof(r)
rune is 32-bit, we are making it 4 times smaller than what we want in the common case of no unicode stuff
Also, we shouldn't need to copy since strings are immutable and can be sliced. I would do something like:
last := 0
// For strings, range skips from rune to rune and i is the byte index of the current rune.
for i := range s {
if i > maxBytes {
break
}
last = i
}
return tree.NewDString((*t)[last])
I think we can also slice and avoid the copy in the DBytes case
There was a problem hiding this comment.
TFTR!
Reviewable status:
pkg/sql/stats/row_sampling.go, line 153 at r1 (raw file):
Previously, RaduBerinde wrote…
[nit] Explain what "truncate" means.. Is it the closest datum of at most that size?
Done.
pkg/sql/stats/row_sampling.go, line 155 at r1 (raw file):
Previously, RaduBerinde wrote…
if we move this chceck outside of the call, we can avoid calling
Size()twice in the common case (where we don't truncate)
Good idea - done.
pkg/sql/stats/row_sampling.go, line 171 at r1 (raw file):
Previously, RaduBerinde wrote…
rune is 32-bit, we are making it 4 times smaller than what we want in the common case of no unicode stuff
Also, we shouldn't need to copy since strings are immutable and can be sliced. I would do something like:last := 0 // For strings, range skips from rune to rune and i is the byte index of the current rune. for i := range s { if i > maxBytes { break } last = i } return tree.NewDString((*t)[last])I think we can also slice and avoid the copy in the
DBytescase
Nice! I didn't know range worked that way with strings.
I think we need to do the copy because otherwise the backing array of the long string/ byte slice won't be garbage collected.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/stats/row_sampling.go, line 153 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
Done.
Sorry to be nitpicky but this doesn't sound right. What we return is not a valid representation of d. It's a different value and we need to describe something about that value (that makes sense in general, not just strings). Maybe say it returns a datum that is "close" (best-effort) to the original datum. Not sure how to define "close" but maybe it's ok to be vague there
pkg/sql/stats/row_sampling.go, line 171 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
Nice! I didn't know range worked that way with strings.
I think we need to do the copy because otherwise the backing array of the long string/ byte slice won't be garbage collected.
Good point, thanks for adding the comments.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/stats/row_sampling.go, line 153 at r1 (raw file):
Previously, RaduBerinde wrote…
Sorry to be nitpicky but this doesn't sound right. What we return is not a valid representation of d. It's a different value and we need to describe something about that value (that makes sense in general, not just strings). Maybe say it returns a datum that is "close" (best-effort) to the original datum. Not sure how to define "close" but maybe it's ok to be vague there
Does this sound better now?
There was a problem hiding this comment.
Reviewed 2 of 4 files at r1, 2 of 2 files at r2.
Reviewable status:
pkg/sql/stats/row_sampling.go, line 153 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
Does this sound better now?
Yeah, thanks!
This commit adds logic to truncate long bit arrays, byte strings, strings, and collated strings during sampling for histogram creation. We do this to avoid using excessive memory or disk space during sampling and storage of the final histogram. Release note: None
There was a problem hiding this comment.
Thanks!
bors r+
Reviewable status:
39418: stats: truncate large datums when sampling for histogram r=rytaft a=rytaft This commit adds logic to truncate long bit arrays, byte strings, strings, and collated strings during sampling for histogram creation. We do this to avoid using excessive memory or disk space during sampling and storage of the final histogram. Release note: None 39424: importccl: Direct-ingest uses two bulk adders instead of one. r=adityamaru27 a=adityamaru27 This is another change to stabilize direct ingest import before it is made the default. As a consequence of #39271, the number of files (L0 and total), along with the cumulative compaction size increased drastically. A consequence of no longer creating buckets of TableIDIndexID before flushing is that the single bulk adder would receive a mix of primary and secondary index entries. Since SSTs cannot span across the splits we inserted between index spans, it would create numerous, small secondary index SSTs along with the bigger primary index SSTs, and flush on reaching its limit (which would be often). By introducing two adders, one for ingesting primary index data, and the other for ingesting secondary index data we regain the ability to make fewer, bigger secondary index SSTs and flush less often. The peak mem is lower than what prebuffering used to hit, while the number of files (L0 and total), and the cumulative compaction size return to prebuffering levels. Some stats below for a tpcc 1k, on a 1 node cluster. With prebuffering: Total Files : 7670 L0 Files : 1848 Cumulative Compaction (GB): 24.54GiB Without prebuffering, one adder: Total Files : 22420 L0 Files : 16900 Cumulative Compaction (GB): 52.43 GiB Without prebuffering, two adders: Total Files : 6805 L0 Files : 1078 Cumulative Compaction (GB): 18.89GiB Release note: None Co-authored-by: Rebecca Taft <[email protected]> Co-authored-by: Aditya Maru <[email protected]>
Build succeeded |
This commit adds logic to truncate long bit arrays, byte strings,
strings, and collated strings during sampling for histogram creation.
We do this to avoid using excessive memory or disk space during
sampling and storage of the final histogram.
Release note: None