sql: implement IntervalStyle #67000
sql: implement IntervalStyle #67000
Conversation
|
This change is |
otan
force-pushed
the
intervalstyle
branch
4 times, most recently
from
51425bf to
2591fe7
Compare
There was a problem hiding this comment.
my comments are all pretty minor. this is great work navigating the maze!
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @otan)
pkg/sql/vars.go, line 802 at r2 (raw file):
// See https://www.postgresql.org/docs/10/static/runtime-config-client.html#GUC-INTERVALSTYLE `intervalstyle`: {
are the session var names case insensitive?
pkg/sql/sem/tree/casts.go, line 903 at r5 (raw file):
FmtDataConversionConfig(ctx.SessionData.DataConversionConfig), ) case *DInterval:
i didn't even realize that casts were affected. nice catch
pkg/sql/sessiondatapb/session_data.proto, line 66 at r2 (raw file):
// DataConversionConfig contains the parameters that influence the conversion // between SQL data types and strings/byte arrays.
maybe update this comment?
pkg/sql/sessiondatapb/session_data.proto, line 76 at r2 (raw file):
int32 extra_float_digits = 2; // IntervalStyle indicates the style to display intervals as. util.duration.IntervalStyle interval_style = 13;
why is it = 13?
pkg/util/duration/duration.go, line 596 at r1 (raw file):
} // extractAbsTime returns signed amount of hours, minutes, seconds,
nit: comment is for extractTime
pkg/util/duration/duration.proto, line 16 at r1 (raw file):
// IntervalStyle matches the PostgreSQL IntervalStyle session parameter. enum IntervalStyle {
dumb Q: why did we want this in a proto?
pkg/util/duration/duration_test.go, line 605 at r1 (raw file):
} func TestFormat(t *testing.T) {
great test -- is there a way we can confirm that this matches the postgres output?
There was a problem hiding this comment.
Reviewable status:
pkg/sql/vars.go, line 802 at r2 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
are the session var names case insensitive?
yep!
pkg/sql/sem/tree/casts.go, line 903 at r5 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
i didn't even realize that casts were affected. nice catch
Done.
pkg/sql/sessiondatapb/session_data.proto, line 66 at r2 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
maybe update this comment?
Done.
pkg/sql/sessiondatapb/session_data.proto, line 76 at r2 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
why is it
= 13?
oops.
pkg/util/duration/duration.proto, line 16 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
dumb Q: why did we want this in a proto?
for storing this into the SessionData proto, which can be serialized.
pkg/util/duration/duration_test.go, line 605 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
great test -- is there a way we can confirm that this matches the postgres output?
credits go to @mneverov for this - i only improved the readability.
i've verified these cases to match, but outside of randomised testing it's hard
Release note: None
Release note (sql change): Support iso_8601 and sql_standard as usable session variables in IntervalStyle. Also add a `sql.defaults.intervalstyle` cluster setting to be used as the default interval style.
First, we place some session data into FmtCtx which are relevant for printing. Plumb the sessiondata into FmtCtx in many places by making evalCtx have a FmtCtx option, which creates a FmtCtx with the session data from the evalCtx. Unfortunately, this turned out to not heavily help, as casts, JSON and pgwire do not go through this codepath... Release note: None
This commit outputs intervals with the the IntervalStyle session variable over pgwire. Release note: None
Apply IntervalStyle to casts from intervals (arrays and tuples included) to string. Release note: None
This enable JSON based operations to correctly apply the IntervalStyle session setting. Release note: None
There was a problem hiding this comment.
lgtm!
what do you think about writing a CompareTest that is just focused on comparing IntervalStyle? it would mean adding a new test to the configs in compare_test.go, then a sqlsmith.SmitherOption that just generates SELECT interval and SET intervalstyle queries. Can be a later PR or a backlog issue if you think it's a reasonable idea!
bigger priority than that would be to implement DateStyle next: #41773
Reviewed 4 of 5 files at r1, 31 of 31 files at r7.
Reviewable status:
that one is going to be an even bigger PITA ...
i think it's a fine idea. will write an issue. |
|
bors r=rafiss |
|
Build succeeded: |
See individual commits for details.
This is supremely messy, and I hope one day we clean this up. We do conversions differently for casts, pgwire and JSON.
ctx.Locationandsessiondatapb.DataConversionConfigshould probably be unified, and be universally applies from the FmtCtx instead of ad-hocly applied throughout the stack (extra_float_digitsandbytes_encode_formatsuffers the same problem and is probably incorrect in a few places). I'll file an issue for this after it merges.Supercedes #62313
Resolves #65724
Resolves #61167