Skip to content
This repository has been archived by the owner on Jun 12, 2024. It is now read-only.

Date arithmetic results in different data type in new evaluator #516

Closed
pbowen-oc opened this issue Nov 2, 2021 · 13 comments
Closed

Date arithmetic results in different data type in new evaluator #516

pbowen-oc opened this issue Nov 2, 2021 · 13 comments
Assignees
Milestone

Comments

@pbowen-oc
Copy link

The old evaluator returned a numeric value for date + integer. The new evaluator returns a date value for the same expression.

date plus zero.xform.txt

Old Evaluator:
Screenshot 2021-11-02 001304

New Evaluator:
Screenshot 2021-11-02 001317

@pbowen-oc pbowen-oc added this to the Next milestone Nov 2, 2021
@MartijnR
Copy link
Member

MartijnR commented Nov 2, 2021

https://docs.google.com/spreadsheets/d/1KNmYzbTUXDZuNTOWG-n0HZlBQx09Lmz_MiEdKksaDrE/edit#gid=0

Screen Shot 2021-11-02 at 10 22 13 AM

  • check what ODK Collect does - It shows an integer (clearly not incorporating timezone, so that might be a remaining difference between old and new evaluator - Collect does seem to have a bug because it doesn't convert to date for type="date")
  • test '2021-11-30' + 1 directly in evaluator
  • create issue: date arithmetic returns date instead of number enketo/enketo#866

@pbowen-oc
Copy link
Author

We observed the same discrepancy with date1 - date2. In the old evaluator, it returns the number of days between the dates. In the new evaluator it returns a nonsensical date. I'll add a test form and screenshots for this scenario.

@MartijnR
Copy link
Member

MartijnR commented Nov 2, 2021

Thanks. Makes sense that for similar dates, it would return a value a short time after 1-1-1970 (e.g. 1 would be 1970-01-02). Will add that failure.

  • add additional example for same bug

@MartijnR
Copy link
Member

MartijnR commented Nov 16, 2021

There is a PR for this! Things seem to be a little quiet in Enketo Dev world at the moment, so if eager you could perhaps deploy that PR branch for testing.

update: approved

@pbowen-oc
Copy link
Author

We are seeing an issue with this. Date1 - Date2 is now producing a number, but not exactly as expected. October 10, 2021 - October 1, 2021 is producing 9.208 days (9 days and 5 hours) rather than 9 days as the old evaluator did. It looks like the timezone is being factored in somehow, but only for one of the dates.

I'll add an example form and screenshots a little later.

@pbowen-oc
Copy link
Author

@MartijnR - Here is an example for the issue in my last comment:

Test form:
date-subtract-date-sm.xml.txt

Screenshot (legacy on left, new evaluator on right):
Screenshot 2022-01-13 122601

@MartijnR
Copy link
Member

enketo/enketo#865

@MartijnR
Copy link
Member

I created a fix PR. I'll update the other outstanding PR 144 so you can start testing with it.

@MartijnR
Copy link
Member

Here it is: enketo/openrosa-xpath-evaluator#148. Please remind me to mark that PR as final if your testing finds no issues.

@pbowen-oc
Copy link
Author

@MartijnR - Thanks!

@MartijnR
Copy link
Member

There is a discussion about what the behavior should be when arithmetic exceeds DST boundaries though...

@pbowen-oc
Copy link
Author

For us, we don't expect values with erroneous timezones or two values with different timezones (other than DST vs ST) so I don't think the implementation would matter (unless I am missing the implication).

The testing looks good so far. We're continuing, but this is likely done and ready to have a final PR.

@pbowen-oc
Copy link
Author

Looks good in testing.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants