feat: add support for dates to max and min functions. #428
Conversation
🦋 Changeset detectedLatest commit: dc6a2a8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
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 |
|
I also checked with |
More templates
@tanstack/angular-db
@tanstack/db
@tanstack/db-ivm
@tanstack/electric-db-collection
@tanstack/query-db-collection
@tanstack/react-db
@tanstack/rxdb-db-collection
@tanstack/solid-db
@tanstack/svelte-db
@tanstack/trailbase-db-collection
@tanstack/vue-db
commit: |
|
OK, managed to make it work. The PR is ready for review. |
There was a problem hiding this comment.
I left some comments with regard to the code changes. I believe the changes here are indeed enough for Dates to work with aggregation functions.
On the longer term, i'm not sure if this is the approach we want to take. Turning the value into a number with Number seems brittle and works for Dates but may not work for other data types.
We may want a bigger refactor such that min and max work on any data type that has a total ordering. This means we need a custom comparator function as well as a bottom value and a top value for that specific data type. For example, for numbers the comparator is <, the bottom value is -Inf and the top value is +Inf.
|
I think in future a larger refactor may be good to support a wider range of types in min/max, but in the sort term this looks like a good route to getting them working for Date. Numbers and dates are by far the most common use for these functions, so lets concentrate on getting that in soon. |
MAST1999
force-pushed
the
feat_add_date_support_to_aggregate_fns
branch
from
4d42ebc to
6bab62a
Compare
|
Hi @MAST1999, I'm finally catching up on this! I hope you don't mind that I have made some tweaks and pushed to your branch. You had identified much of what needed to happen, and while looking through this it's become apparent we actually have a small bug with range lookups on Date properties. @kevin-dp could you take a look at this PR, I think its all good now but I want you to cast your eyes over the normalisation of keys for the btree index. Even though we had a comparison function that supported dates they would still be inserted using the Date object ref, and so range queries would not work. This PR changes it to normalise dates to a number as the key in the index so that the same date is inserted under the same key. As you did this initial work I want to check that we are making the correct changes. May be worth discussing on Monday. @MAST1999 I removed the normalisation of other objects to JSON - this is an edge case that I don't think we need to support, and stringifying to json is very slow. It would be better to just go on object ref for them (essentually random order). |
3 participants
I tried not to change too many things.
If something doesn't look right, or we need some tests that are missing, I'm happy to try and make those changes.
BTW, I tried adding date support to
eqas well but, I couldn't figure out a way to make it work. I made a change inpackages/db/src/query/compiler/evaluators.tsand, after checking it, I can see that it's converting date to number and comparing it correctly.After I added a test to confirm it, the test kept failing :(
So if it's something that needs more work I can take it out and follow up later.
Feel free to make any necessary changes :)