-
Notifications
You must be signed in to change notification settings - Fork 77
feat: add support for dates to max and min functions. #428
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat: add support for dates to max and min functions. #428
Conversation
|
I also checked with |
More templates
@tanstack/db
@tanstack/db-ivm
@tanstack/electric-db-collection
@tanstack/query-db-collection
@tanstack/react-db
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some comments with regard to the code changes. I believe the changes here are indeed enough for Date
s 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 Date
s 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
.
minValue = value | ||
} | ||
} | ||
return minValue === Number.POSITIVE_INFINITY ? 0 : minValue | ||
return minValue === Number.POSITIVE_INFINITY ? (0 as V) : minValue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's type let minValue: Number | V
such that we don't need the typecast here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue I have here is that I need the returned type to be the type passed in. So if they pass in a Date, I want the return type to be Date as well. That's why I ended up casting as V
.
I couldn't figure out what's the best way to handle this minValue
and maxValue
situation since if the user returns a number we can return 0 as a valid response, but what should we do if they pass a date? Do we still return a 0, because then the response type would always be number | Date
which is annoying?
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. |
4d42ebc
to
6bab62a
Compare
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
eq
as well but, I couldn't figure out a way to make it work. I made a change inpackages/db/src/query/compiler/evaluators.ts
and, 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 :)