PR discussion / Duplicate testDateCalcInvalid in ./js/server/tests/aql-functions-date.js

friday commented 9 years ago

I've finally managed to build Arango and have run the unit tests (on a vagrant box, as I couldn't get v8 to compile in os x). Sorry I'm a bit late.

Saw you've just submitted a PR, and got a travis build fail. This is likely the reason? I started fixing the other jshint errors.

CoDEmanX commented 9 years ago

Nice. That error shouldn't occur with the patch from PR, because I removed the old testDateCalc tests there. Thanks to that error, I noticed that I forgot to rename a date add test to testDateAddInvalid. Will push a fix to aql-date branch. Note that aql-date-extra is outdated.

friday commented 9 years ago

Aha. Sorry. Will switch to aql-date.

CoDEmanX commented 9 years ago

Ok, just force-pushed the fix to aql-date branch. Make sure to grab the latest (you can check for testDateCalcInvalid in aql-functions-date.js, it shouldn't occur anymore): https://github.com/CoDEmanX/ArangoDB/commits/aql-date

CoDEmanX commented 9 years ago

Also changed in aql-date-extra, but it is behind origin/devel. Aardvark's lib.test.js was apparently removed upstream, and I did some changes to its app files - but I should not had added these files to the commits. They will be auto-generated when Aardvark is rebuilt next time upstream. I excluded them from aql-date branch.

friday commented 9 years ago

:+1:

Are you able to run the js tests now? I had to rebuild with make clean, so back to the waiting game...

Should I run ./scripts/unittest shell_server_aql or ./scripts/unittest all btw?

CoDEmanX commented 9 years ago

Just pushed small changes (fix missing semicolons, re- definition of vars). Waiting for Travis.

The RegExp can't be split... I could turn it to RegExp constructor, but that would mean to escape backslashes and make it even longer in total. Literal RegExp are said to be faster, but I guess just in terms of parsing -which happens once on server start. Hm...

I'm not sure how to run the tests at all. I'm pretty sure I can't until I build with relative option and sort out all the Ruby issues - but I'm not keen to do that. I would greatly appreciate if you ran the tests. I assume the latter will cover it, although there might be an option to run just the AQL / JS based unit tests.

friday commented 9 years ago

If they're ok with it, I think you can do this (not tested yet)

// ignore maxlen jshint check
// jshint -W130
var ISODurationRegex = /^P(?:(?:(\d+)Y)?(?:(\d+)M)?(?:(\d+)W)?(?:(\d+)D)?)?(?:T(?:(\d+)H)?(?:(\d+)M)?(?:(\d+)(?:\.(\d+))?S)?)?$/i; // jshint ignore:line
// jshint +W130

I found similar overrides in the codebase, so I think it's justified.

friday commented 9 years ago

It worked. I also changed the leapyear check to not use ! in a confusing way. Should I push it or PR?

friday commented 9 years ago

Testrun: shell_server_aql [ Fail ] js/server/tests/aql-functions-date.js -> testDateDayOfYear Failed; Verbose message: at assertion #30: assertEqual: ([ 358 ]) is not equal to ([ 359 ]) (Error at exports.jsUnity.defaultAssertions.assertEqual (/vagrant/src/js/common/modules/jsunity/jsunity.js:103:19) at js/server/tests/aql-functions-date.js:738:28 at Array.forEach (native) at Object.testDateDayOfYear (js/server/tests/aql-functions-date.js:736:14) at Object.exports.jsUnity.run (/vagrant/src/js/common/modules/jsunity/jsunity.js:497:21) at Object.Run (/vagrant/src/js/common/modules/jsunity.js:137:24) at js/server/tests/aql-functions-date.js:1256:9 at RunTest (/vagrant/src/js/common/modules/jsunity.js:196:10) at eval (:1:63) at Function.eval (:1:113) at Function.actions.defineHttp.callback (/vagrant/src/js/actions/_admin/app.js:701:16) ) -> testDateAddInvalid Failed; Verbose message: at assertion #3: unexpected error format while calling [RETURN DATE_ADD(1, 1)]: (false) does not evaluate to true (Error at exports.jsUnity.defaultAssertions.assertTrue (/vagrant/src/js/common/modules/jsunity/jsunity.js:66:19) at assertQueryError (/vagrant/src/js/server/modules/org/arangodb/aql-helper.js:248:92) at Object.testDateAddInvalid (js/server/tests/aql-functions-date.js:896:7) at Object.exports.jsUnity.run (/vagrant/src/js/common/modules/jsunity/jsunity.js:497:21) at Object.Run (/vagrant/src/js/common/modules/jsunity.js:137:24) at js/server/tests/aql-functions-date.js:1256:9 at RunTest (/vagrant/src/js/common/modules/jsunity.js:196:10) at eval (:1:63) at Function.eval (:1:113) at Function.actions.defineHttp.callback (/vagrant/src/js/actions/_admin/app.js:701:16) ) -> testDateAdd Failed; Verbose message: at assertion #8: assertEqual: ([ "2401-03-31T00:00:00.000Z" ]) is not equal to ([ "2101-03-31T00:00:00.000Z" ]) (Error at exports.jsUnity.defaultAssertions.assertEqual (/vagrant/src/js/common/modules/jsunity/jsunity.js:103:19) at js/server/tests/aql-functions-date.js:969:28 at Array.forEach (native) at Object.testDateAdd (js/server/tests/aql-functions-date.js:963:14) at Object.exports.jsUnity.run (/vagrant/src/js/common/modules/jsunity/jsunity.js:497:21) at Object.Run (/vagrant/src/js/common/modules/jsunity.js:137:24) at js/server/tests/aql-functions-date.js:1256:9 at RunTest (/vagrant/src/js/common/modules/jsunity.js:196:10) at eval (:1:63) at Function.eval (:1:113) at Function.actions.defineHttp.callback (/vagrant/src/js/actions/_admin/app.js:701:16) )

CoDEmanX commented 9 years ago

Make a PR, please.

Are those all errors, or is there more to come?

friday commented 9 years ago

Those are all (with shell_server_aql).

CoDEmanX commented 9 years ago

Ok, turns out year 1000 is not a leap year, therefore it's 358.

Not sure about the DATE_ADD(1,1) test, there are two possible solutions:

A. Change the test to assertQueryWarningAndNull and invalid date value B. Change test to assertQueryWarningAndNull and let DATE_ADD() return an argument number mismatch error. A date value error might be confusing, because the actual error is raised by the 2nd parameter, because of an argument type mismatch. If the 2nd is numeric, than there must be a 3rd argument of type string.

(["2401-03-31T00:00:00.000Z" ]) is not equal to ([ "2101-03-31T00:00:00.000Z" ]) is really just a stupid mistake introduced by wildly copy and pasting ;)

friday commented 9 years ago

Sure it's not a leap year? It's valid according to your method and wikipedia (not that the latter is failsafe...).

friday commented 9 years ago

./scripts/unittest all takes a loooooong time...

CoDEmanX commented 9 years ago

Oh really? It shouldn't... 1000 % 4 is a yes, so leap year. 1000 % 100 is a yes too, so not a leap year (exception to % 4 rule). 1000 % 400 is 200, so no exception of the exception, which makes it a non-leap year - no?

return date_leapyear("1000-12-24") // gives false in my test, so correct?

I guess shell_server_aql is sufficient, there are no other unit tests for the date functions. Although I don't know where assumptions.js is used exactly.

friday commented 9 years ago

You're right! My bad

friday commented 9 years ago

I calculated too quickly in my head, thinking it'd match AQL_DATE_LEAPYEAR(). https://en.wikipedia.org/wiki/1000 says otherwise. The means of calculating leap years have changed, so the current method isn't historically correct for all old dates. I'd say we'll ignore that?

CoDEmanX commented 9 years ago

Ah... that might well be. Some date related thing are changed retrospectively. And ISO 8601 states that dates before 1582 aren't safe anyway - because of holes in the calendar (couple days were skipped). I agree on ignoring these kind of issues and add a little comment to the docs.

Can you post me the jslint fixes?

friday commented 9 years ago

PR https://github.com/CoDEmanX/ArangoDB/pull/3

friday commented 9 years ago

You fixed most of them yourself :)

CoDEmanX commented 9 years ago

Added weeks as unit for date_add() / date_subtract(), which I forgot to implement, but already mentioned in docs :laughing:

Changed argument checks and unittests accordingly. Hope it's correct.

CoDEmanX commented 9 years ago

Still missing a general note on ISO8601 dates, but we can add that later at any time. Or add a link to Wikipedia :camel:

friday commented 9 years ago

all-test completed. Lots of (all?) ruby testsuits failing. Could be Arango doesn't like v1.9.3 as much as Ubuntu does. Don't think the other ones concern this PR either?

    [  Fail ] js/server/tests/shell-skiplist-correctness.js
          -> testCorrectnessNonUnique Failed; Verbose message:
[ArangoError 4: internal error] - [ArangoError 4: internal error]
  at Error (native)
  at Object.SkipListCorrSuite.testCorrectnessNonUnique (js/server/tests/shell-skiplist-correctness.js:84:12)
  at Object.exports.jsUnity.run (/vagrant/src/js/common/modules/jsunity/jsunity.js:497:21)
  at Object.Run (/vagrant/src/js/common/modules/jsunity.js:137:24)
  at js/server/tests/shell-skiplist-correctness.js:301:9
  at RunTest (/vagrant/src/js/common/modules/jsunity.js:196:10)
  at eval (:1:63)
  at Function.eval (:1:121)
  at Function.actions.defineHttp.callback (/vagrant/src/js/actions/_admin/app.js:701:16)
          -> testCorrectnessUnique Failed; Verbose message:
[ArangoError 4: internal error] - [ArangoError 4: internal error]
  at Error (native)
  at Object.SkipListCorrSuite.testCorrectnessUnique (js/server/tests/shell-skiplist-correctness.js:154:12)
  at Object.exports.jsUnity.run (/vagrant/src/js/common/modules/jsunity/jsunity.js:497:21)
  at Object.Run (/vagrant/src/js/common/modules/jsunity.js:137:24)
  at js/server/tests/shell-skiplist-correctness.js:301:9
  at RunTest (/vagrant/src/js/common/modules/jsunity.js:196:10)
  at eval (:1:63)
  at Function.eval (:1:121)
  at Function.actions.defineHttp.callback (/vagrant/src/js/actions/_admin/app.js:701:16)
          -> testCorrectnessSparse Failed; Verbose message:
[ArangoError 4: internal error] - [ArangoError 4: internal error]
  at Error (native)
  at Object.SkipListCorrSuite.testCorrectnessSparse (js/server/tests/shell-skiplist-correctness.js:224:12)
  at Object.exports.jsUnity.run (/vagrant/src/js/common/modules/jsunity/jsunity.js:497:21)
  at Object.Run (/vagrant/src/js/common/modules/jsunity.js:137:24)
  at js/server/tests/shell-skiplist-correctness.js:301:9
  at RunTest (/vagrant/src/js/common/modules/jsunity.js:196:10)
  at eval (:1:63)
  at Function.eval (:1:121)
  at Function.actions.defineHttp.callback (/vagrant/src/js/actions/_admin/app.js:701:16)
    [  Fail ] js/server/tests/shell-skiplist-index.js
          -> testUniqueDocuments Failed; Verbose message:
[ArangoError 4: internal error] - [ArangoError 4: internal error]
  at Error (native)
  at Object.SkipListSuite.testUniqueDocuments (js/server/tests/shell-skiplist-index.js:205:23)
  at Object.exports.jsUnity.run (/vagrant/src/js/common/modules/jsunity/jsunity.js:497:21)
  at Object.Run (/vagrant/src/js/common/modules/jsunity.js:137:24)
  at js/server/tests/shell-skiplist-index.js:744:9
  at RunTest (/vagrant/src/js/common/modules/jsunity.js:196:10)
  at eval (:1:63)
  at Function.eval (:1:115)
  at Function.actions.defineHttp.callback (/vagrant/src/js/actions/_admin/app.js:701:16)
          -> testQueriesDocuments Failed; Verbose message:
[ArangoError 4: internal error] - [ArangoError 4: internal error]
  at Error (native)
  at Object.SkipListSuite.testQueriesDocuments (js/server/tests/shell-skiplist-index.js:481:20)
  at Object.exports.jsUnity.run (/vagrant/src/js/common/modules/jsunity/jsunity.js:497:21)
  at Object.Run (/vagrant/src/js/common/modules/jsunity.js:137:24)
  at js/server/tests/shell-skiplist-index.js:744:9
  at RunTest (/vagrant/src/js/common/modules/jsunity.js:196:10)
  at eval (:1:63)
  at Function.eval (:1:115)
  at Function.actions.defineHttp.callback (/vagrant/src/js/actions/_admin/app.js:701:16)
          -> testQueriesDocumentsSparse Failed; Verbose message:
[ArangoError 4: internal error] - [ArangoError 4: internal error]
  at Error (native)
  at Object.SkipListSuite.testQueriesDocumentsSparse (js/server/tests/shell-skiplist-index.js:589:20)
  at Object.exports.jsUnity.run (/vagrant/src/js/common/modules/jsunity/jsunity.js:497:21)
  at Object.Run (/vagrant/src/js/common/modules/jsunity.js:137:24)
  at js/server/tests/shell-skiplist-index.js:744:9
  at RunTest (/vagrant/src/js/common/modules/jsunity.js:196:10)
  at eval (:1:63)
  at Function.eval (:1:115)
  at Function.actions.defineHttp.callback (/vagrant/src/js/actions/_admin/app.js:701:16)
          -> testQueriesDocumentsPartial Failed; Verbose message:
[ArangoError 4: internal error] - [ArangoError 4: internal error]
  at Error (native)
  at Object.SkipListSuite.testQueriesDocumentsPartial (js/server/tests/shell-skiplist-index.js:714:18)
  at Object.exports.jsUnity.run (/vagrant/src/js/common/modules/jsunity/jsunity.js:497:21)
  at Object.Run (/vagrant/src/js/common/modules/jsunity.js:137:24)
  at js/server/tests/shell-skiplist-index.js:744:9
  at RunTest (/vagrant/src/js/common/modules/jsunity.js:196:10)
  at eval (:1:63)
  at Function.eval (:1:115)
  at Function.actions.defineHttp.callback (/vagrant/src/js/actions/_admin/app.js:701:16)
    [  Fail ] js/server/tests/shell-skiplist-rm-performance-timecritical-noncluster.js
          -> testDeletionPerformance Failed; Verbose message:
[ArangoError 4: internal error] - [ArangoError 4: internal error]
  at Error (native)
  at Object.SkipListPerfSuite.testDeletionPerformance (js/server/tests/shell-skiplist-rm-performance-timecritical-noncluster.js:85:20)
  at Object.exports.jsUnity.run (/vagrant/src/js/common/modules/jsunity/jsunity.js:497:21)
  at Object.Run (/vagrant/src/js/common/modules/jsunity.js:137:24)
  at js/server/tests/shell-skiplist-rm-performance-timecritical-noncluster.js:126:9
  at RunTest (/vagrant/src/js/common/modules/jsunity.js:196:10)
  at eval (:1:63)
  at Function.eval (:1:148)
  at Function.actions.defineHttp.callback (/vagrant/src/js/actions/_admin/app.js:701:16)
     [Success] js/server/tests / [aql-arithmetic.js, aql-array-access.js, aql-attribute-access.js, aql-bind.js, aql-call-apply.js, aql-complex.js, aql-cross.js, aql-distinct.js, aql-dynamic-attributes.js, aql-edges-noncluster.js, aql-escaping.js, aql-explain-noncluster.js, aql-failures-noncluster.js, aql-fullcount.js, aql-functions-list.js, aql-functions-misc.js, aql-functions-numeric.js, aql-functions-string.js, aql-functions-types-cxx.js, aql-functions-types.js, aql-functions.js, aql-general-graph.js, aql-graph-visitors.js, aql-graph.js, aql-hash-noncluster.js, aql-is-in-polygon.js, aql-logical.js, aql-modify-noncluster-serializetest.js, aql-modify-noncluster.js, aql-operators.js, aql-optimizer-collect-count.js, aql-optimizer-collect-into.js, aql-optimizer-collect-methods.js, aql-optimizer-costs.js, aql-optimizer-dynamic-bounds.js, aql-optimizer-filters.js, aql-optimizer-indexes.js, aql-optimizer-keep.js, aql-optimizer-plans.js, aql-optimizer-rule-interchange-adjacent-enumerations-noncluster.js, aql-optimizer-rule-move-calculations-down.js, aql-optimizer-rule-move-calculations-up.js, aql-optimizer-rule-move-filters-up.js, aql-optimizer-rule-remove-collect-into.js, aql-optimizer-rule-remove-filter-covered-by-index.js, aql-optimizer-rule-remove-redundant-calculations.js, aql-optimizer-rule-remove-redundant-or.js, aql-optimizer-rule-remove-redundant-sorts.js, aql-optimizer-rule-remove-sort-rand.js, aql-optimizer-rule-remove-unnecessary-calculations.js, aql-optimizer-rule-remove-unnecessary-filters.js, aql-optimizer-rule-replace-or-with-in.js, aql-optimizer-rule-use-index-for-sort.js, aql-optimizer-rule-use-index-range.js, aql-optimizer-stats-noncluster.js, aql-optimizer-v8.js, aql-parse.js, aql-primary-index-noncluster.js, aql-queries-collection.js, aql-queries-fulltext.js, aql-queries-geo.js, aql-queries-noncollection.js, aql-queries-optimizer-in-noncluster.js, aql-queries-optimizer-limit-noncluster.js, aql-queries-optimizer-ref-noncluster.js, aql-queries-optimizer-sort-noncluster.js, aql-queries-optimizer.js, aql-queries-simple.js, aql-queries-variables.js, aql-query-cache-noncluster.js, aql-range.js, aql-ranges.js, aql-refaccess-attribute.js, aql-refaccess-variable.js, aql-relational.js, aql-simple-attributes.js, aql-skiplist-noncluster.js, aql-subquery.js, aql-ternary.js, aql-variables.js, aql-within-rectangle.js, aql-ranges-combined-01.js, aql-ranges-combined-02.js, aql-ranges-combined-03.js, aql-ranges-combined-04.js, aql-ranges-combined-05.js, aql-ranges-combined-06.js, aql-ranges-combined-07.js, aql-ranges-combined-08.js, aql-ranges-combined-09.js, aql-ranges-combined-10.js, aql-ranges-combined-11.js, aql-ranges-combined-12.js, aql-ranges-combined-13.js, aql-ranges-combined-14.js, aql-ranges-combined-15.js, aql-ranges-combined-16.js]
* Testrun: shell_server_aql
    [  Fail ] js/server/tests/aql-functions-date.js
          -> testDateDayOfYear Failed; Verbose message:
at assertion #30: assertEqual: ([ 358 ]) is not equal to ([ 359 ])
(Error
  at exports.jsUnity.defaultAssertions.assertEqual (/vagrant/src/js/common/modules/jsunity/jsunity.js:103:19)
  at js/server/tests/aql-functions-date.js:738:28
  at Array.forEach (native)
  at Object.testDateDayOfYear (js/server/tests/aql-functions-date.js:736:14)
  at Object.exports.jsUnity.run (/vagrant/src/js/common/modules/jsunity/jsunity.js:497:21)
  at Object.Run (/vagrant/src/js/common/modules/jsunity.js:137:24)
  at js/server/tests/aql-functions-date.js:1256:9
  at RunTest (/vagrant/src/js/common/modules/jsunity.js:196:10)
  at eval (:1:63)
  at Function.eval (:1:113)
  at Function.actions.defineHttp.callback (/vagrant/src/js/actions/_admin/app.js:701:16)
)
          -> testDateAddInvalid Failed; Verbose message:
at assertion #3: unexpected error format while calling [RETURN DATE_ADD(1, 1)]: (false) does not evaluate to true
(Error
  at exports.jsUnity.defaultAssertions.assertTrue (/vagrant/src/js/common/modules/jsunity/jsunity.js:66:19)
  at assertQueryError (/vagrant/src/js/server/modules/org/arangodb/aql-helper.js:248:92)
  at Object.testDateAddInvalid (js/server/tests/aql-functions-date.js:896:7)
  at Object.exports.jsUnity.run (/vagrant/src/js/common/modules/jsunity/jsunity.js:497:21)
  at Object.Run (/vagrant/src/js/common/modules/jsunity.js:137:24)
  at js/server/tests/aql-functions-date.js:1256:9
  at RunTest (/vagrant/src/js/common/modules/jsunity.js:196:10)
  at eval (:1:63)
  at Function.eval (:1:113)
  at Function.actions.defineHttp.callback (/vagrant/src/js/actions/_admin/app.js:701:16)
)
          -> testDateAdd Failed; Verbose message:
at assertion #8: assertEqual: ([ "2401-03-31T00:00:00.000Z" ]) is not equal to ([ "2101-03-31T00:00:00.000Z" ])
(Error
  at exports.jsUnity.defaultAssertions.assertEqual (/vagrant/src/js/common/modules/jsunity/jsunity.js:103:19)
  at js/server/tests/aql-functions-date.js:969:28
  at Array.forEach (native)
  at Object.testDateAdd (js/server/tests/aql-functions-date.js:963:14)
  at Object.exports.jsUnity.run (/vagrant/src/js/common/modules/jsunity/jsunity.js:497:21)
  at Object.Run (/vagrant/src/js/common/modules/jsunity.js:137:24)
  at js/server/tests/aql-functions-date.js:1256:9
  at RunTest (/vagrant/src/js/common/modules/jsunity.js:196:10)
  at eval (:1:63)
  at Function.eval (:1:113)
  at Function.actions.defineHttp.callback (/vagrant/src/js/actions/_admin/app.js:701:16)
)
* Testrun: http_server
     [  Fail ] api-admin-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-aqlfunction-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-async-spec-timecritical.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-attributes-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-batch-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-collection-noncluster-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-collection-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-compatibility-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-cursor-spec-noncluster.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-cursor-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-database-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-document-create-noncluster-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-document-create-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-document-delete-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-document-key-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-document-read-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-document-update-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-documents-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-edge-noncluster-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-edge-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-edges-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-explain-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-export-spec-timecritical-noncluster.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-foxx-manager-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-general-graph-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-graph-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-http-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-import-noncluster-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-index-hash-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-index-skiplist-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-index-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-pipelining-spec-noncluster.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-query-analysis-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-simple-all-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-simple-any-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-simple-example-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-simple-first-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-simple-fulltext-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-simple-geo-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-simple-hash-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-simple-modify-example-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-simple-range-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-simple-skiplist-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-statistics-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-structures.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-transactions-noncluster-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-traversal-edgecollection-noncluster-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-traversal-noncluster-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-users-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-wal-noncluster-spec.rb: Whole testsuite failed!
exit code was 1
* Testrun: ssl_server
     [  Fail ] api-admin-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-aqlfunction-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-async-spec-timecritical.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-attributes-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-batch-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-collection-noncluster-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-collection-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-compatibility-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-cursor-spec-noncluster.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-cursor-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-database-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-document-create-noncluster-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-document-create-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-document-delete-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-document-key-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-document-read-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-document-update-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-documents-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-edge-noncluster-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-edge-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-edges-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-explain-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-export-spec-timecritical-noncluster.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-foxx-manager-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-general-graph-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-graph-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-http-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-import-noncluster-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-index-hash-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-index-skiplist-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-index-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-pipelining-spec-noncluster.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-query-analysis-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-simple-all-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-simple-any-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-simple-example-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-simple-first-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-simple-fulltext-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-simple-geo-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-simple-hash-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-simple-modify-example-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-simple-range-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-simple-skiplist-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-statistics-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-structures.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-transactions-noncluster-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-traversal-edgecollection-noncluster-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-traversal-noncluster-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-users-spec.rb: Whole testsuite failed!
exit code was 1
     [  Fail ] api-wal-noncluster-spec.rb: Whole testsuite failed!
exit code was 1
Overall state: Fail
   Suites failed: 104 Tests Failed: 11
2015-09-10T23:04:16Z [24649] INFO ArangoDB has been shut down
removing out/log-24645 out/data-24645
Server has terminated.

CoDEmanX commented 9 years ago

No, not related to this PR. Seems like a systematic problem with the ruby tests indeed, they can't all fail.

New travis build gives still error Line is too long :(

friday commented 9 years ago

I was thinking more of the "testCorrectness"-ones at the top. The ruby suits failing has to be some dependency/conf-failure on my side.

Do you think your Vagrant-problems are due to Windows? I've been running Vagrant before and had no problems with it this time (except for not giving the box enough memory to compile v8 the first time).

Gonna catch some sleep. Great work on this branch! :) (and also your work on the docs :+1: )

CoDEmanX commented 9 years ago

I'm not using Vagrant (had several issues there as well regarding VirtualBox), but a Cygwin environment directly on my host system.

I'll try to fix jshint. Sleep well!

CoDEmanX commented 9 years ago

jshint appears to be picky about comment types, // jshint -W101 failed, /* jshint -W101 */ works.

Some more tests failing, waiting for rebuild. Passing Infinity as arg raised a strange collection not found error... not a good idea and not used anywhere else, thus removed from test (also NaN). One test failed because it was based on "M" / "m" being different time units. Made it "i" now.

CoDEmanX commented 9 years ago

Some issues with the following:

// 2 warnings
DATE_ADD(null, 1, 'year')
DATE_ADD(false, 1, 'year')
DATE_ADD([], 1, 'year')
DATE_ADD({}, 1, 'year')

// these are actually accepted, but shouldn't...
DATE_ADD(DATE_NOW(), '', 'year')
DATE_ADD(DATE_NOW(), '1', 'year')
DATE_ADD(DATE_NOW(), null, 'year')
DATE_ADD(DATE_NOW(), false, 'year')
DATE_ADD(DATE_NOW(), [], 'year')

friday commented 9 years ago

jshint appears to be picky about comment types, // jshint -W101 failed, /* jshint -W101 */

Sorry! ./scripts/unittest doesn't seem to test jshinting, while make unittests does, I didn't feel satisfied having to recompile the tests every time, so I relied on my in-editor jshinting instead, which takes // jshint -W101.

You mentioned having tried Vagrant. I was curious where you got stuck and if I could help if you wanted to try again. On the other hand I have some problems with the Ruby tests . It's hard to know if the problem was with Vagrant, Virtualbox or the distro though.

Have you gotten an email from Frank about signing a Apache Contributor's licence document regarding your work? I just did, and it says my work "on the documentation". While I have made a minor contribution to the docs in november, you've done much more and recent docs work. I don't think there's been a mistake, but I just want to make sure he didn't mix us up somehow.

DATE_ADD(DATE_NOW(), '', 'year')
DATE_ADD(DATE_NOW(), '1', 'year')
DATE_ADD(DATE_NOW(), null, 'year')
DATE_ADD(DATE_NOW(), false, 'year')
DATE_ADD(DATE_NOW(), [], 'year')

Just type-check typeof amount !== "number" if value isn't an iso-duration? You probably already have, by now.

Regarding:

DATE_ADD(null, 1, 'year')
DATE_ADD(false, 1, 'year')
DATE_ADD([], 1, 'year')
DATE_ADD({}, 1, 'year')

I think you could do something similar

Also you might want to change than to then here:

// if amount is not a number, than it must be an ISO duration string

CoDEmanX commented 9 years ago

I signed a CLA a while ago already (also for small docs changes). If you wanna be mentioned as contributor for the date diffing (you deserve it), then you should sign one too, because it's somewhat required. It isn't for very minor contributions AFAIK, but this is a bit different. Everyone one can see that you worked on this on GitHub, and they need to be sure you don't sue them because you didn't formally agree to their license.

Jan modified the type checks, will see if it's already fixed. typeof for numbers is not safe BTW, Infinity and Number.NaN cast to "number" if I'm not mistaken.

You're right about the typo, will leave a comment for Jan.

CoDEmanX commented 9 years ago

Do you think we should add a DATE_DAYS_IN_MONTH function? I added an example to the docs how to do it in a user query, but still.

Vagrant didn't work for me because of the virtual box version. I had to try at least 4 older versions to make it boot. Might be fixed in more recent versions.

Next problem is that I couldn't make the box' network work, so that it has access to my host system. In a non-vagrant box, that would be easily setup using shared folders or network shares. But it didn't really work for the vagrant box, not sure why. Might need some changes somewhere in the guest system.

Thus, I had to download all files inside the box, and had to do that via internet because I could not make it access my host system. Not sure if it persisted the files or if it deleted them on restart...

In short: it was really troublesome to set up and because I'm not a Linux expert, it is time consuming to do simple things because of permission restrictions and do on (and a big lack of good gui tools IMO).

I also stumbled upon v8 memory requirements, but that was rather easy to fix.

I wonder if there's a way to do incremental builds, on either OS. The build scripts seem to be geared towards full, clean builds for releases. Haven't tried to compile the generated sln in visual studio though after minor changes.

friday commented 9 years ago

DATE_DAYS_IN_MONTH() would be useful, and we already have the logic, but maybe it's too late to add new features for 2.7? You'd have to take that up with Jan. I guess it could also be a unit for DATE_FORMAT().

Vagrant has to officially release support for virtualbox versions before you can use them. You can hack some rb confs to add new versions if you're impatient (I did when VBox 5 RC was released), but since they've caught up by now it's not very useful. VBox 5 is a huge performance improvement btw.

Regarding synced/shared networks I think in order to get that working you need config.vm.network "public_network" which is slower. I prefer config.vm.network "private_network", ip: "[local ip here]", and then using config.vm.synced_folder, ex: config.vm.synced_folder ".", "/home/vagrant/src", create: true. This will sync the directory of your Vagrantfile to "/home/vagrant/src".

On a mac I would also add nfs: true and use bindfs to get better performance and access rights. Not sure exactly how that works, and even less so on a Windows host.

I wonder if there's a way to do incremental builds, on either OS.

I think unless you make clean it's verifying which steps are done and which ones has to be rebuilt, based on changes and flags set by previous builds. Then it (re)builds only what's necessary.

CoDEmanX commented 9 years ago

I don't think it's too late, there's no RC yet. Good point about DATE_FORMAT(). I should remove the example from the docs then if there's going to be a built-in function.

BTW: I consider to change the dateMap attributes in DATE_FORMAT() to functions. It should increase performs - at least for simple format strings, because it would calculate the values lazily. I have mixed feelings about it however. A quick comparison showed 2x speedup for "%y", but I'm not entirely sure about complex format strings. It seemed to me that lazy evaluation can minimally decrease performs for complex format strings (all placeholders used). The same placeholder many times didn't really make a difference, which is strange. V8 must be optiminzing this... I did that test with years only, not sure about other date functions.

Does sync folder "copy" files in both directions? I'm rather looking for an actual network share, read/write access on both sides but the actual data only on the host system.

make pack-win64 deletes the entire Build folder, not sure what would happen if I commented out rm -rf Build64 - but I should try that. I remember some console log messages however, that stated that the build parameter for incremental build were ignored because of some other setting, that seem to enforce a full rebuild.

CoDEmanX commented 9 years ago

If you wanna compare the performance of DATE_FORMAT(), here's thecode:

    var dateMap = {
      "%t": function(){ return date.getTime() },
      "%o": function(){ return dateStr },
      "%w": function(){ return AQL_DATE_DAYOFWEEK(dateStr) },
      "%y": function(){ return dateStr.slice(0, 4 + offset) },
      "%m": function(){ return dateStr.slice(5 + offset, 7 + offset) },
      "%d": function(){ return dateStr.slice(8 + offset, 10 + offset) },
      "%h": function(){ return dateStr.slice(11 + offset, 13 + offset) },
      "%i": function(){ return dateStr.slice(14 + offset, 16 + offset) },
      "%s": function(){ return dateStr.slice(17 + offset, 19 + offset) },
      "%f": function(){ return dateStr.slice(20 + offset, 23 + offset) },
      "%x": function(){ return zeropad(AQL_DATE_DAYOFYEAR(dateStr), 3) },
      "%k": function(){ return zeropad(AQL_DATE_ISOWEEK(dateStr), 2) },
      "%l": function(){ return +AQL_DATE_LEAPYEAR(dateStr) },
      "%q": function(){ return AQL_DATE_QUARTER(dateStr) },
      "%%": function(){ return "%" } // Allow for literal "%Y" using "%%Y"
      //"%": "" // Not reliable, because Object.keys() does not guarantee order
    };
    var exp = new RegExp(Object.keys(dateMap).join("|"), "gi"); 
    format = format.replace(exp, function(match){
      return dateMap[match.toLowerCase()]();
    });

And some AQL query for a test (e.g. Aardvark to see the executation time):

FOR i IN 1..1000000
    RETURN DATE_FORMAT(i*3000000000, "try different format strings here")

friday commented 9 years ago

Does sync folder "copy" files in both directions? I'm rather looking for an actual network share, read/write access on both sides but the actual data only on the host system.

It is a network share to your host. You can use Samba or NFS for it, which are networking protocols. The default option for virtualbox is better than samba though.

BTW: I consider to change the dateMap attributes in DATE_FORMAT() to functions. It should increase performs - at least for simple format strings, because it would calculate the values lazily. I have mixed feelings about it however. A quick comparison showed 2x speedup for "%y", but I'm not entirely sure about complex format strings. It seemed to me that lazy evaluation can minimally decrease performs for complex format strings (all placeholders used). The same placeholder many times didn't really make a difference, which is strange. V8 must be optiminzing this... I did that test with years only, not sure about other date functions.

A function invocation gives a tiny overhead. Maybe this negates the performance boost of lazy evaluation in this case, given that the computations are also very fast? I'm guessing this doesn't even sum up to a microsecond? In my DATE_DIFF() tests a year diff was almost 10 times faster than moment.js, but moment took ~20 microseconds (on a 2.5ghz dual core i5 while compiling stuff in the background), so noone will probably notice a difference. Maybe it's better to go with lazy, because you can add support for more units, but only get a performance hit on the ones actually used?

I'm pretty sure v8 does pretty heavy optimization as it gives Chrome a competitive edge. But if you get strange results, make sure to try directly in the console (or better yet, the node REPL). For instance, I got strange results running babel in codepen for my perf tests, which I couldn't replicate in the console.

I'm actually not sure I want DATE_FORMAT() to begin with. I wouldn't use it personally, for the reasons we mentioned. I can see a use case for it, and many other dbms's has this feature. I think it's a reasonable addition, but one that would better not be rushed or designed with a focus on comparing dates. For instance I think it'd be very limited without multiple formats per units (such as "January", "Jan", "1", or "01"). And to be able to do that we couldn't stick to single letter lowercase units. Picking abbreviations like "%m" now, could make it harder to add support for formatting months without the initial zero later (we couldn't use %m for "1" and %mm for "01"). Likewise, starting out case-insensitive and adding case-sensitivity later would break compatibility with old queries.

I also think using it like DATE_FORMAT(...) > DATE_FORMAT(...) would not be very performant. In that case "glue", such as spaces, "-" or ":" doesn't need to be supported and regex isn't actually needed. It could be done much faster with SUBSTRING() (but again, the difference might be very small).

Maybe DATE_COMPARE() could return 1, 0 or -1 depending on if date1 is higher, equal to or lower than date2? DATE_COMPARE() doesn't imply returning a boolean. It would make it more flexible, but harder to use and not as pretty :/ You could do a "x >= y" with DATE_COMPARE(x, y) != -1. Maybe an alias for DATE_COMPARE(...) == 0 could be added? !DATE_COMPARE(...) would work too I think, although it would be counterintuitive?

Sorry I'm responding slowly, and being more opinionated than helpful with testing etc. :/ It's your PR and hence your call. I do have another project in need of my focus now, but it was fun and insightful to help and discuss implementation details with you :)

CoDEmanX commented 9 years ago

// if amount is not a number, than it must be an ISO duration string

Can't find it, must have reworked that section.

I did another comparison and here are the results:

// lazy vs. eager evaluation
// 28s / 47s
for i in 1..1000000
    return date_format(i*3000000000, "The German notation for dates is: %d.%m.%y, whereas the time of day is denoted the same: %h:%i:%s.%f. Literal % can be used in format strings. %%y.%%m.%%y %%h:%%i:%%s")

// 21s (also %y), %q 27s / 40s (also %y and %q)
for i in 1..1000000
    return date_format(i*3000000000, "%d")

// 215s / 94s
for i in 1..1000000
    return date_format(i*3000000000, "%y %m %d %h %i %s %f %q %l %w %k %x %t %o %y %m %d %h %i %s %f %q %l %w %k %x %t %o %y %m %d %h %i %s %f %q %l %w %k %x %t %o %y %m %d %h %i %s %f %q %l %w %k %x %t %o %y %m %d %h %i %s %f %q %l %w %k %x %t %o %y %m %d %h %i %s %f %q %l %w %k %x %t %o %y %m %d %h %i %s %f %q %l %w %k %x %t %o %y %m %d %h %i %s %f %q %l %w %k %x %t %o %y %m %d %h %i %s %f %q %l %w %k %x %t %o %y %m %d %h %i %s %f %q %l %w %k %x %t %o")

// 52s / 49s
for i in 1..1000000
    return date_format(i*3000000000, "%y %m %d %h %i %s %f %q %l %w %k %x %t %o")

Conclusion: there's a notable speed up for more common-case format strings if evaluated lazily, and a minimal impact on performence if all placeholders are used once - but it's unlikely that someone prints out all of them, and the difference is small enough to ignore it. The only case where eager eval is way faster is the same placeholders occur many times - the ratio is not that bad however: 1x every placeholder vs. 10x every placeholder doesn't make lazy eval 10x slower, but only ~2.5x - which is pretty acceptable given that it's a real corner case. I'll submit the change to lazy eval.

If we do a lot faster than moment.js and produce the same results, then that's really great! To be fair, we would need to compare moment.js integrated in Arango with our implementation for an unbiased test.

I can see your point with DATE_FORMAT(). I added it because it's pretty simple to implement, if you exclude certain advanced things that moment.js and others offer. Some people may prefer it over manual CONCAT()ing dates and performance it quite okay. Nonetheless, I put a note in the docs to avoid it if possible and not to run it against millions of dates:

https://github.com/CoDEmanX/ArangoDB/blob/aql-date/Documentation/Books/Users/Aql/DateFunctions.mdpp#L343

The use case I have in mind is to create something like "2/2015" (%q/%y) as document attribute for later queries. It can be also used for non-trivial comparisons of course, some that aren't possible with substring-ing dates (such as quarters, weeks, days of year... or actually combinations thereof, because the individual functions that return numbers can be used instead for single comparisons).

I doubt it will be a heavily used feature, and thus breaking existing queries should not be so much of a problem - 2.6 broke backward compatibility of a lot of graph functions, which had probably a much bigger impact. We would need to mention a breaking-change in the docs of course.

We could also think more about how it should work, and come up with a better design and disable it in the current code base for now. If we want a full-blown date formatting function however, that supports "Jan", "January" etc., then we might wanna use moment.js for it after all (but only formatting). The question remains, which locales should be supported. If English-only is the answer, then it's easily added to the current implementation. Same goes for multi-char placeholders, as long as we don't use Object.keys() but an extra array of keys in a certain order to replace multi-char placeholders before single-char placeholders (at I think that should work).

There are a couple corner cases of course: Should %yyyy give 4 digit years? Or only have %y for 2 digit years (which are discouraged by JS, ECMA etc.), and %yy for 4 digit years?

Or %y for non-padded years (year 5 would stay "5"), %yy to pad it to 2 chars if necessary, but 753 would stay "753" (we wouldn't want to truncate it, would we?), %yyy for a max padding of 3 zeroes and %yyyy for max padding of "0"?

%f for un-padded ms, and %fff for 3 digit ms, but no support for %ff or anything >= %ffff?

Day of year un-padded or padded to 3, or also paddded to 2, which doesn't make much sense?

Should be really make it case-sensitive, or are there available, meaningful placeholders for month and weekday names? (are there any other names we need?!)

Interesting idea for DATE_COMPARE()... 0 evals to false, 1 and -1 to true, but it would reverse the current logic, which seems very counter-intuitive to me (equal dates = false?!). The function name does indeed not require to return a truthy value if the dates match. If we properly document it, then it's acceptable IMO, yet people may stumble upon it. Who is familiar with comparator functions for sort algorithms will understand it though. However, we wouldn't really use it for sorting, and I wonder how often people would use the actual return value. We could return a substring instead to allow users to compare dates, but that should be a separate function (DATE_PART()? the existing DATE_FORMAT()?)

CoDEmanX commented 9 years ago

@friday: Please have a look / test: https://github.com/CoDEmanX/ArangoDB/commit/1c796ea8dc62d46c7a8cf4a8ba201640ed4291e4

I did not test it yet, but it should be working.

friday commented 9 years ago

That's fast! Looks good except:

In AQL_DATE_DAYS_IN_MONTH() I think you might have forgotten + 1 here?

var month = date.getUTCMonth();

CoDEmanX commented 9 years ago

You're right, I messed that up. The getter returns 0-based months as well, but I was convinced it would be 1-based, and only the constructor zero-based. Is there some other code affected as well?

friday commented 9 years ago

Seems you're right. :) Date handling in JS is confusing!

Edit: Or was I? haha!

CoDEmanX commented 9 years ago

Replaced the commit, please see: https://github.com/CoDEmanX/ArangoDB/commit/c401ed11eccc5d539f0d6b8a460091f3b0f270e6

The month names were also affected, I removed the first (null) entry.

friday commented 9 years ago

Looks less hacky without null :)

DATE_FORMAT()

A multi-locale implementation would be a huge undertaking. Those also change every now and then, and would have to be updated. English-only is more reasonable. Locale kind of killed the original github date issue by overcomplicating it.

There are a couple corner cases of course: Should %yyyy give 4 digit years? Or only have %y for 2 digit years (which are discouraged by JS, ECMA etc.), and %yy for 4 digit years?

I'd like %y for a full and unpadded year, %yyyy for 4 digits and %yy for two more than the other suggestion. It's very intuitive to read and write. That would of course imply %mm is a padded variant of %m etc. I think multi-letter units is better than case-sensitivity as well. I don't really see a case for single-digit or 3 digits-padding for years. That should also solve your question about padding years? I never thought about padding years, but it could be useful to not confuse year 72 with year 1972. Assuming most people living today reads the former as the latter I think it's a good idea to support padding.

%f for un-padded ms, and %fff for 3 digit ms, but no support for %ff or anything >= %ffff?

Maybe %f for one digit (deciseconds), %ff (centiseconds) for two and %ffff for 3 (milliseconds)?

Day of year un-padded or padded to 3, or also paddded to 2, which doesn't make much sense?

Unpadded only I think. Padding can be added later if it's wanted, by increasing the digits to the amount of minimum digits.

Should we really make it case-sensitive, or are there available, meaningful placeholders for month and weekday names?

Seems you solved that one? :)

Performance

Regarding your performance tests I think the ones where lazy was slower are veeery unlikely to be used, and if anyone write such long format strings, those are probably 215μs they can spare. Still surprised by the difference though.

Changing DATE_COMPARE() return value?

it would reverse the current logic, which seems very counter-intuitive to me (equal dates = false?!)

In a way I think it's just as logical that "what's the comparison of 5 and 5" returns 0 (like diff) but this wasn't how I thought of it either first.

I wonder how often people would use the actual return value

Probably never. I think it would have to be 0, 1 or -1. Not the actual residue of the calculation. The romantic version we ditched before DATE_MATCH(...), could be an alias for DATE_COMPARE(...) == 0, but I don't think it's needed.

CoDEmanX commented 9 years ago

I'm not exactly happy with %g, multi-char placeholders would actually improve the situation.

Placeholder	Padding	Output 1	Output 2	Output 3	Comment
%y	none	-753	1	753	✔
%yy	2?	-753	01	15 for 2015 ?	would it abbreviate recent years similar to MySQL? (confusing!)
%yyyy	4	-0753 ?	0001	0753	How to handle negative years? ISO8601 requires "-" + 6 padding
%yyyyyy?	6	-000753	+000001	+000753	ISO8601 conform padding if negative years occur
%m	none	8	9	10	✔
%mm	2	08	09	10	✔
%d	none	4	14	24	✔
%dd	2	04	14	24	✔
%h	none	8	9	10	✔
%hh	2	08	09	10	✔
%s	none	8	9	10	✔
%ss	2	08	09	10	✔
%f?	none	1	21	321	like that? Or would it only refer to "3" in 3rd example and be 0 otherwise?
%ff?	?	?	?	?
%fff	3	001	021	321	or did you really mean 4x `f`?
%w	none	0	4	6	✔
%l	none	0	1	-	✔
%q	none	1	2	4	✔
%t	none	-12345	0	1234567	we could pad it to the maximum possible width, but doesn't make any sense
%z	none				automatically padded actually, except for negative dates! Do we therefore need `%zz`?
%k?	none	5	35	53	ISO week, change to `%ww`? (`%w` is weekday)
%kk	2	05	35	53	it's not possible to compare without padding!
%x	none	3	53	153	✔
%xx?	?	?	?	?	padding to 2 doesn't make sense, and having it pad to 3 would be confusing
%xxx?	3	003	053	153	required for comparisons!
%a	none	28	29	31	✔ there's no month with <10 days, so always 2 chars
%n	(3)	Jan	Feb	Dec	✔ abbreviated month name
%nn	none	January	May	August	full month name, another placeholder for space-padded string?
%e	(3)	Sun	Mon	Wed	✔ abbreviated weekday name
%ee	none	Sunday	Monday	Wednesday	full week name, another placeholder for space-padded string?

CoDEmanX commented 9 years ago

Padding is not for human consumption only, it is required for string-based comparisons.

Some more ideas:

%m and %mm for (un-)padded month number, %mmm for abbrev. month name, %mmmm for full month name?
%w for weekday number, %www for abbrev. weekday name, %wwww for full weekday name?
Keep current logic of DATE_COMPARE() and add optional leapling handling (default = 0, Feb 28th = -1, Mar 1st = 1) to make it more than just a substring comparer. Leave other partial date comparisons up to the user (0, -1, 1 is rarely I needed I think)

The current code does not handle positive years > 9999 BTW., ISO strings switch from 9999-12-31 to +010000-01-01. We should probably handle such situations, not only negative years.

friday commented 9 years ago

I'm a bit confused, without being able to see the input values. What I meant with %f was the number of "f"s to represent the number of decimals:

Input: 0,873 seconds

Placeholdrer	Output
%f	9
%ff	88
%fff	873

But I wasn't thinking about padding. As a decimal value, padding would have to be on the right side. Input 1: 0s Input 2: 0.3s

Placeholdrer	Output 1 w padding	Output 2 w/o padding	Output 2 w padding
%f	0	3	3
%ff	00	3	30
%fff	000	3	300

"Output 1 w/o padding" could also be expected to be an empty string. But assuming the format is "%s,%f" you wouldn't want "1,"and "0," with no decimals. If we returned empty strings we'd also have to add placeholder decimals which would be removed if there isn't any decimal. And given decimals separators are written as "." or "," depending on locale that would make matters even more complicated. I think that if you specify %f it's fair to assume that you want at least one decimal. If we go this way I'm not sure with or without padding?

%yywould take the last to digits, and in case of a single-digit year I think it would need to left pad. So "Summer of '%yy" would output "Summer of '04" regardless of the year was 2004, 1904 ore just 4. Not sure about "Summer of '-04". MySql's 2 digit input for years is indeed strange. I don't think we have that problem.

Not sure about %mmmm, but I don't have any suggestion of my own. Need to think about your other suggestions and look at other implementations. Will try to reply tomorrow.

CoDEmanX commented 9 years ago

Rounding is not something I would like to introduce to the formatting function, because it's a loss in information. A formatted string should parse to the original input. %f should return unpadded ms IMO, and %fff a padded version. It's also the cleanest to implement.

I'm also against %yy for the same reason. "Summer of '69" is probably something that is used so rare, that we don't need it - and you can still do LEFT(DATE_YEAR(), 2).

Here's a branch with a different implementation of date_format(): https://github.com/CoDEmanX/ArangoDB/commits/aql-date-alt

It does not enforce the padding lengths:

// "-2015 -002015 -002015"
return date_format("-002015", "%y %yyyy %yyyyyy")

// "753 0753 +000753"
return date_format("0753", "%y %yyyy %yyyyyy")

What I hadn't thought of: multi-char placeholders can lead to ambiguities. %mmonth will not turn out as 4month, but 04onth. I therefore added %%%, which is simply removed. But it acts as a separator to resolve the ambiguity: %m%%%month will output 4month. A single literal % is now removed. That's not a problem anymore, because the RegExp is not constructured from Object.keys() anymore, but a sorted array (%%% first, then by placeholder length).

It's not possible to print consecutive literal % however, because %%%% will be recognized as %%% + %, and both are removed. I don't see a way to resolve this, but it's unlikely to be needed anyway.

CoDEmanX commented 9 years ago

I think it can be solved by using a different character. %+ caused issues because + is a special char in regexp. But %& seems to work without complications. Literal % can be added by doubling, also consecutively. A literal %& is also possible by escaping the %: %%&.

friday commented 9 years ago

Rounding is not something I would like to introduce to the formatting function, because it's a loss in information. A formatted string should always parse to the original input.

This doesn't make sense. DATE_FORMAT() is already lossy, because it can very easily be used to discard information required to parse the result back to the input date. If someone wants to use deciseconds rather than milliseconds or use the more sloppy 2-digit standard (which is quite common) for years, that doesn't make it harder to parse than if they completely leave out decimals or the year. Date formatting can still be lossless if you want it to be, and I would expect anyone who has that requirement to use the function accordingly.

The default stopwatch in android Lollipop is precise to the decisecond. If I want to use it to track the laptime of a running competition, and then enter them into a custom web app (which I actually have), it doesn't make sense to have it formatted with three digits, because that would just add information. In a way that's lossy too, because it adds false information implying a preciseness which isn't there.

I'm not saying this particular case is one we need to cover, just that it exists.

%f should return unpadded ms IMO, and %fff a padded version. It's also the cleanest to implement.

I don't think that's unreasonable. You're very right about the latter part, if we round the value. If the ms part is 999 and we round it to one or two digits, the result would be 0. To stay accurate we would then have to add 1 to the seconds. I think this is the most fair implementation, but indeed not the cleanest. If we could control the execution order of the functions this could be done by starting with the smallest units and raise some kind of flag, which the seconds computation would listen to, but that would only make sense for decimals (hence only milliseconds), and it's quite a big change for such a small win. We could substring instead however.

function getDecimals(date, digits) {
  return LEFT_PAD(date.getUTCMilliseconds().toString(), 3).substr(0, digits);
}

I think I've seen some kind of pad function (perhaps written by you?), but the syntax is likely different.

What I hadn't thought of: multi-char placeholders can lead to ambiguities. %mmonth will not turn out as 4month, but 04onth.

This is pretty common. Moment.js, .NET and PHP doesn't have strftime-like "%" placeholders so most letters are unsupported. In .NET you have to use StringBuilder strings along with date.ToString(). In PHP you escape them with like `"\summer \of yy". Moment.js might not handle it at all (couldn't find in the docs and haven't tried).

friday commented 9 years ago

I'm not a c++ dev, but it seems to me strftime() is used internally in Arango's c++ code (like ararangod/VocBase/server.cpp). Does that mean you could wrap it instead of using a custom date_format function?

Pros: Standardized (it's popular and ported to most C-like languages, hence lower threshold for anyone used to it, and compatibility with formats in server languages). Less work Well tested Better performance is likely? No documentation, just write some example and link to http://strftime.org/ or https://docs.python.org/2/library/datetime.html#strftime-and-strptime-behavior

Cons: Choices are already made and it's a bit limited. Doesn't have days in month, and perhaps there's something else missing? Case-sensitive We might want to adjust DATE_COMPARE() units accordingly?

friday commented 9 years ago

Oh yeah, it does Microseconds... I guess that's not going to work. Also has "Locale’s appropriate date and time representation", which we wouldn't want I think.

CoDEmanX / ArangoDB