sql/stats: bring back guard against non-zero NumRange in forecasts

[michae2]

It occurred to me tonight that the code we removed in #143955 not only generated a sentry report, but also returned an error instead of producing a faulty histogram. I too hope that #93892 is now fixed, but it seems wise to at least keep some code that guards against faulty histograms, even if we don't think the sentry report is necessary any more.

Informs: #93892

Epic: None

Release note: None

[blathers-crl]

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[cockroach-teamcity]

This change is 

[yuzefovich]

Thanks for catching this! I agree that more guardrails never hurt - I didn't realize that the check was useful as a guardrail.

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

---

pkg/sql/stats/forecast.go line 335 at r1 (raw file):

		forecast.setHistogramBuckets(hist)

		// Verify that the first two buckets (the initial NULL bucket and the first

nit: this verification is stricter than the one we do in props/histogram.go - there we return "the first bucket should have NumRange=0" assertion error in two spots, but in both we're only looking at 0th bucket and only NumRange value (multiplied by selectivity). Why do we deviate here and verify more?

[michae2]

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

---

pkg/sql/stats/forecast.go line 335 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: this verification is stricter than the one we do in props/histogram.go - there we return "the first bucket should have NumRange=0" assertion error in two spots, but in both we're only looking at 0th bucket and only NumRange value (multiplied by selectivity). Why do we deviate here and verify more?

In props/histogram.go in both filter and maxDistinctValuesCount we don't know whether we're working with a portion of a histogram (one that has already been filtered) or an entire histogram. So we only check the first bucket.

Here in forecasting we know we're working with the entire histogram, including the synthesized NULL bucket if it exists, so we might as well check that there's nothing between the synthesized NULL bucket and the first non-NULL value.

[michae2]

TFTR!

bors r=yuzefovich

[craig]

Build failed (retrying...):

• examples_orms

[craig]

This PR was included in a batch that successfully built, but then failed to merge into master (it was a non-fast-forward update). It will be automatically retried.

[yuzefovich]

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

---

pkg/sql/stats/forecast.go line 335 at r1 (raw file):

Previously, michae2 (Michael Erickson) wrote…

In props/histogram.go in both filter and maxDistinctValuesCount we don't know whether we're working with a portion of a histogram (one that has already been filtered) or an entire histogram. So we only check the first bucket.

Here in forecasting we know we're working with the entire histogram, including the synthesized NULL bucket if it exists, so we might as well check that there's nothing between the synthesized NULL bucket and the first non-NULL value.

I see, makes sense about the bucket, thanks.

Other part of my comment was about also verifying DistinctRange - it doesn't look like we assert anything about that in props/histogram. Do we add that check here since NumRange = 0 and DistinctRange > 0 doesn't make sense, in general, even if we don't assert that later?

[craig]

Build succeeded:

• examples_orms
• linux_amd64_fips_build
• local_roachtest
• lint
• local_roachtest_fips
• check_generated_code
• acceptance
• unit_tests
• linux_amd64_build
• docker_image_amd64

[michae2]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/sql/stats/forecast.go line 335 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

I see, makes sense about the bucket, thanks.

Other part of my comment was about also verifying DistinctRange - it doesn't look like we assert anything about that in props/histogram. Do we add that check here since NumRange = 0 and DistinctRange > 0 doesn't make sense, in general, even if we don't assert that later?

yes, that's right