Skip to content
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

Add support of PostgreSQL-specific VALUES(..) expression #284

Merged
merged 5 commits into from Sep 30, 2021

Conversation

NikitaRazmakhnin
Copy link
Contributor

@NikitaRazmakhnin NikitaRazmakhnin commented Sep 22, 2021

At my company we really wanted to use VALUES Postgres expression to use it inside of JOIN
between large db data-set with some smaller user-input data-set without loading of data into app-memory.
VALUES allows us to do that and even gives performance benefit to our app.

I think it is a relevant issue #231 :)

This PR tries to bring an ability to use like:

SQL : SELECT * FROM (VALUES (a1), (a2) ...) as "vs"("v1")
Haskell: select $ from $ values [val 1, val 2, val 3]

It also works for more than one columns as well (a bit doubting about aliases/refs assembling but works good so far). :)

I implemented it as part of From and thanks to latest refactoring from
GADTs to type-classes it was not so difficult :)

Few appropriate semi-integrational tests with other sql capabilities as join, sinigle/multicolumn were added as well.

Please, let me know if I made something in a wrong way - I am ready to try to fix :)

Thanks!

Before submitting your PR, check that you've:

After submitting your PR:

  • Update the Changelog.md file with a link to your PR.
  • Check that CI passes (or if it fails, for reasons unrelated to your change, like CI timeouts).

Copy link
Collaborator

@parsonsmatt parsonsmatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome, thank you!

Only thing is that I am pretty sure we can put this directly into esqueleto proper instead of Esqueleto.Postgresql. Can you try doing that and see if the tests all work out still?

@@ -363,3 +371,75 @@ filterWhere aggExpr clauseExpr = ERaw noMeta $ \_ info ->
in ( aggBuilder <> " FILTER (WHERE " <> clauseBuilder <> ")"
, aggValues <> clauseValues
)

newtype PgValuesExprs a = PgValuesExprs { unPgValuesExprs :: NE.NonEmpty a }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a Postgres specific feature?

This article seems to indicate that it is a SQL-92 feature, and should be supported by all SQL databases.

MySQL supports VALUES, as does SQLite. So I think we can move this into esqueleto proper.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah sorry: my bad! Thanks for the links.

Sure, I can do that, but looks like other vendors requires different syntax, so I need to address that then :)

I will try to do that.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, if they require different syntax, then let's not bother. The current solution is to keep anything that varies in these separate modules.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In some basic detail:

  • MySql wants keyword ROW(..) for one row of values;
  • SqLite requires some other placement of commas as far as I understand.. but worth to play a bit more.

I think consistency of API for vendors matters, so I am ready to try to cover other vendors as well.
But if you allow to go Postgres API earlier than other vendors - it would be awesome!

Otherwise - I could add necessary extensions for other vendors right here and ping you when it's ready :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

esqueleto doesn't currently do anything to differentiate the database provider - it's all Sqlbackend as far as we're concerned. So I'd rather not try and do anything clever here and just support PG directly. If someone else wants MySQL support or Sqlite, then we can add those in vendor-specific modules, and if we really want to unify it, we can try and do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, understood: I also meant similar extensions in vendor-specific modules (like here) without unification (which probably also will present some code duplication but not a big deal I think).

Then I guess from Postgres perspective everything looks ok now? :)

-- pure (bound, count @Int $ user^.UserName)
-- @
--
-- @since 3.5.2.3
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙌🏻 this is dope

@@ -1270,6 +1271,53 @@ testLateralQuery = do
let _ = res :: [(Entity Lord, Value (Maybe Int))]
asserting noExceptions

testValuesExpression :: SpecDb
testValuesExpression = do
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blessed tests ❤️

@parsonsmatt
Copy link
Collaborator

Try merging master once #285 is pulled in

Comment on lines 377 to 378
instance (ToSomeValues a, Ex.ToAliasReference a, Ex.ToAlias a) => Ex.ToFrom (PgValuesExprs a) a where
toFrom = fromValues . unPgValuesExprs
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why you are going through ToFrom instead of just just making values === fromValues?

ToFrom is mostly there to support SqlSetOperation as a restricted subset and backwards compatibility with the *Join datatypes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, no reasons, this is just a lack of my knowledge of code-base and how it works in day when I wrote that :)
Thanks a lot for note, this is more clear for me now!

I have fixed that :)

-- select $ do
-- bound :& user <- from $
-- values ( (val (10 :: Int), val ("ten" :: Text))
-- :| [ (val 20, val "twenty")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish the OverloadedList extension wasn't such a foot cannon with NonEmpty and actually just gave a compilation error.

Nothing to change here, just me 😞 in haskell

of `ToFrom` typeclass for postgres `values` expression.
Copy link
Collaborator

@parsonsmatt parsonsmatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding functionality is a minor bump - otherwise this is great and looks good to go 😄

I'll make the suggested modifications myself when I release for 3.5.3.0.

@@ -1,3 +1,9 @@
3.5.2.3
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a new feature is a minor bump, so

Suggested change
3.5.2.3
3.5.3.0

@@ -1,7 +1,7 @@
cabal-version: 1.12

name: esqueleto
version: 3.5.2.2
version: 3.5.2.3
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
version: 3.5.2.3
version: 3.5.3.0

@parsonsmatt parsonsmatt merged commit 2a44844 into bitemyapp:master Sep 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants