Relax the type of the Unpackspec for makeAggrExplicit #581
Conversation
shane-circuithub
force-pushed
the
relax-type-makeAggrExplicit
branch
from
adceb3b
to
ac24797
Compare
shane-circuithub
force-pushed
the
relax-type-makeAggrExplicit
branch
from
6350f1f
to
049570c
Compare
| A.makeAggrExplicit | ||
| (U.unpackspecField <* lmap (const delimiter) U.unpackspecField) | ||
| HPQ.AggrStringAggr |
There was a problem hiding this comment.
This is just a refactoring, right? I think I actually prefer the other way, since shouldn't we morally be able to have
stringAgg' :: Aggregator (F.Field T.SqlText) F.Field T.SqlText) (F.Field T.SqlText)
stringAgg' = A.makeAggrExplicit def HPQ.AggrStringAggand then
stringAgg :: F.Field T.SqlText
-> Aggregator (F.Field T.SqlText) (F.Field T.SqlText)
stringAgg delimiter = lmap (\a -> (a, delimiter)) stringAdd'I don't think there's anything stopping the delimiter being dynamic (i.e. a value depending on the execution of the query -- although I haven't checked that).
There was a problem hiding this comment.
The change to stringAgg is just a refactoring, yes. And you're right that the delimiter can be dynamic, I just didn't want to break the API of the existing stringAgg.
But I do need to relax the type of the Unpackspec for makeAggrExplicit (which I'm using in Rel8 for ordered set aggregation functions). Maybe Unpackspec is the wrong thing to use here, but basically I'm using it to select the PrimExprs that will be passed as the argument list to the aggregation function. I'm open to other suggestions
There was a problem hiding this comment.
OK, let's merge this to unblock you and we can attend to other considerations later if we want.
No description provided.