Open
Conversation
teunbrand
reviewed
Apr 27, 2026
teunbrand
reviewed
May 6, 2026
Collaborator
teunbrand
left a comment
teunbrand
left a comment
There was a problem hiding this comment.
I'm excited for this! Don't let the nitpicky comments convince you otherwise
thomasp85
commented
May 6, 2026
Co-authored-by: Thomas Lin Pedersen <thomasp85@gmail.com> Co-authored-by: Teun van den Brand <49372158+teunbrand@users.noreply.github.com>
teunbrand
approved these changes
May 7, 2026
Collaborator
teunbrand
left a comment
teunbrand
left a comment
There was a problem hiding this comment.
Thanks Thomas this looks to be in good shape to me now. There are a few things that we could pickup in a different PRs and one thing I'd like to have, but don't need to have.
Maybe for another PR:
- Since we're wiring now
aesthetic_ctxto every stat, we should see if we can use it to throw better error messages from the stats. - It might be interesting to investigate wether we could generalise the pattern of stat chaining you introduced here to arbitrary stat combinations in some way.
Like to have, not a blocker:
- I'd really like the aggregation parameter to follow the paths we've carved out for all the other parameters as well. The less custom logic the better. I understand it has more complex validation requirements, so what I'd propose is the following:
- It gets included in the
default_paramsof the affected layers - The ParamDefinition gets the
string_or_string_arraycontraint, which is purely for type-checking the parameter. - The extra validation can happen in the
setup_layer()method(s). Ideally it is parsed right there but I don't know how easy it would be to transfer the parsed result to the stat. (Thesetup_layer()may also need to be called in the validate.rs path, but that was an oversight of mine).
- It gets included in the
Most of the comments below this are minor. Nice work!
Comment on lines
+719
to
+730
| return Err(GgsqlError::ValidationError(format!( | ||
| "aggregate target '{}' is not mapped on this layer", | ||
| user_aes | ||
| ))); | ||
| } | ||
| for internal in resolved { | ||
| if targets_internal.contains_key(&internal) { | ||
| return Err(GgsqlError::ValidationError(format!( | ||
| "aggregate target '{}' resolves to aesthetic '{}' which is already targeted", | ||
| user_aes, internal | ||
| ))); | ||
| } |
Collaborator
There was a problem hiding this comment.
It'd be nice if we could catch this also in the standalone validate.rs, but I'd understand if that is inconvenient
Comment on lines
+1028
to
+1030
| fn func_literal(s: &str) -> String { | ||
| format!("'{}'", s.replace('\'', "''")) | ||
| } |
Collaborator
There was a problem hiding this comment.
I think this pattern is used in a few more places around the codebase. Might be worth having a quick peek if we could put it higher up in the crate and reuse it. Or inline it at the call site above.
Comment on lines
+885
to
+890
| "WITH {raw_src} AS ({query}), {rn_src} AS (\ | ||
| SELECT *, \ | ||
| ROW_NUMBER() OVER ({partition}ORDER BY (SELECT 1)) AS \"__ggsql_rn__\", \ | ||
| COUNT(*) OVER ({partition_no_order}) AS \"__ggsql_max_rn__\" \ | ||
| FROM {raw_src}\ | ||
| )", |
Collaborator
There was a problem hiding this comment.
I think all tests we have for this FIRST/LAST thing are at the words of the query level. Maybe we should add a little test where this FIRST/LAST thing gets executed see if the resulting data is correct (just to watch for regressions).
Co-authored-by: Teun van den Brand <49372158+teunbrand@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix #160