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
9651 scalar current date #10978
9651 scalar current date #10978
Conversation
Can one of the admins verify this patch? |
docs/general/builtins/scalar.rst
Outdated
``CURRENT_DATE`` | ||
---------------- | ||
|
||
The ``CURRENT_DATE`` expression returns the date in UTC timezone at the time the SQL statement was handled. Clock |
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.
Thanks for your contribution!
Would you mind wrapping this to 79 chars per line?
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.
Done
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.
Thank you for your contribution.
There are a couple of parts that are missing to add a new type, so I've left a couple of comments.
We'd probably also need to follow up and support the date time in some artihmetic functions. E.g. to support something like current_date + '1 day'::interval
. But I think that could be done in a follow up PR if you're interested.
|
||
public static final int ID = 23; | ||
public static final DateType INSTANCE = new DateType(); | ||
public static final int DATE_SIZE = (int) RamUsageEstimator.shallowSizeOfInstance(Long.class); // Value is written/read long number representing epoch milliseconds. |
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.
I think LocalDate
is bigger than just a Long
- this would have to be adjusted if we stick with LocalDate
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.
Done. I thought I'm estimating size of the data, written to StreamOutput, somehow missed a hint coming from RamUsageEstimator class name)
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.
Upd - As you suggested, I'm using now Long as internal date, reverted DATE_SIZE to Long size
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.
thanks for the PR!
I want to add a quick note to follow up on @mfussenegger's comment. last week, I added a TIP to the CURRENT_TIMESTAMP
subsection about CURRENT_TIMESTAMP - '1 day'::interval
(see de7cd71)
as the CURRENT_DATE
subsection is being added directly below this, it might be a good idea to add a short note saying either that CURRENT_DATE - '1 day'::interval
is supported or is not supported. might be overkill though?
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.
thanks for the PR!
I want to add a quick note to follow up on @mfussenegger's comment. last week, I added a TIP to the
CURRENT_TIMESTAMP
subsection aboutCURRENT_TIMESTAMP - '1 day'::interval
(see de7cd71)as the
CURRENT_DATE
subsection is being added directly below this, it might be a good idea to add a short note saying either thatCURRENT_DATE - '1 day'::interval
is supported or is not supported. might be overkill though?
Thanks for the comments! Regarding upper case - done, but I used actual name CURDATE - I added big comment below with all my concern and one of them is that CURRENT_DATE name was failing tests while CURDATE passed them.
TIPS - I think everything related to +/- intreval has to be done in a separate PR in order to keep the current small.
I would like to highlight potential issues with the PR. I think it's fine for now but worth mentioning anyway.
- Initially I used "current_date" name for the new scalar function. My test were failing and it took a while before I tried to use different name and same code worked with name "curdate".
As far as I understand, "current_time" is reserved word (saw it in SqlBaseParser) and probably having a new function with that name causes parsing failure.
Also, I saw builder.append("current_date"); in ExpressionFormatter, the only git commit for that line shows "add the presto sql parser".
While usage of curdate passes test, I found it confusing that current_date is a new function but I cannot use that name.
-
I haven't updated method visitCurrentTime in ExpressionAnalyzer class.It has switch (node.getType()) and that type enum already has DATE - looks like it was a placeholder for the future and I do have to add
case DATE:
funcName = CurrentTimeFunction.NAME;
break; but I decided to clarify that. -
DataTypes class has 2 maps:
TYPE_IDS_TO_MAPPINGS for mapping type to ES type and I mapped new type to "date" - same as existing types TIMESTAMPZ and TIMESTAMP.
However, there is also a reversed map MAPPING_NAMES_TO_TYPES - where ES type is mapped to Crate type.
We cannot have 2 "date" keys and that's why I didn't an entry here. I think mapping ES date to Crate TIMESTAMPZ makes sense as TIMESTAMPZ is the most specific among DATE, TIMESTAMP, TIMESTAMPZ types.
I believe it's fine but I have to explicitly highlight it since it looks like a place where new type might leave a trace but it was not done.
- New types' ID field value - I spent some time trying to figure out a pattern or registry of values as I had a feeling that value must be unique throughout all types but the only approach was looking over all type files and checking their ids and keeping some unused number. There are some maps with IDS but one cannot use them as a quick ID-dictionary since they are given not as numbers but in Type.ID form. I hope my approach is fine and I haven't missed some file or even script for assigning those ids.
|
||
public static final int ID = 23; | ||
public static final DateType INSTANCE = new DateType(); | ||
public static final int DATE_SIZE = (int) RamUsageEstimator.shallowSizeOfInstance(Long.class); // Value is written/read long number representing epoch milliseconds. |
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.
Done. I thought I'm estimating size of the data, written to StreamOutput, somehow missed a hint coming from RamUsageEstimator class name)
docs/general/builtins/scalar.rst
Outdated
``CURRENT_DATE`` | ||
---------------- | ||
|
||
The ``CURRENT_DATE`` expression returns the date in UTC timezone at the time the SQL statement was handled. Clock |
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.
Done
1546ef3
to
711c0ba
Compare
Yes we'd have to add here a new case for
Yes good spot. As the underlaying field mappers (of ES) do not make any difference between
Yes there is no real patter, it should just not be used already. So your chosen ID looks fine. |
711c0ba
to
3f7bda7
Compare
Thanks for the comment, added case to switch and removed map entry. |
3f7bda7
to
f363c3d
Compare
server/src/main/java/io/crate/protocols/postgres/types/PGTypes.java
Outdated
Show resolved
Hide resolved
server/src/main/java/io/crate/protocols/postgres/types/PGTypes.java
Outdated
Show resolved
Hide resolved
056fe96
to
89572b8
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.
Thanks for your efforts. I've added another round of suggestions/comments.
server/src/main/java/io/crate/protocols/postgres/types/DateType.java
Outdated
Show resolved
Hide resolved
server/src/test/java/io/crate/expression/scalar/CurrentDateFunctionTest.java
Show resolved
Hide resolved
server/src/test/java/io/crate/protocols/postgres/types/DateTypeTest.java
Show resolved
Hide resolved
87a1690
to
2426dc5
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.
Looks really good already, I've added final documentation suggestions.
9c96c30
to
f90d8ff
Compare
retest this please |
@BaurzhanSakhariev Thanks a lot for addressing all comments. btw. some |
I'm checking locally, some tests are failing with timeout (looks like I need to configure environment) for example Thanks for pointing out the specific file (I couldn't see it inGitHub report as I need to log in to Jenkins) I will take a look for that specific case. |
ebde802
to
7c417bf
Compare
I fixed However, neither itest nor app:run are not working on my machine. I updated My OS is 64-bit Windows 10 and problem with I tried that and also changed Regarding it fails with
I've been investigating and trying to fix both commands failure - if I manage to make itest related scripts cross-platform I will create another PR for that. It would be nice if we can trigger remaining |
thanks for your efforts. Regarding Windows runtime support, CrateDB should work - you might have observed that the main test suite is also being invoked on GitHub Actions. While we occasionally have to adjust things in this space regarding flakyness (e.g. #11045), it works in general. Regarding blackbox testing, I believe it is using This is just to give you a heads-up which kinds of patches would have to be applied as a minimum to make it work on Windows. The state of the onion is that, by using those patches, it should already work reasonably but currently fails on test layer teardown on GHA/CI due to some nasty deadlocking issue where I haven't been able to investigate further. So, while it will probably be a good idea to continue day-to-day development of CrateDB on Linux or macOS, I am happy to hear you also know your way around Windows and hope that you will be available to improve Windows support for all the test harnesses we are running as well as Windows support in general. See, for example, #10516 and #6349. Additionally, you might want to inspect the GHA recipe to learn about respective arguments to Gradle: crate/.github/workflows/main.yml Line 28 in 9389bb2
After rebasing your branch on top of current master, you might be able to trigger that on your own by adding a comment saying With kind regards, |
retest this please |
04e27ef
to
9532635
Compare
retest this please |
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.
Thanks for your changes. I've added some comments mostly related to documentation.
docs/general/ddl/data-types.rst
Outdated
Date also accepts a ``bigint`` representing UTC milliseconds since | ||
the epoch. | ||
|
||
:: | ||
|
||
cr> select 1615248000000::date as cd; | ||
+---------------+ | ||
| cd | | ||
+---------------+ | ||
| 1615248000000 | | ||
+---------------+ | ||
SELECT 1 row in set (... sec) |
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.
I'd remove this example if you take my suggestion above which already mention the support of numeric->date casts.
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.
Done. I removed string casting example select '2021-03-09'::date
as well as it's also mentioned in suggestion.
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.
I think keeping the string cast example would still be valuable (also this may be the common use case).
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.
done
server/src/main/java/io/crate/expression/scalar/CurrentDateFunction.java
Outdated
Show resolved
Hide resolved
d1214aa
to
936bb83
Compare
retest this please |
936bb83
to
a36ae04
Compare
retest this please |
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.
Left some more coments.
Overall seems like it's almost ready 👍
server/src/main/java/io/crate/expression/scalar/CurrentDateFunction.java
Outdated
Show resolved
Hide resolved
LocalDate dt = LocalDate.parse(s, ISO_FORMATTER); | ||
return dt.atStartOfDay(ZoneOffset.UTC).toInstant().toEpochMilli(); |
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.
PostgreSQL supports more text formats as input:
https://www.postgresql.org/docs/current/datatype-datetime.html
I think we either have to extend this, or make it explicit in the documentation that the text format support is not compatible with all PostgreSQL clients.
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.
I updated data-types.rst
and added TODO to DateType
4046b55
to
c6788c8
Compare
retest this please |
ok to test |
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.
Nice work 👍
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.
👍 Great!
396e910
to
1c70e58
Compare
Summary of the changes / Why this improves CrateDB
Checklist
CHANGES.txt
for user facing changessql_features
table for user facing changesCHANGES.txt
(E.g. AdminUI)