sql/sem: add check for interval for asof.DatumToHLC()#93146
sql/sem: add check for interval for asof.DatumToHLC()#93146craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
ef11c6c to
cd07b13
Compare
rafiss
left a comment
There was a problem hiding this comment.
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
3232246 to
0f61337
Compare
ZhouXing19
left a comment
There was a problem hiding this comment.
Reviewable status:
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
<= 0and>= 0in theifs
Done.
0f61337 to
fb12bca
Compare
rafiss
left a comment
There was a problem hiding this comment.
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:
-- 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)
fixes cockroachdb#91021 Currently, if the given interval for `AS OF SYSTEM TIME interval` is a small postive duration, the query will 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 >= a microsecond. Similarly, when the logic is used under the SPLIT stmt, the interval's value has to be positive (for the experation duration), with an absolute value >= a microsecond. link epic CRDB-17785 Release note (bug fix): add restriction for duration value for AS OF SYSTEM TIME and SPLIT statement.
fb12bca to
14c89e1
Compare
ZhouXing19
left a comment
There was a problem hiding this comment.
Reviewable status:
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
syncheck 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.
|
TFTR! |
|
This PR was included in a batch that was canceled, it will be automatically retried |
|
Build failed (retrying...): |
|
Build succeeded: |
Currently, if the given interval for
AS OF SYSTEM TIME intervalis a small postive duration, the query can incorrectly pass. It's because when we callclock.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.