sql/sem: add check for interval for asof.DatumToHLC()

[ZhouXing19]

Currently, if the given interval for AS OF SYSTEM TIME interval is a small postive duration, the query can incorrectly pass. It's because when we call clock.Now(), it has passed the threashold ('statement timestamp + duration').

Now we add a check for the duration value. It has to be negative, with the absolute value greater than a nanosecond.

fixes #91021
link epic CRDB-17785

Release note (bug fix): add restriction for duration value for AS OF SYSTEM TIME statement.

[cockroach-teamcity]

This change is 

[rafiss]

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

---

pkg/sql/sem/asof/as_of.go line 253 at r1 (raw file):

		if iv, err := tree.ParseDInterval(evalCtx.GetIntervalStyle(), s); err == nil {
			if (iv.Duration == duration.Duration{}) {
				convErr = errors.Errorf("interval value %v too small, absolute value must be >= %v", d, time.Microsecond)

is time.Microsecond correct in this earlier message?

actually, we can just remove this if case if we change the below ones to iv.Duration.Compare(duration.Duration{}) >= 0 and iv.Duration.Compare(duration.Duration{}) <= 0

---

pkg/sql/sem/asof/as_of.go line 255 at r1 (raw file):

				convErr = errors.Errorf("interval value %v too small, absolute value must be >= %v", d, time.Microsecond)
			} else if (usage == AsOf && iv.Duration.Compare(duration.Duration{}) > 0 && !syn) {
				convErr = errors.Wrapf(

i think this should just be:

convErr = errors.Newf("interval value %v too large, AS OF interval must be <= %v", d, -time.Nanosecond)

---

pkg/sql/sem/asof/as_of.go line 261 at r1 (raw file):

				)
			} else if (usage == Split && iv.Duration.Compare(duration.Duration{}) < 0 && !syn) {
				convErr = errors.Wrapf(

i think this should just be:

convErr = errors.Newf("interval value %v too small, SPLIT AT interval must be >= %v", d, time.Nanosecond)

---

pkg/sql/sem/asof/as_of.go line 290 at r1 (raw file):

				errors.Newf("unsupported interval duration: %v", d),
				"expiration interval must be non-negative",
			)

same comment on error messages (and using <= 0 and >= 0 in the ifs

[ZhouXing19]

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

---

pkg/sql/sem/asof/as_of.go line 253 at r1 (raw file):

is time.Microsecond correct in this earlier message?

Ah yes, it was correct, I made a typo. Changed them back.

change the below ones to iv.Duration.Compare(duration.Duration{}) >= 0 and iv.Duration.Compare(duration.Duration{}) <= 0

I separated the == case because we always disallow it, but we actually allow > for AS OF and < for SPLIT if the interval is synthetic ,see here.

---

pkg/sql/sem/asof/as_of.go line 255 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i think this should just be:

convErr = errors.Newf("interval value %v too large, AS OF interval must be <= %v", d, -time.Nanosecond)

Done.

---

pkg/sql/sem/asof/as_of.go line 261 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i think this should just be:

convErr = errors.Newf("interval value %v too small, SPLIT AT interval must be >= %v", d, time.Nanosecond)

Done.

---

pkg/sql/sem/asof/as_of.go line 290 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

same comment on error messages (and using <= 0 and >= 0 in the ifs

Done.

[rafiss]

thanks for the fix!

Reviewed 1 of 4 files at r1, 2 of 3 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ZhouXing19)

---

-- commits line 11 at r4:
nit: commit message still uses nanosecond instead of microsecond

---

pkg/sql/sem/asof/as_of.go line 256 at r4 (raw file):

			} else if (usage == AsOf && iv.Duration.Compare(duration.Duration{}) > 0 && !syn) {
				convErr = errors.Errorf("interval value %v too large, AS OF interval must be <= -%v", d, time.Microsecond)
			} else if (usage == Split && iv.Duration.Compare(duration.Duration{}) < 0 && !syn) {

are you sure the syn check is needed on this one? (i'm not too familiar with it)

[ZhouXing19]

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

---

-- commits line 11 at r4:

Previously, rafiss (Rafi Shamim) wrote…

nit: commit message still uses nanosecond instead of microsecond

Done.

---

pkg/sql/sem/asof/as_of.go line 256 at r4 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

are you sure the syn check is needed on this one? (i'm not too familiar with it)

Good point, I'm not familiar either, and there wasn't check / test for synthetic timestamp previously for SPLIT AT. I removed it for now and added a comment.

[ZhouXing19]

TFTR!
bors r=rafiss

[craig]

This PR was included in a batch that was canceled, it will be automatically retried

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)