-
Notifications
You must be signed in to change notification settings - Fork 175
Rework VALUES clause and fix a bug on SQLite #755
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: master
Are you sure you want to change the base?
Conversation
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.
This is great!
(\_ -> Nothing) snd) | ||
where | ||
useAliases = knownBool @(BeamSqlBackendSupportsColumnAliases be) | ||
colAliases | useAliases = Just fieldNames |
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.
Is there a reason to use type-level bools for this? Seems like a typeclass method using either mkFieldNames
or mkDefaultFieldNames
might be more straightforward
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.
Good point.. yeah that would be easier
@@ -479,7 +479,7 @@ instance IsSql92TableNameSyntax TableName where | |||
data TableSource | |||
= TableNamed TableName | |||
| TableFromSubSelect Select | |||
| TableFromValues [ [ Expression ] ] | |||
| TableFromValues Int [ [ Expression ] ] |
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.
Would love to see a little comment here about what this Int
represents
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.
This is the number of columns in the result (in the case that the expressions list is empty, we wouldn't be able to infer that from the values).
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.
I meant as documentation, just like you did in tableFromValues
This fixes an old bug #326 by factoring out all SQLite VALUEs into common table expressions, which seems to be the approach the SQLite manual takes to naming VALUE columns.