opt: support BYTES for histogram range calculations by mgartner · Pull Request #68740 · cockroachdb/cockroach

mgartner · 2021-08-11T23:12:55Z

Release note (performance improvement): The accuracy of histogram
calculations for BYTES types has been improved. As a result, the
optimizer should generate more efficient query plans in some cases.

cockroach-teamcity · 2021-08-11T23:13:01Z

This change is

michae2

Thank you for looking at this!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner and @rytaft)

pkg/sql/opt/props/histogram.go, line 864 at r1 (raw file):

Quoted 5 lines of code…

	switch typ.Family() {
	case types.StringFamily, types.BytesFamily, types.UuidFamily, types.INetFamily:
		return true
	}
	return false

To my untrained eye it looks like this list is missing a few more non-numeric type families, such as CollatedStringFamily and JsonFamily. Plus, I'm wondering if ArrayFamily and TupleFamily belong here as well? Hmm. I am new to this code, but it almost seems like getRangeNonNumeric should be the default range-calculation function, and getRange should only be called for special cases, since presumably every type's key encoding is bytewise ordered / comparable and at least somewhat bytewise "dense".

rytaft

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner and @michae2)

pkg/sql/opt/props/histogram.go, line 864 at r1 (raw file):

Previously, michae2 (Michael Erickson) wrote…

	switch typ.Family() {
	case types.StringFamily, types.BytesFamily, types.UuidFamily, types.INetFamily:
		return true
	}
	return false
To my untrained eye it looks like this list is missing a few more non-numeric type families, such as CollatedStringFamily and JsonFamily. Plus, I'm wondering if ArrayFamily and TupleFamily belong here as well? Hmm. I am new to this code, but it almost seems like getRangeNonNumeric should be the default range-calculation function, and getRange should only be called for special cases, since presumably every type's key encoding is bytewise ordered / comparable and at least somewhat bytewise "dense".

I like the idea of swapping the default, and checking instead for the types that we explicitly support in getRange. Can these other non-numeric types be easily added in this PR? Otherwise, maybe open an issue for the other types that we're missing?

On a somewhat related note, I think we can also add Interval, Enum, and Oid as numeric types (in a separate PR...).

pkg/sql/opt/props/histogram_test.go, line 710 at r1 (raw file):

	})

	t.Run("bytes", func(t *testing.T) {

is all of this basically directly copied from above? if so, can you just have one copy that's used for both string and bytes?

mgartner

Sorry for the delay in getting back to these comments. PTAL!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @michae2 and @rytaft)

pkg/sql/opt/props/histogram.go, line 864 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

I like the idea of swapping the default, and checking instead for the types that we explicitly support in getRange. Can these other non-numeric types be easily added in this PR? Otherwise, maybe open an issue for the other types that we're missing?

On a somewhat related note, I think we can also add Interval, Enum, and Oid as numeric types (in a separate PR...).

I've combined the three functions getRange, getRangeNonNumeric and isNonNumeric range into a single switch statement. I think this is more clear and avoids having to keep three functions in-sync.

pkg/sql/opt/props/histogram_test.go, line 710 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

is all of this basically directly copied from above? if so, can you just have one copy that's used for both string and bytes?

Done.

mgartner

I created #70320 to track calculating ranges for additional types.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @michae2 and @rytaft)

michae2

Reviewed 1 of 3 files at r1, 1 of 1 files at r2, 4 of 4 files at r4.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @mgartner, @michae2, and @rytaft)

pkg/sql/opt/props/histogram.go, line 864 at r1 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

I've combined the three functions getRange, getRangeNonNumeric and isNonNumeric range into a single switch statement. I think this is more clear and avoids having to keep three functions in-sync.

OK, thanks for the refactor and the filed issue.

pkg/sql/opt/props/histogram_test.go, line 647 at r4 (raw file):

	})

	t.Run("string/bytes", func(t *testing.T) {

nit: "/" is the subtest separator for go test, so this name creates a TestFilterBucket/string/bytes sub-sub-test instead of a TestFilterBucket/<x> sub-test. Maybe use "string,bytes" or some other symbol instead?

rytaft

Reviewed 1 of 1 files at r2, 4 of 4 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @mgartner)

pkg/sql/opt/props/histogram.go, line 716 at r5 (raw file):

//
func getRangesBeforeAndAfter(
	bucketLowerBound, bucketUpperBound, spanLowerBound, spanUpperBound tree.Datum, swap bool,

nit: I think we can get rid of the bucket v. span distinction to further clarify things. i.e. bucket -> before, span -> after

pkg/sql/opt/props/histogram.go, line 820 at r5 (raw file):

		// For non-numeric types, convert the datums to encoded keys to
		// approximate the range. We utilize an array to reduce repetitive code
		// number of repetitive calls.

nit: "...to reduce repetitive code number of repetitive calls." is repetitive! 😉

mgartner

Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @rytaft)

pkg/sql/opt/props/histogram.go, line 716 at r5 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

nit: I think we can get rid of the bucket v. span distinction to further clarify things. i.e. bucket -> before, span -> after

Done.

pkg/sql/opt/props/histogram.go, line 820 at r5 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

nit: "...to reduce repetitive code number of repetitive calls." is repetitive! 😉

🙃

pkg/sql/opt/props/histogram_test.go, line 647 at r4 (raw file):

Previously, michae2 (Michael Erickson) wrote…

nit: "/" is the subtest separator for go test, so this name creates a TestFilterBucket/string/bytes sub-sub-test instead of a TestFilterBucket/<x> sub-test. Maybe use "string,bytes" or some other symbol instead?

Good catch. Fixed.

rytaft

Reviewed 2 of 2 files at r6, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @mgartner)

pkg/sql/opt/props/histogram.go, line 666 at r6 (raw file):

}

// getRangesBeforeAndAfter returns the size of the ranges before and after the

nit: update comment to remove references to bucket/span

pkg/sql/opt/props/histogram.go, line 736 at r6 (raw file):

	// TODO(rytaft): handle more types here.
	// Note: the calculations below assume that bucketLowerBound is inclusive and
	// Span.PreferInclusive() has been called on the span.

nit: update comment (just say all bounds are inclusive)

Fixes cockroachdb#68346 Release note (performance improvement): The accuracy of histogram calculations for BYTES types has been improved. As a result, the optimizer should generate more efficient query plans in some cases.

Histogram range calculation has been refactored into a single switch statement. This prohibits inconsistencies between the three previous functions `getRange`, `getRangeNonNumeric` and `isNonNumeric`. It also highlights the preference for calculating numeric ranges when possible. Release note: None

mgartner

Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @rytaft)

pkg/sql/opt/props/histogram.go, line 666 at r6 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

nit: update comment to remove references to bucket/span

Done.

pkg/sql/opt/props/histogram.go, line 736 at r6 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

nit: update comment (just say all bounds are inclusive)

Done.

rytaft

Reviewed 2 of 2 files at r7, 2 of 2 files at r8, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @mgartner)

mgartner · 2021-09-24T18:51:29Z

TFTRs!

bors r+

craig · 2021-09-24T20:08:47Z

Build succeeded:

GitHub CI (Cockroach)

mgartner requested review from a team, michae2 and rytaft August 11, 2021 23:12

michae2 requested changes Aug 11, 2021

View reviewed changes

rytaft reviewed Aug 17, 2021

View reviewed changes

mgartner mentioned this pull request Sep 16, 2021

opt: calculate histogram ranges for more types #70320

Open

mgartner force-pushed the fix-bytes-histogram-filtering branch from e2ae5b2 to 28adb74 Compare September 16, 2021 16:31

mgartner requested a review from a team as a code owner September 16, 2021 16:31

mgartner commented Sep 16, 2021

View reviewed changes

mgartner force-pushed the fix-bytes-histogram-filtering branch from 28adb74 to 73f3a67 Compare September 16, 2021 20:16

michae2 approved these changes Sep 20, 2021

View reviewed changes

rytaft approved these changes Sep 20, 2021

View reviewed changes

mgartner force-pushed the fix-bytes-histogram-filtering branch from 73f3a67 to a041913 Compare September 23, 2021 16:34

mgartner commented Sep 23, 2021

View reviewed changes

rytaft approved these changes Sep 23, 2021

View reviewed changes

mgartner added 2 commits September 23, 2021 16:05

opt: support BYTES for histogram range calculations

2d6994e

Fixes cockroachdb#68346 Release note (performance improvement): The accuracy of histogram calculations for BYTES types has been improved. As a result, the optimizer should generate more efficient query plans in some cases.

mgartner force-pushed the fix-bytes-histogram-filtering branch from a041913 to 03c45a0 Compare September 23, 2021 20:16

mgartner commented Sep 24, 2021

View reviewed changes

rytaft approved these changes Sep 24, 2021

View reviewed changes

craig bot merged commit e4e9a78 into cockroachdb:master Sep 24, 2021

cockroach-teamcity mentioned this pull request Sep 24, 2021

opt: support BYTES for histogram range calculations cockroachdb/docs#11782

Closed

mgartner deleted the fix-bytes-histogram-filtering branch September 24, 2021 20:42

This was referenced Feb 18, 2022

release-21.2: opt: support BYTES for histogram range calculations #76796

Merged

release-21.1: opt: support BYTES for histogram range calculations #76797

Merged

mgartner mentioned this pull request Mar 8, 2022

sql: introduce special type for inverted index keys #77501

Merged

Conversation

mgartner commented Aug 11, 2021

Uh oh!

cockroach-teamcity commented Aug 11, 2021

Uh oh!

michae2 left a comment

Choose a reason for hiding this comment

Uh oh!

rytaft left a comment

Choose a reason for hiding this comment

Uh oh!

mgartner left a comment

Choose a reason for hiding this comment

Uh oh!

mgartner left a comment

Choose a reason for hiding this comment

Uh oh!

michae2 left a comment

Choose a reason for hiding this comment

Uh oh!

rytaft left a comment

Choose a reason for hiding this comment

Uh oh!

mgartner left a comment

Choose a reason for hiding this comment

Uh oh!

rytaft left a comment

Choose a reason for hiding this comment

Uh oh!

mgartner left a comment

Choose a reason for hiding this comment

Uh oh!

rytaft left a comment

Choose a reason for hiding this comment

Uh oh!

mgartner commented Sep 24, 2021

Uh oh!

craig bot commented Sep 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants