WIP adding a list aggregate for puting all of the fields or table into an array with groupBy #433
Conversation
|
|
Hey, I'm in dire need of this operator 😄 Do you need any help or testing? I have sometime this weekend. |
| @@ -344,7 +344,7 @@ function getAggregateFunction(aggExpr: Aggregate) { | |||
| const valueExtractor = ([, namespacedRow]: [string, NamespacedRow]) => { | |||
| const value = compiledExpr(namespacedRow) | |||
| // Ensure we return a number for numeric aggregate functions | |||
| return typeof value === `number` ? value : value != null ? Number(value) : 0 | |||
There was a problem hiding this comment.
I'm not sure if this is a good change, since the other function rely on the value being a number.
Might be a good idea to add another value extractor specific to your need. At least that's what I've done in my PR for min and max to support dates.
There was a problem hiding this comment.
Yes that a workaround for now I didn't have the time to do it and I needed it to work fast for my project
There was a problem hiding this comment.
I agree this should be a separate extractor
There was a problem hiding this comment.
Hey @Uziniii
Thanks for picking this up - I'm supportive of adding this, although it needs a little more work first.
This will work well for small groups, but will not be ideal for large hierarchical aggregations, for that we have a plan to make them fully incidental all each level.
The key thing is tidying up the value extractor code.
We should also maybe add some type tests ( in group-by.test-d.ts) to ensure that the value extracted in the list results in the current type on the resulting collection, let me know when you are further along and I will take another look.
| @@ -344,7 +344,7 @@ function getAggregateFunction(aggExpr: Aggregate) { | |||
| const valueExtractor = ([, namespacedRow]: [string, NamespacedRow]) => { | |||
| const value = compiledExpr(namespacedRow) | |||
| // Ensure we return a number for numeric aggregate functions | |||
| return typeof value === `number` ? value : value != null ? Number(value) : 0 | |||
There was a problem hiding this comment.
I agree this should be a separate extractor
#422
not all tests are correct and theres change to be done with AggregateFunction type cause it doesnt return initial type value
the pull request is only for review i dont know how to implement the type in the AggregateFunction type and for knowing if a features like this can and would be added to tanstack db because i really need it and i thinks its necessary