Round the to date down to the start of the day
#224
Conversation
🦋 Changeset detectedLatest commit: f55cc18 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
dubiousdavid
changed the title
to date to the start of the current day.to date
dubiousdavid
changed the title
to dateto date down to the start of the day
| last36Months: { from: 'now/d-36M', to: 'now' }, | ||
| last48Months: { from: 'now/d-48M', to: 'now' }, | ||
| last60Months: { from: 'now/d-60M', to: 'now' }, | ||
| last12Months: { from: 'now/d-12M', to: 'now/d' }, |
There was a problem hiding this comment.
If the to date is not supposed to be inclusive, should we be doing the following https://github.com/smartprocure/contexture/blob/main/packages/provider-elasticsearch/src/utils/dateUtil.js#L59 It seems like we are somewhat inconsistent on what each lastX uses as the to expression
There was a problem hiding this comment.
I'm actually thinking the following might be the best - now+1d/d-1ms. It should cache nicely while also returning results from the current day. The cache expires after six hours, so each time there is a cache miss the query will return any new records. Thoughts?
There was a problem hiding this comment.
I'm actually thinking the following might be the best -
now+1d/d-1ms. It should cache nicely while also returning results from the current day. The cache expires after six hours, so each time there is a cache miss the query will return any new records. Thoughts?
It makes more sense to me personally to include the current date in the lastX expressions, should we adjust the from to also do now+1d/d in that case so that we are not picking up an extra day in the beginning if the end date include the current day?
There was a problem hiding this comment.
Elastic datemath still gets converted to a timestamp so the cache is ineffective.
Ignore this. Per our talk, /d rounds to the start of the day so we're good 👍
You can see it in the elastic query produced for the issued date filter in spending
Also we may want to factor dateUtil into the new util package since it looks identical to the one used for other providers.
There was a problem hiding this comment.
@dshishkov In a meeting with Bradley we decided to be inclusive on the entire current day for either past/future ranges. This means that to will always be now+1d/d-1ms for past ranges and from will always be now/d for future ranges.
stellarhoof
force-pushed
the
feature/pin-dates
branch
from
c6f5af3 to
be16abc
Compare
|
Coverage after merging feature/pin-dates into main will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
todate to the end of the current day for alllastXDaysandlastXMonthsdate ranges.fromdate to the start of the day for alllastXDaysdate ranges.See: https://www.elastic.co/guide/en/elasticsearch/reference/2.4/common-options.html#date-math