Minimally useful query syntax #132

robacarp · 2018-01-31T18:43:51Z

There's been some chatter about building a query syntax for granite. I've been tinkering with this, added to my Amber project directly. I've included it here as a granite extension for simplicity.

Goals:

I really like how easy it is to get to the Granite raw SQL, and I don't want to hide that.
The basic ORM is working great.
I'm also seeing several of the reasons (besides database interoperability) why builder syntax is handy for efficiency and also for readability.
I think Granite should, where possible and efficient, stay out of macrospace.
My example tries to provide good separation of concerns. Several classes which each address a very specific problem. Comments welcome.
I really would like the ability to pass around partial query objects without mangling strings, so an intermediate class which holds a pending/partial query which can be appended to is necessary.
I don't want to handle caching here. Granite would benefit from that, but it's a different problem, for a different day.
I don't want to handle any relationships here. Eventually it'd be great to be able to build multiple models out of one joined query, but: crawl, walk, run.
Most importantly, this functionality should be expressly opt-in, at least for now. Easy to include into a single model, not monkeypatching the base. At least for now.

Current status

This works:

Model.where(field: "value").count
Model.where(field: "value").first(n) / .first / .any?
Model.where(field: "value").order(field: :desc)
Model.where(field: "value").delete
Model.where(...).raw_sql

Currently there is only a query builder for Postgres, but all query generation logic is contained with Assembler::Postgres, so adding a Mysql and Sqlite compliant version shouldn't be too difficult. 🍌

I'd really like to work together on this. Granite has a great history of working together over time! If the current code is a viable starting point, let's get it in and everyone can contribute.

🥑

marksiemers

A couple inline comments.

My only concern here is the ability to eventually insert JOINs to handle associations along with the query syntax.

Have we looked at Lucky's query builder to see if it could be compatible and if it does what is needed?

marksiemers · 2018-02-02T19:29:02Z

src/query/builder.cr

+    self
+  end
+
+  def _build_order


Are these underscore methods meant to indicate internal use? If so, make them private.

Yes and no. They can't be private because they're called by the Query::Compiled class. I think this is evidence of an architectural mis-step in this design, but I can't quite see what the correction would be yet.

This doesn't really change things, but I like it a little more:

def order set_default_order if @order.none? build_order end private def build_order @order.map { |expression| "#{expression[:field]} #{expression[:direction]}" }.join(", ") end private def set_default_order @order = [{ field: T.primary_name, direction: "ASC" }] end

protected might be what you are looking for

Actually, technically, _build_order is returning an order clause, right? so:

def order - def order_clause : String
private def build_order -> private def build_order_clause : String

@eliasjpr - There isn't any inheritance, an instance of this class is used in class he mentioned.

Some of these classes could potentially be made into module mixins, just going off names:
Query - noun, class
A query you can build, compile, and run - those could be modules that the Query mixes in

marksiemers · 2018-02-02T19:31:26Z

src/query/runner.cr

+  def initialize(@query : Compiled(T))
+  end
+
+  def log(*args)


This will need to hook into a logger eventually, right? So it doesn't log in production.

yep, it's just a stub for now

robacarp · 2018-02-02T19:44:34Z

@marksiemers I haven't done anything thing in here yet with it, but I'm trying to keep joins in mind for sure. Thanks for looking!

eliasjpr

Good start. Some comments.

eliasjpr · 2018-02-02T20:03:36Z

src/query/builder.cr

+    self
+  end
+
+  def order(field : Symbol)


what about using a tuple instead order(updated_at: :asc)?

The other overload does exactly that. This method is meant to be called: .order(:id) and assumes :asc.

eliasjpr · 2018-02-02T20:06:05Z

src/query/builder.cr

+    self
+  end
+
+  def _build_order


protected might be what you are looking for

eliasjpr · 2018-02-02T20:09:16Z

src/query/builder.cr

+#
+# - Model.where(field: value).or( Model.where(field3: value3) )
+# or
+# - Model.where(field: value).or { whehre(field3: value3) }


why no Model.where(field: value).or(field3: value3)

robacarp · 2018-02-13T17:29:43Z

Added postgres where(field: nil) correct handling
Added builder.raw_sql:

> amber exec
User.where(email: nil).raw_sql
  SELECT id, name, email, crypted_password, admin, created_at, updated_at
    FROM users
    WHERE email IS NULL
    ORDER BY id DESC

There are a couple tests now, but they're written with some project specific requires where I'm using this query language in my amber project. Changing the require statements will be required to get the suite to pass.

faustinoaq · 2018-03-17T21:54:33Z

Fixes #158

drujensen · 2018-03-31T18:11:57Z

@robacarp what is the status on this? I really like the work done and think the foundation provides more additional features down the road. Let me know if I can help work on some of it.

robacarp · 2018-04-02T17:07:26Z

@drujensen Thanks for looking. It's status is "pending". I'd like to move the tests into place for the granite repo, so that it can pass specs, and then it can be merged in relatively short order so others can contribute. There are some big components missing, but I think it's the beginnings of a usable foundation.

So long as it continues to be opt-in, I'd be fine having it in master. At some point when it's more stable, it can be included by default. Is that a sane way to open the doors for more collaboration?

drujensen · 2018-04-02T21:03:36Z

@robacarp yeah, we can include this in master as long as it doesn't break the existing stuff and then add to it until its full featured. We can keep it as an undocumented until we feel comfortable its ready for prime time. I think this is a great start!

If we do document it, let's mark it as experimental because more than likely going to change over time as we iterate over it. WDYT?

robacarp · 2018-04-02T21:08:22Z

Agreed

robacarp · 2018-04-09T18:42:01Z

@drujensen @eliasjpr I've moved tests into the right place to run them, so I think this is ready for a merge so others can start collaborating on it.

faustinoaq · 2018-04-09T18:57:31Z

@robacarp I like this 👍

Just one question, does this conflict with #176 in some way?

robacarp · 2018-04-09T19:17:23Z

@faustinoaq good question, no. None of this takes effect in a granite model without explicitly including it, like this:

class MyModel < Granite::ORM::Base
  include Granite::Query::BuilderMethods
end

It does share some of the same functionality, which I hope will merge down the line, but it's independent.

eliasjpr · 2018-04-15T22:49:53Z

src/granite_orm/query/executors/base.cr

+    end
+
+    def log(*stuff)
+      puts


Can we use granite logger?

…ified

faustinoaq · 2018-05-22T16:13:11Z

Hi, Is this documented in Granite's README? 😅

robacarp · 2018-05-23T15:38:57Z

@faustinoaq no, it's not. I'm not sure it's ready for the readme yet. I do want to ping @noahlh that this code exists though, because I think it might be relevant to the custom query work he's doing.

faustinoaq · 2018-05-23T19:09:43Z

@robacarp Oh, ok, I just opened a tracking issue for this 👍 , see: #216

noahlh · 2018-05-23T19:22:59Z

@robacarp thank you for the ping! I have to apologize - I totally missed this PR when I started diving into my query macro (I saw this code but didn't know how WIP it was). Would love to help out / make sure I'm not duplicating any of your work or causing more complexity. I'll dive in to better understand this plugin - it looks super cool and makes a lot of sense. LMK thoughts.

robacarp · 2018-05-23T19:32:15Z

@noahlh no apology necessary. This is pretty minimal, but I think it'd benefit from the way you're thinking about things as well. Happy to hop on a pairing session to see how we can make it all work together

robacarp added kind:enhancement status:wip kind:rfc labels Jan 31, 2018

robacarp requested review from drujensen, a team, eliasjpr, elorest and marksiemers January 31, 2018 18:44

robacarp changed the title ~~First stab at a query syntax~~ Minimally useful query syntax Feb 2, 2018

marksiemers reviewed Feb 2, 2018

View reviewed changes

eliasjpr reviewed Feb 2, 2018

View reviewed changes

faustinoaq added pr:wip pr:needs-review and removed status:wip pr:wip labels Mar 17, 2018

drujensen mentioned this pull request Mar 31, 2018

Support query syntax #158

Closed

faustinoaq approved these changes Apr 9, 2018

View reviewed changes

eliasjpr reviewed Apr 15, 2018

View reviewed changes

eliasjpr approved these changes Apr 15, 2018

View reviewed changes

robacarp and others added 17 commits April 25, 2018 17:18

Update to be more generic

884a883

#first can return nil

c64275d

[GQL] Fix parameter numbering on where clauses

d74ab8e

[GQL] add Builder#any?

09edee0

[GQL] implement Builder#order

55bdb84

[GQL] implement Builder#delete

e3fa5a7

refactor query generation into one place

45a362d

A couple simple tests for the query builder

5a85223

Builds out better Executors, refactors generic naming

efbcfe4

[GQL] PSQL properly render GROUP BY only if aggregate fields are spec…

ca30916

…ified

[GQL] PSQL properly query with Nil values

391cc72

[GQL] fix builder#order

9a830d5

[GQL] provide builder #each

d29ca71

Updates querybuilder proposal for latest changes to granite core

febf648

moving everything into the correct namespace

22c11c4

Use granite logger instead of puts

b4c25a0

Merge branch 'master' into query_syntax_proposal

3eeb92c

drujensen approved these changes Apr 27, 2018

View reviewed changes

drujensen removed kind:rfc pr:needs-review labels Apr 27, 2018

robacarp merged commit ea226b4 into amberframework:master Apr 27, 2018

robacarp deleted the query_syntax_proposal branch April 27, 2018 22:13

faustinoaq mentioned this pull request May 23, 2018

Add new query syntax to Granite's README #216

Closed

c910335 mentioned this pull request Oct 6, 2018

Add limit and offset to Builder/Assembler with some fixes #284

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minimally useful query syntax #132

Minimally useful query syntax #132

robacarp commented Jan 31, 2018 •

edited

Loading

marksiemers left a comment

marksiemers Feb 2, 2018

robacarp Feb 2, 2018

marksiemers Feb 2, 2018 •

edited

Loading

eliasjpr Feb 2, 2018

marksiemers Feb 2, 2018

marksiemers Feb 2, 2018

marksiemers Feb 2, 2018

robacarp Feb 2, 2018

robacarp commented Feb 2, 2018

eliasjpr left a comment

eliasjpr Feb 2, 2018

robacarp Feb 2, 2018

eliasjpr Feb 2, 2018

eliasjpr Feb 2, 2018

robacarp commented Feb 13, 2018

faustinoaq commented Mar 17, 2018

drujensen commented Mar 31, 2018

robacarp commented Apr 2, 2018

drujensen commented Apr 2, 2018

robacarp commented Apr 2, 2018

robacarp commented Apr 9, 2018

faustinoaq commented Apr 9, 2018 •

edited by robacarp

Loading

robacarp commented Apr 9, 2018

eliasjpr Apr 15, 2018

faustinoaq commented May 22, 2018

robacarp commented May 23, 2018

faustinoaq commented May 23, 2018

noahlh commented May 23, 2018

robacarp commented May 23, 2018

Minimally useful query syntax #132

Minimally useful query syntax #132

Conversation

robacarp commented Jan 31, 2018 • edited Loading

Goals:

marksiemers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marksiemers Feb 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robacarp commented Feb 2, 2018

eliasjpr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robacarp commented Feb 13, 2018

faustinoaq commented Mar 17, 2018

drujensen commented Mar 31, 2018

robacarp commented Apr 2, 2018

drujensen commented Apr 2, 2018

robacarp commented Apr 2, 2018

robacarp commented Apr 9, 2018

faustinoaq commented Apr 9, 2018 • edited by robacarp Loading

robacarp commented Apr 9, 2018

Choose a reason for hiding this comment

faustinoaq commented May 22, 2018

robacarp commented May 23, 2018

faustinoaq commented May 23, 2018

noahlh commented May 23, 2018

robacarp commented May 23, 2018

robacarp commented Jan 31, 2018 •

edited

Loading

marksiemers Feb 2, 2018 •

edited

Loading

faustinoaq commented Apr 9, 2018 •

edited by robacarp

Loading