Repository navigation
listagg support? #169
Description
Activity
You're right, and PRs are welcome!
According to the spec, the syntax seems to be:
LISTAGG( [ { DISTINCT | ALL } ] <expression>, 'separator' [ ON OVERFLOW { ERROR | TRUNCATE [ 'filler' ] WITH[OUT] COUNT } ] ) WITHIN GROUP (ORDER BY <OrderByExpr> [, ...])You're right, and PRs are welcome!
According to the spec, the syntax seems to be:
LISTAGG( [ { DISTINCT | ALL } ] <expression>, 'separator' [ ON OVERFLOW { ERROR | TRUNCATE [ 'filler' ] WITH[OUT] COUNT } ] ) WITHIN GROUP (ORDER BY <OrderByExpr> [, ...])Should this be an extension to the
Exprenum or its own node type altogether?Well, #40 made the point that the more complex enum variants are more convenient to work with when they wrap a separate type, like
Expr::Functiondoes. In simpler cases likeUnaryOp { op: UnaryOperator, expr: Box<Expr> }we use some inlining.. So it mainly depends on the approach you want to take here.The most straightforward approach would be to add an
Expr::ListAgg, treating it as a unique case, like we do forEXTRACT(), but with a corresponding type since it would have a number of members unlike EXTRACT. That would have some overlap with other aggregate functions, but coming up with a more generic solution would require more research.Well, #40 made the point that the more complex enum variants are more convenient to work with when they wrap a separate type, like
Expr::Functiondoes. In simpler cases likeUnaryOp { op: UnaryOperator, expr: Box<Expr> }we use some inlining.. So it mainly depends on the approach you want to take here.The most straightforward approach would be to add an
Expr::ListAgg, treating it as a unique case, like we do forEXTRACT(), but with a corresponding type since it would have a number of members unlike EXTRACT. That would have some overlap with other aggregate functions, but coming up with a more generic solution would require more research.Makes sense. I started implementing this as an entirely new type a la
TOPbut let me know if you think that doesn’t sound like the right approach.A new type is fine, the question is how it integrates with the existing AST types.
TOP is not a good analogy in this regard, as it's referred to via a
top: Option<Top>member onSelect, meaning there can be zero or one TOP statement in a SELECT.listagg, I believe, can be a part of a string expression, and so must be referred to fromExprsomehow. (Your own example has 2 instances of listagg in a single SELECT.) In my previous comment I took this as a given and was talking about the various ways this can be achieved.By the way, I just noticed your example had this:
listagg(dateid) as dates. The spec saysWITHIN GROUP (ORDER BY ...)is mandatory, which makes sense. Is this a typo? If not, please make sure to mention which dialect supports omitting it and including a test for it.My plan was to tie this new type into
Expr(TOPdoesn’t make much sense as an example, aside from the fact it’s its own type, oops); much like the function implementation you pointed out.That is not a typo, that’s valid Redshift (and I would assume Postgres) SQL.
Sure, if you just meant Top to be an example of a separate type, that's cool.
Thanks for confirming that Redshift decided to make WITHIN GROUP optional, and the delimiter too, it appears:
LISTAGG( [DISTINCT] aggregate_expression [, 'delimiter' ] ) [ WITHIN GROUP (ORDER BY order_list) ]( https://modern-sql.com/feature/listagg doesn't list Postgres a supporting LISTAGG at all yet.)
Interesting—I didn’t realize Postgres doesn’t support it.
It seems like there’s not yet a Redshift dialect in the codebase. I’d be interested in this since that’s our primary use case but happy to address that separately if that makes sense to you.
Yes, if you're willing to discuss a Redshift dialect, let's do it in a separate thread. At this time adding a dialect is not a prerequisite for handling its quirks, as the active dialect currently can affect the tokenizer only, not the parser. Just make a note about the quirks you choose to implement (example).
Reacted by Max Countryman- added a commit that references this issue
on May 30, 2020
It seems the parser doesn't currently support listagg. For instance, the following Redshift example fails to parse: