-
-
Save CoDEmanX/1c7bfd266ad70da2d641 to your computer and use it in GitHub Desktop.
diff --git a/arangod/Aql/Executor.cpp b/arangod/Aql/Executor.cpp | |
index b3017b6..3eb0d11 100644 | |
--- a/arangod/Aql/Executor.cpp | |
+++ b/arangod/Aql/Executor.cpp | |
@@ -238,6 +238,9 @@ std::unordered_map<std::string, Function const> const Executor::FunctionNames{ | |
{ "DATE_TIMESTAMP", Function("DATE_TIMESTAMP", "AQL_DATE_TIMESTAMP", "ns|ns,ns,ns,ns,ns,ns", true, true, false, true, true) }, | |
{ "DATE_ISO8601", Function("DATE_ISO8601", "AQL_DATE_ISO8601", "ns|ns,ns,ns,ns,ns,ns", true, true, false, true, true) }, | |
{ "DATE_DAYOFWEEK", Function("DATE_DAYOFWEEK", "AQL_DATE_DAYOFWEEK", "ns", true, true, false, true, true) }, | |
+ { "DATE_DAYOFYEAR", Function("DATE_DAYOFYEAR", "AQL_DATE_DAYOFYEAR", "ns", true, true, false, true, true) }, | |
+ { "DATE_LEAPYEAR", Function("DATE_LEAPYEAR", "AQL_DATE_LEAPYEAR", "ns", true, true, false, true, true) }, | |
+ { "DATE_ISOWEEK", Function("DATE_ISOWEEK", "AQL_DATE_ISOWEEK", "ns", true, true, false, true, true) }, | |
{ "DATE_YEAR", Function("DATE_YEAR", "AQL_DATE_YEAR", "ns", true, true, false, true, true) }, | |
{ "DATE_MONTH", Function("DATE_MONTH", "AQL_DATE_MONTH", "ns", true, true, false, true, true) }, | |
{ "DATE_DAY", Function("DATE_DAY", "AQL_DATE_DAY", "ns", true, true, false, true, true) }, | |
@@ -245,6 +248,7 @@ std::unordered_map<std::string, Function const> const Executor::FunctionNames{ | |
{ "DATE_MINUTE", Function("DATE_MINUTE", "AQL_DATE_MINUTE", "ns", true, true, false, true, true) }, | |
{ "DATE_SECOND", Function("DATE_SECOND", "AQL_DATE_SECOND", "ns", true, true, false, true, true) }, | |
{ "DATE_MILLISECOND", Function("DATE_MILLISECOND", "AQL_DATE_MILLISECOND", "ns", true, true, false, true, true) }, | |
+ { "DATE_CALC", Function("DATE_CALC", "AQL_DATE_CALC", "ns,s,n", true, true, false, true, true) }, | |
// misc functions | |
{ "FAIL", Function("FAIL", "AQL_FAIL", "|s", false, false, true, true, true) }, | |
diff --git a/js/server/modules/org/arangodb/aql.js b/js/server/modules/org/arangodb/aql.js | |
index 9914ad5..02e95cb 100644 | |
--- a/js/server/modules/org/arangodb/aql.js | |
+++ b/js/server/modules/org/arangodb/aql.js | |
@@ -4494,6 +4494,25 @@ function AQL_DATE_DAYOFWEEK (value) { | |
} | |
//////////////////////////////////////////////////////////////////////////////// | |
+/// @brief return the ISO week date of the date passed (1..53) | |
+//////////////////////////////////////////////////////////////////////////////// | |
+ | |
+function AQL_DATE_ISOWEEK (value) { | |
+ 'use strict'; | |
+ | |
+ try { | |
+ var date = MAKE_DATE([ value ], "DATE_ISOWEEK"); | |
+ date.setUTCHours(0, 0, 0, 0); | |
+ date.setUTCDate(date.getUTCDate() + 4 - (date.getUTCDay() || 7)); | |
+ return Math.ceil((((date - Date.UTC(date.getUTCFullYear(), 0, 1)) / 864e5) + 1) / 7); | |
+ } | |
+ catch (err) { | |
+ WARN("DATE_ISOWEEK", INTERNAL.errors.ERROR_QUERY_INVALID_DATE_VALUE); | |
+ return null; | |
+ } | |
+} | |
+ | |
+//////////////////////////////////////////////////////////////////////////////// | |
/// @brief return the year of the date passed | |
//////////////////////////////////////////////////////////////////////////////// | |
@@ -4605,6 +4624,150 @@ function AQL_DATE_MILLISECOND (value) { | |
} | |
} | |
+//////////////////////////////////////////////////////////////////////////////// | |
+/// @brief add/subtract a time unit and return the calculated date of the date passed | |
+//////////////////////////////////////////////////////////////////////////////// | |
+ | |
+function AQL_DATE_CALC (value, unit, amount) { | |
+ 'use strict'; | |
+ | |
+ try { | |
+ var date = MAKE_DATE([ value ], "DATE_CALC"); | |
+ var unitGetter; | |
+ var unitSetter; | |
+ switch (unit.toLowerCase()){ | |
+ case "y": | |
+ case "year": | |
+ case "years": | |
+ unitGetter = "getUTCFullYear"; | |
+ unitSetter = "setUTCFullYear"; | |
+ break; | |
+ case "m": | |
+ if (unit == "M") { | |
+ unitGetter = "getUTCMonth"; | |
+ unitSetter = "setUTCMonth"; | |
+ } else { | |
+ unitGetter = "getUTCMinutes"; | |
+ unitSetter = "setUTCMinutes"; | |
+ } | |
+ break; | |
+ case "month": | |
+ case "months": | |
+ unitGetter = "getUTCMonth"; | |
+ unitSetter = "setUTCMonth"; | |
+ break; | |
+ case "d": | |
+ case "day": | |
+ case "days": | |
+ unitGetter = "getUTCDate"; | |
+ unitSetter = "setUTCDate"; | |
+ break; | |
+ case "h": | |
+ case "hour": | |
+ case "hours": | |
+ unitGetter = "getUTCHours"; | |
+ unitSetter = "setUTCHours"; | |
+ break; | |
+ // "m" already handled above | |
+ case "minute": | |
+ case "minutes": | |
+ unitGetter = "getUTCMinutes"; | |
+ unitSetter = "setUTCMinutes"; | |
+ break; | |
+ case "s": | |
+ case "second": | |
+ case "seconds": | |
+ unitGetter = "getUTCSeconds"; | |
+ unitSetter = "setUTCSeconds"; | |
+ break; | |
+ case "ms": | |
+ case "millisecond": | |
+ case "milliseconds": | |
+ unitGetter = "getUTCMilliseconds"; | |
+ unitSetter = "setUTCMilliseconds"; | |
+ break; | |
+ default: | |
+ // TODO: distinct error? | |
+ //WARN("DATE_CALC", INTERNAL.errors.ERROR_QUERY_INVALID_DATE_VALUE); | |
+ return null; | |
+ } | |
+ date[unitSetter](date[unitGetter]() + amount); | |
+ return date; | |
+ } | |
+ catch (err) { | |
+ WARN("DATE_CALC", INTERNAL.errors.ERROR_QUERY_INVALID_DATE_VALUE); | |
+ return null; | |
+ } | |
+} | |
+ | |
+//////////////////////////////////////////////////////////////////////////////// | |
+/// @brief return if year of the date passed is a leap year | |
+//////////////////////////////////////////////////////////////////////////////// | |
+ | |
+function AQL_DATE_LEAPYEAR(value) { | |
+ 'use strict'; | |
+ | |
+ try { | |
+ var yr = MAKE_DATE([ value ], "DATE_LEAPYEAR").getUTCFullYear(); | |
+ return !((yr % 4) || (!(yr % 100) && (yr % 400))); | |
+ } | |
+ catch (err) { | |
+ WARN("DATE_LEAPYEAR", INTERNAL.errors.ERROR_QUERY_INVALID_DATE_VALUE); | |
+ return null; | |
+ } | |
+} | |
+ | |
+//////////////////////////////////////////////////////////////////////////////// | |
+/// @brief return the day of the year of the date passed | |
+//////////////////////////////////////////////////////////////////////////////// | |
+ | |
+function AQL_DATE_DAYOFYEAR(value) { | |
+ 'use strict'; | |
+ | |
+ try { | |
+ var dayOfYearOffsets = [ | |
+ 0, | |
+ 31, // + 31 Jan | |
+ 59, // + 28 Feb * | |
+ 90, // + 31 Mar | |
+ 120, // + 30 Apr | |
+ 151, // + 31 May | |
+ 181, // + 30 Jun | |
+ 212, // + 31 Jul | |
+ 243, // + 31 Aug | |
+ 273, // + 30 Sep | |
+ 304, // + 31 Oct | |
+ 334 // + 30 Nov | |
+ ]; | |
+ | |
+ var dayOfLeapYearOffsets = [ | |
+ 0, | |
+ 31, // + 31 Jan | |
+ 59, // + 29 Feb * | |
+ 91, // + 31 Mar | |
+ 121, // + 30 Apr | |
+ 152, // + 31 May | |
+ 182, // + 30 Jun | |
+ 213, // + 31 Jul | |
+ 244, // + 31 Aug | |
+ 274, // + 30 Sep | |
+ 305, // + 31 Oct | |
+ 335 // + 30 Nov | |
+ ]; | |
+ var date = MAKE_DATE([ value ], "DATE_DAYOFYEAR"); | |
+ var m = date.getUTCMonth(); | |
+ var d = date.getUTCDate(); | |
+ //var ly = AQL_DATE_LEAPYEAR(date); | |
+ var yr = date.getUTCFullYear(); | |
+ var ly = !((yr % 4) || (!(yr % 100) && (yr % 400))); | |
+ return (ly ? (dayOfLeapYearOffsets[m] + d) : (dayOfYearOffsets[m] + d)); | |
+ } | |
+ catch (err) { | |
+ WARN("DATE_DAYOFYEAR", INTERNAL.errors.ERROR_QUERY_INVALID_DATE_VALUE); | |
+ return null; | |
+ } | |
+} | |
+ | |
// ----------------------------------------------------------------------------- | |
// --SECTION-- graph functions | |
// ----------------------------------------------------------------------------- | |
@@ -8546,6 +8709,9 @@ exports.AQL_DATE_NOW = AQL_DATE_NOW; | |
exports.AQL_DATE_TIMESTAMP = AQL_DATE_TIMESTAMP; | |
exports.AQL_DATE_ISO8601 = AQL_DATE_ISO8601; | |
exports.AQL_DATE_DAYOFWEEK = AQL_DATE_DAYOFWEEK; | |
+exports.AQL_DATE_DAYOFYEAR = AQL_DATE_DAYOFYEAR; | |
+exports.AQL_DATE_LEAPYEAR = AQL_DATE_LEAPYEAR; | |
+exports.AQL_DATE_ISOWEEK = AQL_DATE_ISOWEEK; | |
exports.AQL_DATE_YEAR = AQL_DATE_YEAR; | |
exports.AQL_DATE_MONTH = AQL_DATE_MONTH; | |
exports.AQL_DATE_DAY = AQL_DATE_DAY; | |
@@ -8553,6 +8719,7 @@ exports.AQL_DATE_HOUR = AQL_DATE_HOUR; | |
exports.AQL_DATE_MINUTE = AQL_DATE_MINUTE; | |
exports.AQL_DATE_SECOND = AQL_DATE_SECOND; | |
exports.AQL_DATE_MILLISECOND = AQL_DATE_MILLISECOND; | |
+exports.AQL_DATE_CALC = AQL_DATE_CALC; | |
exports.reload = reloadUserFunctions; | |
I created pull request arangodb/arangodb#1462 for some documentation corrections, then pushed my WIP date patch to my fork, which made it part of the PR. That's how PRs work on Github. I then moved the date patch to another branch to exclude it. You can find it in my fork repo:
https://github.com/CoDEmanX/ArangoDB/commits/aql-date-extra?author=CoDEmanX
I did not create a new PR for the date patch yet, because it's not ready. Not sure where to continue the discussion best... Maybe a new issue in my repo? (That's where the code is, this is just a pastebin).
You're right that a full year is over the moment the exact same time is reached one year later. It's not necessarily midnight, that was only an example. Starting at
2015-05-05 12:34:56.789, a difference of 1 year is reached the moment
2016-05-05 12:34:56.788 is over and we reach
2016-05-05 12:34:56.789 - the exact same time. That millisecond is not part of the previous virtual year.
Test: codepen.io
Problem: I need to use two different constants to convert to years, depending on leap years. An averaged constant would yield inaccurate results. Does it mean we need to "count" the number of leap years in the range, or is there a smarter way?
I guess there's not really an issue with calculating age based on current time and a birthday, unless the birthday is given with an exact time and the current time is past that time. The result would be numerically correct, but may not be perceived as correct (a person born at 15:00:00 is commonly assumed to be one year older from the birthday's 00:00:00 on, not 15:00:00).
BTW: moment.js supports ISO durations and stuff, maybe we could just use it in ArangoDB. Question is if they do it right / the way be want it to work. It would add unnecessary code to the code base and be slightly slower than a minimalist implementation.
Seems you got thumbs up about adding moment.js now. I thought of another solution yesterday and had to try to implement it. What do you think? I'm not exactly doing DB or performance-critical stuff for a living, so I imagine it could be improved on (and try/catched, etc).
function AQL_DATE_DIFF (date1, date2, unit) {
'use strict';
var d = {},
diff,
divisor,
// milliseconds per month, counting february as 28 days (compensating later)
msPerMonth = [26784e5, 24192e5, 26784e5, 2592e6, 26784e5, 2592e6, 26784e5, 26784e5, 2592e6, 26784e5, 2592e6, 26784e5],
msPerUnit = {
ms: 1,
s: 1e3, // 1000
m: 6e4, // 1000 * 60
h: 36e5, // 1000 * 60 * 60
d: 864e5, // 1000 * 60 * 60 * 24
w: 6048e5 // 1000 * 60 * 60 * 24 * 7
};
if (date1 === date2) {
return 0;
}
if (!unit) { // default unit
unit = "d";
}
// Unit is in our collection. Get the key as a divisor
if (divisor = msPerUnit[unit]) {
return (date2 - date1) / divisor;
}
// Month and year units
else if (unit === "M" || unit === "y") {
// Get year
d.y1 = date1.getUTCFullYear();
// Get month (index from 0 to 11)
d.m1 = date1.getUTCMonth();
// Get milliseconds in specified month, leap year compensated if february and leap year
d.month1ms = msPerMonth[d.m1] + ((d.m1 === 1 && AQL_DATE_LEAPYEAR(date1)) ? msPerUnit.d : 0);
// Get milliseconds passed since beginning of month
d.month1msOffset = date1 - Date.UTC(d.y1, d.m1);
// Repeat for date2
d.y2 = date2.getUTCFullYear();
d.m2 = date2.getUTCMonth();
d.month2ms = msPerMonth[d.m2] + ((d.m2 === 1 && AQL_DATE_LEAPYEAR(date2)) ? msPerUnit.d : 0);
d.month2msOffset = date2 - Date.UTC(d.y2, d.m2);
// Diff between YYYY-MM parts only.
diff = (d.y2 * 12 + d.m2) - (d.y1 * 12 + d.m1);
// Add diff between month offsets relative to the mean of ms in both months
diff += ((d.month2msOffset - d.month1msOffset) / ((d.month1ms + d.month2ms) / 2));
// return a 12/th of month span if the unit is year - maybe this isn't what we want to do?
return unit === "y" ? diff / 12 : diff;
}
else {
// invalid unit error?
}
}
Haven't even properly tested it, but it behaved as expected in the tests I did perform...
Forked your pen and added my AQL_DATE_DIFF to the second row. http://codepen.io/fullkornslimpa/pen/BoyJpP
It seems to behave like moment.js (with floating point numbers), but negative, due to using date2 - date1
(which I thought moment did too). It differs from moment at diffing 2016-02-29Z with 2016-02-28 23:59:59.999Z. Might need to look at that.
Turn out this is extremely similar to how moment does it (but not relying on other methods). https://github.com/moment/moment/pull/571/files
if (units === 'year' || units === 'month') {
diff = (this.daysInMonth() + that.daysInMonth()) * 432e5; // 24 * 60 * 60 * 1000 / 2
output = ((this.year() - that.year()) * 12) + (this.month() - that.month());
output += ((this - moment(this).startOf('month')) - (that - moment(that).startOf('month'))) / diff;
if (units === 'year') {
output = output / 12;
}
}
Diffing 2016-02-29Z
and 2016-02-28 23:59:59.999Z
with month
as a unit should give you the same as 1 / 1000 / 60 / 60 / 24 / 29
, since it's leap month. 1 / 1000 / 60 / 60 / 24 / 29
is 3.9910600255427846e-10
. Which is my result.
Moment.js yields 3.7335722819593786e-10
. I get that result when I'm using 31 days instead of 29. This is inaccurate, and will accumulate if you're diffing more than 1ms. I can't understand it though. Maybe their daysInMonth() is broken?
At a first glance, the numbers in your pen seem to be identical to the ones in mine?
Updated my pen to use the same dates, except that I added 28/29 Feb date comparisons at the top:
http://codepen.io/simran/pen/jbEmQB
It returns all 0 except for the milliseconds, which are 1 or -1 respectively (depending on the order). That seems to be all correct? (in my as well as in your pen)
BTW: I forgot to change the counter variable to 0 at some point, you should edit it:
for (let i=0; i<dates.length; i++){ // start from 0, not 1
So I think your code could be added to aql.js with some slight modifications (mainly casting to UTC dates by calling MAKE_DATE on input values and moving constants out of functions up to the top).
We could also add some convenience functions, like DATE_DAYSINMONTH()
and DATE_LASTDAYOFMONTH()
to get on par with MySQL (although it's hard to read in all upper-case... maybe add some extra underscores). More readable: DATE_FANCY(date, "lastDayOfMonth")
, DATE_FANCY(date, "theDayBeforeObamaBecamePresident")
, ... ;)
DATE_ISBIRTHDAY()
might be a bit of an overkill, but a way to surpass MySQL. Internally, it would simply do: SUBSTRING(date, 0, 10) == dateOfToday
I'm not sure yet if we should rename DATE_CALC()
to DATE_ADD()
and DATE_SUBTRACT()
. It seems redundant, but if ISO durations are accepted as well, then it suddenly makes sense to have two, because the ISO standard doesn't say anything about negative durations. It's pretty common though to prefix the duration with a minus. To negate an already exsiting duration string however, you would need to do CONCAT("-", durationStr)
in AQL, which looks a bit hacky to me.
Thinking a bit further, it can be beneficial to also support minus characters before every component in an ISO duration for mixed add/subtract operations. Getting the last day of a month would become as easy as:
DATE_CALC("2016-02", "P1M-1D")
in contrast to:
DATE_CALC(DATE_CALC("2016-02", "P1M"), "-P1D")
or:
DATE_CALC(DATE_CALC("2016-02", "month", 1), "day", -1)
Yes, I stole your numbers :), but tested with some other ones, while forgetting auto save was activated at the pen. I synced back with your numbers.
Fixed the starting index of the loop to 0 ✓ and changed the template to only focus on the mathematical differences between moments floating point numbers and my custom method (compensating for "negative" calculation).
Technically If we want "human" it shouldn't be too hard to implement, but I think like you that it isn't very practical and would need to be parsed/translated in a lot of cases, so I'd rather have it on the client too. Postgres age()
function returns something like X years Y mons Z days
, which I'm sure was useful back in the day when consonants were expensive ;)
Feel free to change anything you want. It's your PR, and you might want to make it behave consistent with other functions I haven't even looked at. I just thought the idea about me helping to add to a dbms code base was too cool to pass on :). The units in mine were mainly chosen for being shorter to write than "seconds" etc, so change them too if you want. :)
There's a weird js runtime bug which causes minor date miscalculations if there is no time zone. I think this is the same one moment.js is warning for in the terminal. You know more about this and AQL than me. Can UTC casting fix this (I tried and failed)? Maybe it won't be a problem with Arango anyway?
(new Date('1993-04-06 02:16:05') - new Date('1993-03-25 01:28:36')) / 1000 / 60 / 60 / 24
-> 11.99130787037037
(new Date('1993-04-06T02:16:05.000Z') - new Date('1993-03-25T01:28:36.000Z')) / 1000 / 60 / 60 / 24
-> 12.032974537037036
Edit: That was actually due to Daylight saving time, and not a js bug. Didn't consider that in my code, so my assumption that a day is 24h is not safe. Will update it later.
Edit 2: How do we actually want to treat DST?
Your support is much appreciated!
We don't need to worry about DST actually, because you either pass a number as date and JS/Arango will assume it's milliseconds since 1970, or a string and Date.UTC() will treat it as UTC date string (or turn it to UTC+0 if there is an explicit timezone offset given).
Because we will give the raw input dates to MAKE_DATE()
first, we'll get UTC dates back automatically, because it's handled there. There's no DST in UTC time, so your assumption is safe.
Note that you mustn't put the new
keyword in front of Date.UTC()
, and that it accepts strings only - for numbers, you can just use new Date()
.
In moment.js, it's moment.utc()
, and it seems to be more flexible about the input type.
Great! :) I should have figured that out from your previous comments. Thanks for your patience!
I switched to using moment.utc()
for the comparison codepen. Also needed Date.UTC()
instead of new Date()
to get the month offset, so I updated my comment.
This feels like an unorthodox way to handle code considering where we are, but this way I don't have to learn the dev environment, which is something I would definitely have to do for a PR. And I consider this a prototype anyway.
Regarding DATE_CALC()
vs DATE_ADD()
and DATE_SUBTRACT()
I agree and would prefer the latter two, both allowing negative values. Requiring CONCAT()
is indeed hacky.
Mysql has DATE_ADD()
and DATE_SUB()
, MS SQL has DATEADD()
(only). Postgres has operators instead. I'm not aware of any SQL dialect with DATE_CALC()
, and wouldn't it technically be adding, making DATE_ADD()
a more semantical name?
DATE_DAYS_IN_MONTH()
like MySQLs LAST_DAY()
would be useful. Shimming through the docs there already seems to be a IS_IN_POLYGON()
, so the extra underscores seems to be more consistent. Would DATE_LAST_DAY_OF_MONTH()
be an alias?
An alternative could be DATE_END(date, unit)
, which given M
or month
as unit would return the full date at the last ms of the month (technically incorrect by 1ms, but more useful), which could either be wrapped by DATE_DAY()
to behave like LAST_DAY()
or used with DATE_DIFF()
to get the remaining time of the month (off by 1 ms). Not sure how practical it would actually be, but I like my functions more versatile :)
Similarly DATE_ISBIRTHDAY()
could be DATE_MD()
(like the existing functions DATE_MONTH()
and DATE_DAY()
combined) so DATE_MD(u.birthdate) == DATE_MD(DATE_NOW())
could filter out users you'd want to spam out gift vouchers for, or something. Not really a great name, though. MySql has EXTRACT(part FROM date) which is more versatile, but I think the part formats like YEAR_MONTH
looks odd too.
Maybe you should ask in the issue which date functions people feel are missing? I think you've covered the most important ones.
Changed the pen some more to make better sense for a moment.js bug report.
Also did performance tests (in the pen), and I really don't think it's a problem (I did before, but that was when I had a different method in mind which would traverse through the time span and check for leap months). It's faster than moment.js too (with v8 anyway).
So here's an implementation of your code in actual ArangoDB:
CoDEmanX/ArangoDB@9f9bc31
Windows 64bit build:
https://github.com/CoDEmanX/ArangoDB/releases/tag/aql-date-extra
It seems to work, but I haven't tested it thoroughly. One issue I saw is that if an invalid date is supplied, no error is raised and 0 returned. If I pass the same date to DATE_ISO8601() however, an error is raised. Not sure what is causing this havior. MAKE_DATE() is called internally, and should raise the error already...
DATE_ADD() would indeed be more in line with what other DBMS call this function, and I also like it semantically better than CALC. Will do that change soon and also add ISO duration support.
A days-in-month function would be trivial to do, and last-day may also be handy. However, both can be implemented with current AQL. Is it overkill to add all sorts of convenience functions? Or will it make date handling much more appealing, so that it's justified?
I'm against a generic DATE_END() function. End of year is not very useful (always year-12-31), for last day in month there's DATE_CALC / DATE_ADD and we can cover this in the documentation (take year and month, add one month, subtract one day), end of week is rather vague because of different first weekdays, and all smaller units would be time- and not date related, and can be easily calculated like last-day-of-month (plus it's not very useful to know that the last minute of 2015-05-05 05:30
is 2015-05-05 05:59:59.999
, and not necessarily as expected).
Likewise, birthday testing could be added to the documentation using existing date functions, and it's also possible to "extract" date components already. If at all, I would suggest a DATE_FORMAT() function: DATE_FORMAT(date, "YYYYMM")
. But that's pretty much like humanize(), which rather belongs on the client-side.
What I forgot: should the plural forms of time units be accepted as well? I supported them in my original patch, but not in the current implemention. They would be trivial to add, just not sure if it's needed. In case of DATE_ADD(), the singular forms seem fine. But for DATE_DIFF(), the plural forms make much more sense...
DATE_DIFF(date1, date2, "d")
DATE_DIFF(date1, date2, "day") // odd, because it's very rare that it returns 1
DATE_DIFF(date1, date2, "days")
"Hey computer, what is the DATE_DIFF
erence between date1
and date2
in days?"
I also considered to switch order of parameters in DATE_ADD:
"Hey computer, ADD
1 year to date
"
Not sure if DATE_ADD(amount, unit, date)
is a good idea, but DATE_ADD(date, amount, unit)
seems like a good choice:
DATE_ADD(date, 8, "hours")
There's one drawback: If ISO duration support is added, then the 2nd argument can either be a number or a string. If we keep the signature date, unit, amount
, then it's always a string (function signature doesn't change). One the other hand, we could assume that it's an amount if it's a number, and an ISO duration otherwise...
Days in month in AQL:
LET dates = [
"2014-02-15",
"2016-02-15",
"2016-03-01",
"2017-09-30"
]
LET daysInMonth = [29, 31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31]
FOR d IN dates
RETURN [d, daysInMonth[DATE_MONTH(d) == 2 && DATE_LEAPYEAR(TO_STRING(DATE_YEAR(d))) ? 0 : DATE_MONTH(d)]]
Or with currently available AQL functions:
FOR d IN dates
LET yr = DATE_YEAR(d)
RETURN [d, daysInMonth[DATE_MONTH(d) == 2 && (!((yr % 4) || (!(yr % 100) && (yr % 400)))) ? 0 : DATE_MONTH(d)]]
DATE_QUARTER() is buggy, but I fixed it - forgot to adapt it for JS, which is 0-based for months.
Added an isNaN() check to MAKE_DATE(), because it didn't warn if new Date() failed to create a proper date. Had to add another check in DATE_DIFF() still, to not return a number in case one or both dates are invalid (I made MAKE_DATE() return null and check if either or both of the dates given to DATE_DIFF() are null after the call and return null itself if that's the case). A JS throw to execute the catch-block, which warns and returns null, did not work - gave me "internal error while optimizing AST". I guess I would need to set canThrow in Executor.cpp to true for that, but it would actually issue a redundant warning. So just returning null in DATE_DIFF() and let MAKE_DATE() warn about it:
Great job again 👍
Don't forget to remove "maybe this isn't what we want to do?" from my comment before submitting the PR. The alternative would be a separate method for years, but with the ambiguity of "year" as a unit, I think this is a better implementation, and moment.js does the same. It just needs to be documented.
Regarding DATE_IS_BIRTHDAY()
I'm in general much in favor of "syntactic sugar" if the use case is strong enough (like classes, fat arrows, destructuring assignment, or str.includes()
in ES2015). However if it isn't, I think it's suboptimal and could even increase the threshold for learning a language. DATE_IS_BIRTHDAY()
is very readable and easy to understand, for checking someone's birthday, but I think when you feel it's justified to design such specific features you might be better off looking for a terse but more versatile alternative, which once you've learned can be used to solve other problems. If a function is very specific/limited, people might not even look for it in the manual or try "googling" using terms similar enough, or if they have read about it they might not remember once they actually need it. Our brains create neural pathways when we solve problems, which is how we learn and create memories. We then generally prefer to apply the same patterns to different situations to save time and energy, and we tend to overall ignore excessive information ("cognitive miser").
People might also use it for hacks like DATE_IS_BIRTHDAY(DATE_ADD(e.date, 7, "d"))
to check if the anniversary of an event is in a week, because they'd find it easier than using DATE_MONTH(DATE_ADD(e.date, 7, "d")) == DATE_MONTH(DATE_NOW()) && DATE_DAY(DATE_ADD(e.date, 7, "d")) == DATE_DAY(DATE_NOW())
. For instance in CSS text-indent
seems to be mostly used to hide something for eyes, but not screen readers, or replacing images for high pixel density screens. I don't think I've used it to actually indent text in years.
I don't think it's a big deal with DATE_IS_BIRTHDAY()
either way, but I just wanted to add a little philosophy to the context. There are no generic rules to follow. Some people like languages like Go because it's "small" and consistent while others like Scala, for the opposite reason.
Maybe DATE_MATCH(date1, date2, units)
and/or DATE_PART(date, units)
, where units would be an array of units? DATE_PART()
is pretty standard in sql (but not with array units) and it would be better if you want to check "was user born in february 13?" (or you could use DATE_MONTH()
and DATE_DAY()
).
I'm not sure I want DATE_END()
either. It wouldn't be very useful for presentation. I was thinking it would be useful in queries like "Find all events between the date of event x and the end of the month/year/day of the same event", but that's probably not a very common query either.
Units
I think time units as well as other units are typically expressed as singular outside a given context, and hence when thought of as "arguments" it makes sense with singular, but I agree days
just looks better than day
, so I also have a slight preference for plural. MS SQL uses singular and abbreviations: https://msdn.microsoft.com/en-us/library/ms189794.aspx, while moment.js as you know uses plural (only?).
I understand why you don't want the same letter for month and minute. Have you considered "i" for minute? That would make DATE_DIFF()
s unit arguments compatible with php's date_format, and MySql also uses "%i" for minutes.
DATE_ADD args
DATE_ADD(date, amount, unit)
feels more intuitive to me, and it's more similar to mysql.
Regarding the drawback, I'm not a C++ or Arango dev, so I can't be of much help. Not sure if you're talking about an extra line of code in Executor.cpp, better handling of memory, or if it's plain impossible with ISO duration.
Oh, and I'm not getting any notifications here and have to manually check for updates. Maybe you could enable issue tracking on your fork as previously suggested?
Yeah, me neither... Let's continue here: CoDEmanX/ArangoDB#1
Discussion continued from arangodb/arangodb#317 (comment):
I think you might be limiting your math using "days" as the smallest component? Comparing two dates, hasn't a year fully passed when it's the same exact time a year later, not at 00:00?
Assuming Date1 is "2015-01-01 00:01" and Date 2 is "2016-01-01 00:00" and that the behavior of DATE_DIFF with "years" is to return a floored number I'd expect the result to be
0
. If the time of Date1 instead was 00:00 I'd expect the result to be1
.Maybe the implementation is more useful if it returned decimals, compatible with https://docs.arangodb.com/Aql/NumericFunctions.html
So
DATE_DIFF(Date1, Date2, "years")
would be something like 0.999.. andFLOOR(DATE_DIFF(Date1, Date2, "years"))
would be 0.