-
Notifications
You must be signed in to change notification settings - Fork 594
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(expr): add support for make_date/time/timestamp #14827
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally LGTM.
@xiangjinwu please double check. 🥹
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rest LGTM!
let adjusted_year = if year < 0 && !for_timestamp { | ||
// For a year `-X`, The `Date` type is printed as `X+1 BC`, while `Timestamp` is printed as `-X`. | ||
year + 1 | ||
} else { | ||
year | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did not quite get this part. Date / timestamp / timestamptz shall all follow the same rule:
- In PG SQL, the year before 1 is -1. There is no year 0. It is printed as
1 BC
. - In ISO 8601 (chrono crate), the year before 1 is 0. It can be printed as
1 BC
(year_ce
instead ofyear
)
Practically it is okay to be incompatible with PG for BC years. But the logic here is both wrong and more complicated
In short, year + 1
as long as year < 0
when passing a PG SQL input to the chrono crate. This yields the correct value in memory during input. Whether showing X+1 BC
or -X
shall be adjusted on the output side. (same issue discovered multiple times recently #14832 (comment))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After discussion with xiangjin, we conduct +1
for both Date
and Timestamp
/Timestamptz
. However, as our ToText
for Date
and Timestamp
/Timestamptz
are different, the output of make_date
and make_timestamp/timestamptz
are different.
SELECT make_timestamp(-5, 02, 29, 08, 15, 55.33);
----
-0004-02-29 08:15:55.330
SELECT make_date(-5, 2, 29);
----
0005-02-29 BC
We'll fix it later in another PR by modify the impl of ToText
forTimestamp
/Timestamptz
.
921abf5
to
c8cdc3f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
||
query error Invalid parameter sec: invalid sec | ||
query error Invalid parameter year, month, day: invalid date: -3-2-29 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In postgres we have date field value out of range: -4-02-29
. Note the -4
here. Are we behaving correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
> SELECT make_date(-4, 02, 29);
ERROR: date field value out of range: -3-02-29
> SELECT make_timestamp(-4, 02, 29, 08, 15, 55.33);
ERROR: date field value out of range: -4-02-29
Pg behaves differently on make_date
and make_timestamp
. Seems to be a misaligned design. So I think it's OK if we don't follow it strictly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rest LGTM
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
As title
https://www.postgresql.org/docs/current/functions-datetime.html
Checklist
./risedev check
(or alias,./risedev c
)Documentation
Release note
If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.