sql: internal error with BUCKET_COUNT = NULL

[vidit-bhat]

Previously, the EvalShardBucketCount function did not properly handle the case where BUCKET_COUNT was set to NULL, causing evaluation errors. This change adds explicit handling for NULL values, ensuring that BUCKET_COUNT cannot be set to NULL and returns an appropriate error message.

Epic: none
Fixes: #130353
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 

[spilchen]

Looks good. I just had a comment on the error message used.

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @vidit-bhat)

---

pkg/sql/catalog/tabledesc/table.go line 290 at r1 (raw file):

		// Check if shardBuckets is NULL
		if shardBuckets == tree.DNull {
			return 0, pgerror.Newf(pgcode.InvalidParameterValue, `"BUCKET_COUNT" cannot be NULL`)

Can we use invalidBucketCountMsg as the error message? I believe it still makes sense even when the count is NULL.

[vidit-bhat]

Looks good. I just had a comment on the error message used.

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @vidit-bhat)

pkg/sql/catalog/tabledesc/table.go line 290 at r1 (raw file):

		// Check if shardBuckets is NULL
		if shardBuckets == tree.DNull {
			return 0, pgerror.Newf(pgcode.InvalidParameterValue, `"BUCKET_COUNT" cannot be NULL`)

Can we use invalidBucketCountMsg as the error message? I believe it still makes sense even when the count is NULL.

Done

[spilchen]

Reviewed 2 of 2 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @vidit-bhat)

---

pkg/sql/catalog/tabledesc/table.go line 290 at r2 (raw file):

		// Check if shardBuckets is NULL
		if shardBuckets == tree.DNull {
			return 0, pgerror.Newf(pgcode.InvalidParameterValue, invalidBucketCountMsg, buckets)

Can we pass in "NULL" for the 3rd parm? I'm hoping the error message will say we passed in NULL if we do.

Suggestion:

return 0, pgerror.Newf(pgcode.InvalidParameterValue, invalidBucketCountMsg, "NULL")

[vidit-bhat]

Reviewed 2 of 2 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @vidit-bhat)

pkg/sql/catalog/tabledesc/table.go line 290 at r2 (raw file):

		// Check if shardBuckets is NULL
		if shardBuckets == tree.DNull {
			return 0, pgerror.Newf(pgcode.InvalidParameterValue, invalidBucketCountMsg, buckets)

Can we pass in "NULL" for the 3rd parm? I'm hoping the error message will say we passed in NULL if we do.

Suggestion:

return 0, pgerror.Newf(pgcode.InvalidParameterValue, invalidBucketCountMsg, "NULL")

Oops yes. Updated.

[spilchen]

Thanks for contributing.

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @vidit-bhat)

[vidit-bhat]

TFTR!
bors r=spilchen

[craig]

Build succeeded:

• lint
• check_generated_code
• macos_arm64_build
• linux_amd64_fips_build
• linux_arm64_build
• windows_build
• examples_orms
• local_roachtest_fips
• local_roachtest
• linux_amd64_build
• macos_amd64_build
• acceptance
• docker_image_amd64
• unit_tests

[rafiss]

we should backport this fix, since older branches are affected and the issue was causing a crash

blathers backport 23.1 23.2 24.1 24.2