builtins: implement to_char for timestamps and intervals #91382
Conversation
|
This change is |
otan
force-pushed
the
to_char_interval
branch
5 times, most recently
from
ceb2f8d to
2113b0a
Compare
There was a problem hiding this comment.
this is really great! my comments are nits mostly
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @otan)
pkg/sql/sem/builtins/builtins.go line 2428 at r1 (raw file):
}, Info: "Convert an timestamp to a string using the given format.", Volatility: volatility.Immutable,
in postgres this is stable. any reason we shouldn't match it?
rafiss=# select proname, prosrc, provolatile, proleakproof from pg_proc where proname like 'to_char%';
proname | prosrc | provolatile | proleakproof
---------+---------------------+-------------+--------------
to_char | timestamptz_to_char | s | f
to_char | numeric_to_char | s | f
to_char | int4_to_char | s | f
to_char | int8_to_char | s | f
to_char | float4_to_char | s | f
to_char | float8_to_char | s | f
to_char | interval_to_char | s | f
to_char | timestamp_to_char | s | f
pkg/util/duration/BUILD.bazel line 18 at r2 (raw file):
"//pkg/sql/pgwire/pgcode", "//pkg/sql/pgwire/pgerror", "//pkg/sql/types",
this will pull in the protobuf dependency transitively. is it worth avoiding that by not using the types package from within the duration package?
pkg/util/tochar/constants.go line 296 at r1 (raw file):
var dchIndex = []dchPos{ /* 0 1 2 3 4 5 6 7 8 9
what's with this counting? is the comment supposed to line up with the code? probably we'll want to do something else due to the autoformatter
pkg/util/tochar/tochar.go line 28 at r1 (raw file):
// TimeToChar converts a time and a `to_char` format string to a string. func TimeToChar(t time.Time, f string) (string, error) { // TODO(#sql-experience): consider caching parse formats.
if this is a goal we should have, then should we have some microbenchmarks in place?
pkg/util/tochar/tochar.go line 401 at r1 (raw file):
// parseFormat matches parse_format. We do not take in a flags as we only do // the DCH mode.
what is the DCH mode?
pkg/util/tochar/tochar.go line 63 at r3 (raw file):
} // durationWrapper wraps duration.Duration to implement timeInterface.
it looks like it's not wrapping it, but instead reimplementing it. could be worth explaining why in the comment
pkg/sql/logictest/testdata/logic_test/timestamp line 554 at r1 (raw file):
1 day # Rough translation of the to_char tests from PostgreSQL.
what about these tests and the ones below?
pkg/util/tochar/tochar_test.go line 27 at r1 (raw file):
// TestDataDriven tests the relevant to_char facilities. func TestDataDriven(t *testing.T) {
i love this!
Release note (sql change): The `to_char(timestamp, string)` function has now been implemented.
To allow parsing intervals in more places, move the duration parsing logic to the duration package. Release note: None
There was a problem hiding this comment.
Reviewable status:
Previously, rafiss (Rafi Shamim) wrote…
👏
Done.
pkg/sql/sem/builtins/builtins.go line 2428 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
in postgres this is stable. any reason we shouldn't match it?
rafiss=# select proname, prosrc, provolatile, proleakproof from pg_proc where proname like 'to_char%'; proname | prosrc | provolatile | proleakproof ---------+---------------------+-------------+-------------- to_char | timestamptz_to_char | s | f to_char | numeric_to_char | s | f to_char | int4_to_char | s | f to_char | int8_to_char | s | f to_char | float4_to_char | s | f to_char | float8_to_char | s | f to_char | interval_to_char | s | f to_char | timestamp_to_char | s | f
Done.
pkg/util/duration/BUILD.bazel line 18 at r2 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
this will pull in the
protobufdependency transitively. is it worth avoiding that by not using thetypespackage from within thedurationpackage?
eh, it's hard-ish to remove IntervalTypeMetadata so i'm inclined to go with no unless we have a good reason we shouldn't pull it in. I think we already do due to IntervalStyle pulling protobufs.
pkg/util/tochar/constants.go line 296 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
what's with this counting? is the comment supposed to line up with the code? probably we'll want to do something else due to the autoformatter
Ah, this was leftover from a PG comment. I can delete it.
pkg/util/tochar/tochar.go line 28 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
if this is a goal we should have, then should we have some microbenchmarks in place?
do microbenchmarks catch caching though? :P
(yes, but i'm not fussed enough to do it atm)
pkg/util/tochar/tochar.go line 401 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
what is the DCH mode?
Done. date-char i think, but i just changed the comment
pkg/util/tochar/tochar.go line 63 at r3 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
it looks like it's not wrapping it, but instead reimplementing it. could be worth explaining why in the comment
Done.
pkg/sql/logictest/testdata/logic_test/timestamp line 554 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
what about these tests and the ones below?
Done.
weird, we don't support naming a column "of" (syntax error), but other than that i copied it.
Release note (sql change): The `to_char(interval, string)` builtin has now been implemented.
For ~shits and giggles~ completeness! This commit implements the roman numeral commands for to_char. Release note: None
otan
force-pushed
the
to_char_interval
branch
from
2113b0a to
17a96ea
Compare
There was a problem hiding this comment.
lgtm!
Reviewable status:
|
bors r=rafiss |
|
Build succeeded: |
Resolves #91156
These commits implement to_char for timestamp and interval types. Most
of the code is copied from the PG implementation.