Separate projection from query execution #1120

Closed
timowest opened this Issue Jan 15, 2015 · 19 comments

Projects

None yet

5 participants

@timowest
Member

e.g.

query.from(customer).where(customer.firstName.eq("Tom")).list(customer)

into

query.select(customer).from(customer).where(customer.firstName.eq("Tom")).list()

This has the following benefits

  • queries can be written both with select definition in the beginning as well as the end
  • subqueries can be defined via normal Query instances
  • Projectable and SimpleProjectable can be merged into the same class

Disadvantages

  • more verbose syntax in some cases
  • generic parameter in Query interface/class
@timowest timowest added this to the 4.0.0 milestone Jan 15, 2015
This was referenced Jan 15, 2015
@timowest timowest added the progress label Feb 22, 2015
@timowest timowest self-assigned this Feb 23, 2015
@BuBuaBu
BuBuaBu commented Feb 23, 2015

I like this new syntax, this is more "fluent"

Did you also plan to add a method like com.querydsl.sql.ProjectableSQLQuery.getSQL() ?
Otherwise, we will not be able to call either list() or getSQL() for the same query.

@timowest
Member

getSQL() will be available also in the new version.

@cowwoc
Contributor
cowwoc commented Apr 6, 2015

I currently waste a lot of time trying to figure out how to convert arbitrary SQL statements into QueryDSL syntax (and I've been using QueryDSL for a while). I like this change because it eases this pain.

I hope you continue moving in this direction. Non-standard expressions are also a pain point (e.g. "SELECT " or "SELECT function(x)" etc... The fact that we have to reach over to classes like com.mysema.query.support.Expressions makes the API harder to learn because it's not clear what are options are at any given point when building the query. Ideally I should be able to figure out what my options are without navigating away from the IDE's code-complete :)

@timowest
Member
timowest commented Apr 8, 2015

I suggest that rename a few methods from Projectable to have a clear separation between query construction and fetching

  • list -> fetch
  • iterate -> fetchIterate ?!?
  • firstResult -> fetchFirst / fetchOne / fetchAny
  • uniqueResult -> fetchUnique / fetchOne
  • listResults -> fetchResults
  • count -> fetchCount

Otherwise there will be clashes for exists, notExists, count which are useful both in expression form and as fetch results. What do you think?

@timowest
Member
timowest commented Apr 8, 2015

@cowwoc Expressions contains mostly factory methods for custom expression creation. In normal situations you won't be using this class much.

@cowwoc
Contributor
cowwoc commented Apr 8, 2015

@timowest I favor introducing an intermediate execute() call and leaving the method names unchanged.

Example: query.select(customer).from(customer).where(customer.firstName.eq("Tom")).execute().list()

where execute() would return some sort of result type, and list() would convert the result into a list.

@cowwoc
Contributor
cowwoc commented Apr 8, 2015

Regarding Expressions I find myself using it 10-25% of the time. As such, I think it is should be easier to discover/use.

@Shredder121
Member

@timowest I favor introducing an intermediate execute() call and leaving the method names unchanged.

Could you elaborate on why you favor this?

where execute() would return some sort of result type, and list() would convert the result into a list.

This has some implications on complexity, since the intermediate 'result' should have to know what state it is in and what results can be expected, (e.g. iteration is something special entirely).

Also, we already have the projection type in the Query, so I think it is best to keep the result processing local to the Query class of the module.

@timowest
Member
timowest commented Apr 9, 2015

@cowwoc execute() as you describe it is not feasible, since the different projection methods have also different result fetching behaviour

  • list = eager loading of all results
  • iterate = lazy iteration
  • firstResult = only first loaded
  • uniqueResult = first two loaded
  • listResults = eager results and total count for paging
  • count = only count

execute() implies that the query is executed, when the method is called.

@timowest
Member
timowest commented Apr 9, 2015

@cowwoc Concerning Expressions usage feel free to sketch a more fluent usage pattern for the functionality it provides.

@cowwoc
Contributor
cowwoc commented Apr 13, 2015

Could you elaborate on why you favor this?

My intent was to:

  • Visually separate query construction from query execution (fetching the results).
  • In the existing API (even the one you are proposing) it isn't obvious at which point the query is actually getting fired. With the API I was thinking users could potentially construct a query once and reuse it multiple times, potentially reading the results differently each time (once as a list, once as an iterator, etc).

These points come about from my impression that there is a performance gain to be had if we can reuse PreparedStatements (something that QueryDSL does not currently allow). However, if there is no benefit to this (because, say, connection pools already do this) then I withdraw my suggestion.

@timowest The methods you listed seem to violate the "query construction" vs "query execution" separation of concerns. For example, it sounds as if count() alters the query but the entire point of this issue is to move the query construction components to the beginning of the line.

The methods you listed seem to blur the line a bit between query construction vs execution vs enforcing assertions on the response. I would expect users to select(Expressions.count(customer)).from(customer).where(...).fetchFirst(Integer.class).

How about this syntax (v2)?

select(...columns...).from(...tables...).where(...conditions...).X() where X() is one of:

  • fetchFirst().as(C) fetches up to one row and casts it to an instance of type C
  • hasMaxRows(1).fetchFirst().as(C) fetches up to two rows, ensures that there is only one, and casts the result to an instance of type C
  • fetchList().of(C) fetches all rows and cast them to a List<C>
  • iterate().as(C) lazy iteration over the rows as a Iterator<C>

Notice that I moved the assertions in front of the fetching operation, because hasMaxRows(1).fetchList().of(C) is legal too (I'd expect this to return a list of zero or one elements).

and this opens up the door to future assertions.

For count() we'd do something along the lines of:

select(Expressions.count(customer)).from(customer).where(...).fetchFirst().as(Integer.class) ... I like how this reads a lot closer to raw SQL/JDBC code (lower learning curve).

As for making Expressions more discoverable, I guess we could start off by using them more frequently in the documentation (show at least 2-3 different examples showcasing things like invoking count(), user functions, advanced expressions, etc)

@timowest
Member

You are right that the count options are redundant for the querydsl modules that allow to define a projection.

The queries will be typed, so the as invocations are not necessary. The type parameter of the query is the element type of the result.

@timowest
Member

here are the Projectable/Fetchable method names for review, the first option is the current choice and the second an alternative

  • list -> fetch or fetchAll
  • iterate -> fetchIterate or iterate
  • firstResult -> fetchFirst or fetch
  • uniqueResult -> fetchOne
  • listResults -> fetchResults
  • count -> fetchCount

at least fetchIterate feels weird, so maybe iterate is better? what about the others?

@cowwoc
Contributor
cowwoc commented Apr 16, 2015

I like the second option for all methods.

@cowwoc
Contributor
cowwoc commented Apr 16, 2015

Correction. I like fetchAll, iterate (second option) but then fetchFirst (first option).

@cowwoc
Contributor
cowwoc commented Apr 16, 2015

For fetchOne, consider fetchOnlyOne instead. The latter implies there must be only one, whereas the former sounds like "give me any one".

On second thought, I don't like either option. I wish we could call this method fetchTheOnlyOne() but the name sounds kind of long.

@timowest
Member

I'd still like to keep the fetch name as the default, since returning all matching results feels like a reasonable default behaviour, so

  • list -> fetch
  • iterate (no change)
  • firstResult -> fetchFirst
  • uniqueResult -> fetchOne
  • listResults -> fetchResults
  • count -> fetchCount
@timowest timowest closed this in #1217 Apr 28, 2015
@timowest timowest removed the progress label May 9, 2015
@fanste
Contributor
fanste commented Aug 24, 2015

Sorry for this late comment on that issue, but got the map()-method removed?

@Shredder121
Member
Shredder121 commented Aug 24, 2015 edited

EDIT: most people wanting a map result probably mean to use the GroupBy API

query()
.from(employee)
.transform(GroupBy.groupBy(employee.firstName).as(employee.lastName))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment