Re: [Tarantool-patches] [PATCH v6 3/5] box, datetime: datetime comparison for indices

[Safin Timur via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

On 19.08.2021 14:18, unera@tarantool.org wrote:
> Hi!
> I looked trhough the patchset.
> I also compiled the bracnch and tested by hand.
> Example:
> tarantool> datetime = require('datetime')
> ---
> ...
> tarantool> datetime.new('2020-01-02 02:00')
> ---
> - 1970-01-01T00:00Z
> ...
> tarantool> datetime.new('2020-01-02 02:00+0300')
> ---
> - 1970-01-01T00:00Z
> ...
> tarantool> datetime.new('2020-01-02 02:00:11+0300')
> ---
> - 1970-01-01T00:00Z
> ...
> tarantool> datetime.new('2020-01-02T02:00:11+0300')
> ---
> - 1970-01-01T00:00Z
> ...
> The code looks like as broken.

Not exactly :)

.new{} is constructor which is not expecting string at the moment. 
.parse() expect strings, or defaul __call handler for the module, which 
is dispatching between parsing, if passed string, and, direct calling 
constructor, if there are no arguments passed or they are object.

```
tarantool> date('2020-01-02 02:00')
---
- 2020-01-02T02:00Z
...

tarantool> date('2020-01-02 02:00')
---
- 2020-01-02T02:00Z
...

tarantool> date('2020-01-02 02:00:11+0300')
---
- 2020-01-02T02:00:11+03:00
...

tarantool> date('2020-01-02T02:00:11+0300')
---
- 2020-01-02T02:00:11+03:00
...
```

> Also my notes:
> 
>  1. tostring has to return datetime string with seconds and timezone

Will add required seconds - they are reduced now for more compact form 
like here below.
------------------------------------------------
	if (second || nanosec) {
		SNPRINT(sz, snprintf, buf, len, ":%02d", second);
		if (nanosec) {
------------------------------------------------

here is incrementa change which enforce required seconds:
------------------------------------------------
diff --git a/src/lib/core/datetime.c b/src/lib/core/datetime.c
index ebc05e29c..6e2a76da5 100644
--- a/src/lib/core/datetime.c
+++ b/src/lib/core/datetime.c
@@ -93,20 +93,17 @@ datetime_to_string(const struct datetime *date, char 
*buf, int len)
  	nanosec = date->nsec;

  	int sz = 0;
-	SNPRINT(sz, snprintf, buf, len, "%04d-%02d-%02dT%02d:%02d",
-		year, month, day, hour, minute);
-	if (second || nanosec) {
-		SNPRINT(sz, snprintf, buf, len, ":%02d", second);
-		if (nanosec) {
-			if ((nanosec % 1000000) == 0)
-				SNPRINT(sz, snprintf, buf, len, ".%03d",
-					nanosec / 1000000);
-			else if ((nanosec % 1000) == 0)
-				SNPRINT(sz, snprintf, buf, len, ".%06d",
-					nanosec / 1000);
-			else
-				SNPRINT(sz, snprintf, buf, len, ".%09d", nanosec);
-		}
+	SNPRINT(sz, snprintf, buf, len, "%04d-%02d-%02dT%02d:%02d:%02d",
+		year, month, day, hour, minute, second);
+	if (nanosec != 0) {
+		if ((nanosec % 1000000) == 0)
+			SNPRINT(sz, snprintf, buf, len, ".%03d",
+				nanosec / 1000000);
+		else if ((nanosec % 1000) == 0)
+			SNPRINT(sz, snprintf, buf, len, ".%06d",
+				nanosec / 1000);
+		else
+			SNPRINT(sz, snprintf, buf, len, ".%09d", nanosec);
  	}
  	if (offset == 0) {
  		SNPRINT(sz, snprintf, buf, len, "Z");
------------------------------------------------
>  2. also the other databases try to avoid to use «Z» symbol in
>     datetime-format, so I thing we should use «+00:00» instead «Z»

I don't care which way to output UTC, but here I need more examples 
(which vendor, how and when outputs timezone)...

Like - here is DATE WITH TIMEZONE column, and here is DATE WITHOUT 
TIMEZONE. And here TIMESTAMP WITHOUT TIMEZONE, and with UTC, and we see...

> 
> Yesterday we approved the following API:
> T:add{year=XXX, month=YYY, ...}
> T:sub{year=XXX, month=YYY, ...}
> T:set{year=XXX, …, tz=’+03:00’}
> T:strftime(‘%F %D %z’)
> I want to see a test set for the API.

FWIW, :add{}/:sub{}/:strftime{} are there, but not :set() as 
unimplemented yet.

> 
> Also we approved the following methods:
> T:sec() — 35
> T:min() — 12
> T:day() — 19
> T:wday() — 5
> T:yday() — 231
> T:year() — 2021
> T:month() — 8
> T:hour() — 23
> T:tz() — ‘+03:00’

Those have not yet been exported, yes.

And BTW, tz() assumed to return numeric information (difference in 
minutes to UTC), not string representation?

What about :totable() - I've got an impression that we also approved it. 
Yes?

> It would be nice to see test set for each of the functions.
> Also it would be nice to rename ambigouos items «secs» to «epoch» or 
> «timestamp»

Here I am confused - which "items" do you mean here? [Internal strcuture 
fields, or some API elements?]

> Datetime object must have methods to return integer and float timestamp:
> T:epoch()
> T:timestamp()

epoch() is integer, and timestamp() is double, correct?

> 
>     Четверг, 19 августа 2021, 5:58 +03:00 от Timur Safin via
>     Tarantool-patches <tarantool-patches@dev.tarantool.org>:
>     * storage hints implemented for datetime_t values;
>     * proper comparison for indices of datetime type.
> 
>     Part of #5941
>     Part of #5946
> 
>     @TarantoolBot document
> 
>     Title: Storage support for datetime values
> 
>     It's now possible to store datetime values in spaces and create
>     indexed datetime fields.
> 
>     Please refer to
>     https://github.com/tarantool/tarantool/discussions/6244
>     <https://github.com/tarantool/tarantool/discussions/6244>
>     for more detailed description of a storage schema.
> 
> --
> Dmitry E. Oboukhov

Thanks,
Timur