-
-
Notifications
You must be signed in to change notification settings - Fork 1
Add SQLLAGExpression
#3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3 +/- ##
==========================================
+ Coverage 91.68% 91.86% +0.18%
==========================================
Files 61 63 +2
Lines 926 947 +21
==========================================
+ Hits 849 870 +21
Misses 77 77
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment on the serialize method is purely advisory; I would really prefer to add syntax that provides more general support for window functions rather than for just one specific function. Indeed, even this one function's implementation is incomplete without at least adding support support for PARTITION BY
and FILTER
and correct support for ORDER BY
. The correct syntax of lag()
is lag(value[, offset[, default]])
, per https://www.postgresql.org/docs/17/functions-window.html, https://dev.mysql.com/doc/refman/9.0/en/window-function-descriptions.html#function_lag, and https://sqlite.org/windowfunctions.html#built_in_window_functions - where you got this OVER (ORDER BY 1) DEFAULT 0
syntax I can't even guess.
serializer.write("LAG(") | ||
value.serialize(to: &serializer) | ||
serializer.write(") OVER (ORDER BY ") | ||
offset.serialize(to: &serializer) | ||
serializer.write(")") | ||
if let defaultValue = defaultValue { | ||
serializer.write(" DEFAULT ") | ||
defaultValue.serialize(to: &serializer) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's always preferable to leverage existing expressions when possible in generating output for new ones - and to use serializer.statement {}
when the syntax allows. For example:
serializer.write("LAG(") | |
value.serialize(to: &serializer) | |
serializer.write(") OVER (ORDER BY ") | |
offset.serialize(to: &serializer) | |
serializer.write(")") | |
if let defaultValue = defaultValue { | |
serializer.write(" DEFAULT ") | |
defaultValue.serialize(to: &serializer) | |
} | |
serializer.statement { | |
$0.append(SQLFunction("lag", args: value)) | |
$0.append("OVER") | |
$0.append(SQLGroupExpression(SQLList([SQLRaw("ORDER BY"), offset], separator: SQLRaw(" "))) | |
if let defaultValue = self.defaultValue { | |
$0.append(SQLLiteral.default, defaultValue) | |
} | |
} |
(keeping in mind that this is not even correct syntax for LAG
to begin with - see the overall review comment for more details)
@gwynne do you want to add proper window function support? I think that should be doable. As for the LAG syntax, what I added is what I've successfully been using in a production system for some time now - I got it from an example somewhere (I think it was https://www.geeksforgeeks.org/postgresql-lag-function/ but I'm not 100% sure) |
I definitely want to add proper support, if we're gonna add anything at all. Also, that link also shows correct syntax. I think what you have mostly works by accident 😅 |
This adds support for the LAG window function which allows support for accessing a value from a previous row