provide LISTAGG implementation #174

maxcountryman · 2020-05-29T13:40:17Z

This patch provides an initial implemenation of LISTAGG[1]. Notably this
implemenation deviates from ANSI SQL by allowing both WITHIN GROUP and
the delimiter to be optional. We do so because Redshift SQL works this
way and this approach is ultimately more flexible.

Fixes #169.

[1] https://modern-sql.com/feature/listagg

nickolay

Thanks! I've posted some suggestions below, that I think will simplify or otherwise improve the code.

src/ast/mod.rs

src/parser.rs

maxcountryman · 2020-05-29T23:38:45Z

@nickolay thank you for the thorough and detailed feedback. It's most appreciated! I've made a number of updates and left a couple of things open just to clarify or get your feedback.

src/parser.rs

src/ast/mod.rs

src/parser.rs

maxcountryman · 2020-05-30T01:59:22Z

@nickolay I think I've addressed your additional comments. Thank you for your patience, I appreciate it: I'm still very much a Rust novice. 🙂

nickolay

Yep, almost there - I'll look again tomorrow, but I think that apart from the few remaining nits, this is good to merge.

I hope I have not put you off with the amount of comments, and thanks for bearing with me!

src/ast/mod.rs

src/parser.rs

nickolay · 2020-05-30T14:12:25Z

src/parser.rs

                    }
-                    None => None,
+                    Some(Token::SingleQuotedString(_)) => Some(Box::new(self.parse_expr()?)),


If you want to only support literal string filler, use parse_literal_string() and make it a filler: Option<String>.

Ah thanks. That seems to be to spec, right?

I doubt this is a good idea though, as I bet someone will go and ask us to at least extend support to N'...' literals and then Postgres will accept expressions here...

Hm, it does seem like we should be a little more concrete here: not every expression is valid in this position.

Also for context, I don't think Postgres supports LISTAGG altogether (or maybe I'm just failing to find it).

not every expression is valid in this position

The same can be said about 'delimiter'. Omitting which is also not "valid", yet you yourself wrote "we choose to make the separator optional as this provides the more general implementation". The same logic can be applied here: "we choose to allow any expression not starting with WITH or WITHOUT as a <filler> as this provides the more general implementation".

I know Postgres doesn't support LISTAGG at the moment. I was making a prediction that someone will deviate from the spec by supporting something other than 'string' literals here. I brought up Postgres, as I have the impression they like to make things more general whenever possible...

I see you decided to keep it an Expr in the AST, but restrict the allowed tokens to certain kinds of literals ('', N'', X''). OK, let's do it this way.

You're right that the same logic could be applied here, re: the more general implementation. What occurred to me however is this question: What possible values actually make sense here? In other words, what non-string filler would work in practice? I couldn't really think of any, but I very well may be overlooking the obvious. 🙂

I'm more than happy to do what you think is best, so if you decide you'd prefer another implementation then I will swap that in for what we have here.

Your argument makes perfect sense, but in practice "LISTAGG(ALL" shouldn't exist, yet in SQL it does ;p

'.'+'.'+'.' is a string filler just not a literal string.

But as I said, the way you did it is fine by me. If/when someone comes around wanting to lift this restriction, it would be a trivial change, rather than if you followed my original idea of changing Expr to String in the AST.

maxcountryman · 2020-05-30T14:42:43Z

src/parser.rs

+                    Some(Token::SingleQuotedString(_))
+                    | Some(Token::NationalStringLiteral(_))
+                    | Some(Token::HexStringLiteral(_)) => Some(Box::new(self.parse_expr()?)),


This allows for any supported string literal to be parsed as a filler and uses parse_expr to handle this ergonomically. (There may be a more precise way of handling this, but I wasn't immediately able to find it.)

Also it might be a good idea to be stricter with the separator parsing, but I'll defer to your feedback.

nickolay · 2020-05-30T14:56:08Z

The Travis failure is not a problem with your code. Please let me know what you think about https://github.com/andygrove/sqlparser-rs/pull/174#discussion_r432849585 while I'm trying to figure out what's wrong with it.

nickolay · 2020-05-30T15:05:23Z

Ah, travis runs on the results of a merge between the PR and the upstream master. Would you like to do the merge and add the two nulls_first in the parse_listagg testcase? Or I can do it when merging.

maxcountryman · 2020-05-30T15:06:16Z

Ah, travis runs on the results of a merge between the PR and the upstream master. Would you like to do the merge and add the two nulls_first in the parse_listagg testcase? Or I can do it when merging.

I'll try to rebase once you feel confident the implementation is acceptable.

This patch provides an initial implemenation of LISTAGG[1]. Notably this implemenation deviates from ANSI SQL by allowing both WITHIN GROUP and the delimiter to be optional. We do so because Redshift SQL works this way and this approach is ultimately more flexible. Fixes apache#169. [1] https://modern-sql.com/feature/listagg

This consolidates the act of checking for both `ALL` and `DISTINCT` into a parser function. Please note that this present implementation reduces the information provided by the resulting error message. This may be undesirable, so pending review, may certainly be updated or reverted. However it may also be the case that the additional contextual information is not necessary.

maxcountryman · 2020-05-30T15:11:39Z

All right, it seems like the rebase was clean. I'll force push the rebased branch when you give the go ahead. 🙂

nickolay · 2020-05-30T15:19:29Z

Feel free to either rebase and force-push or commit a merge with upstream master - I recently learned that Github's "Squash and merge" is pretty useful.

maxcountryman · 2020-05-30T15:27:03Z

Feel free to either rebase and force-push or commit a merge with upstream master - I recently learned that Github's "Squash and merge" is pretty useful.

I love Squash and Merge. 🙂

All right, rebased off master, force pushed to my branch. Hopefully should be passing on Travis shortly.

nickolay · 2020-05-30T15:51:27Z

Sweet.

Thank you!

maxcountryman force-pushed the feature/listagg branch 4 times, most recently from 49784d9 to 4726c90 Compare May 29, 2020 14:18

nickolay suggested changes May 29, 2020

View reviewed changes

maxcountryman commented May 29, 2020

View reviewed changes

src/parser.rs Show resolved Hide resolved

maxcountryman commented May 29, 2020

View reviewed changes

src/parser.rs Show resolved Hide resolved