opt: normalize comparison with addition/subtraction of TS and INTERVALs#118307
Conversation
Release note: None
This commit lifts some of the limitations placed on comparison normalization rules in cockroachdb#90266. That PR limited algebraic transformations of comparison expressions when the types involved were anything other than integers, decimals, or floats. Other types, like `TIME`, can overflow without error during addition or subtraction, making the transformation incorrect. `TIMESTAMP` and `TIMESTAMPTZ` types will error on overflow when an `INTERVAL` is added to or subtracted from them. So this commit allows the rule to apply for expressions with these types. Specifically, the following transformations are now made: ts + const_interval [cmp] const_ts => ts [cmp] const_ts - const_interval ts - const_interval [cmp] const_ts => ts [cmp] const_ts + const_interval const_ts - ts [cmp] const_interval => const_ts - const_interval [cmp] ts This ultimately allows more rules to match, like GenerateConstrainedScans. Release note (performance improvement): The optimizer now generates more efficient query plans for queries with comparison of timestamp and interval columns, e.g., `timestamp_col - '1 day'::INTERVAL > now()`.
|
This change is |
DrewKimball
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @mgartner and @yuzefovich)
-- commits line 13 at r2:
I think it's only TIME and TIMETZ that have this problem, since their arithmetic is modular. Maybe we can omit those specifically and allow all other arithmetic types?
pkg/sql/opt/norm/testdata/rules/comp line 167 at r2 (raw file):
# The rule does not currently apply when it would subtract two TIMESTAMPs. # TODO(mgartner): Determine if this case is safe to allow.
FWIW, I think this is safe.
|
Previously, DrewKimball (Drew Kimball) wrote…
It looks like INTERVALs can wrap around: |
DrewKimball
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/norm/testdata/rules/comp line 167 at r2 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
It looks like INTERVALs can wrap around:
defaultdb> select '99999999999999999 years'::interval + '1000000000000000001 years'::INTERVAL; ?column? ------------------------------------- -437228672809129301 years -4 mons
Hm, well they're not supposed to...
postgres=# select '99999999999999999 years'::interval + '1000000000000000001 years'::INTERVAL;
ERROR: interval field value out of range: "99999999999999999 years"
LINE 1: select '99999999999999999 years'::interval + '10000000000000...
^
|
Previously, DrewKimball (Drew Kimball) wrote…
Ya our INTERVAL type is fairly broken. They don't match Postgres semantics closely: #84078 Well, I guess it wouldn't be possible to wrap around like this when subtracting two timestamps. |
yuzefovich
left a comment
There was a problem hiding this comment.
Thanks for the quick fix! I'll defer to Drew on this one.
Reviewed 1 of 1 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status:
DrewKimball
left a comment
There was a problem hiding this comment.
I think it's alright to keep the limitations until our interval handling is fixed.
Reviewable status:
|
I've created #118475 to track lifting the restrictions further. TFTRs! bors r+ |
|
Build succeeded: |
opt: add TIMESTAMP, TIMESTAMPTZ, and INTERVAL columns to comp norm tests
Release note: None
opt: normalize comparison with addition/subtraction of TS and INTERVALs
This commit lifts some of the limitations placed on comparison
normalization rules in #90266. That PR limited algebraic transformations
of comparison expressions when the types involved were anything other
than integers, decimals, or floats. Other types, like
TIME, canoverflow without error during addition or subtraction, making the
transformation incorrect.
TIMESTAMPandTIMESTAMPTZtypes will error on overflow when anINTERVALis added to or subtracted from them. So this commit allowsthe rule to apply for expressions with these types. Specifically, the
following transformations are now made:
This ultimately allows more rules to match, like
GenerateConstrainedScans.
Epic: None
Release note (performance improvement): The optimizer now generates more
efficient query plans for queries with comparison of timestamp and
interval columns, e.g.,
timestamp_col - '1 day'::INTERVAL > now().