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 'rawSql'. #34

Merged
merged 5 commits into from
Jan 1, 2012
Merged

Add 'rawSql'. #34

merged 5 commits into from
Jan 1, 2012

Conversation

meteficha
Copy link
Member

No description provided.

snoyberg added a commit that referenced this pull request Jan 1, 2012
@snoyberg snoyberg merged commit a75c9f2 into yesodweb:master Jan 1, 2012
@snoyberg
Copy link
Member

snoyberg commented Jan 1, 2012

And great cheer was heard throughout the land. Thanks Felipe, this will make a lot of people very happy :)

@meteficha
Copy link
Member Author

Very nice! =D

Note that there's one bug that I think that may be relatively easily fixable and we should go for it before releasing persistent 0.7. If one uses "Entity".* syntax, then the DB returns the columns in the order that they are on the DB. This order usually is the same as the entity's order, however there's a huge case when it's not: after migrating if you create a new field in the middle of the entity.

So I see two possible venues that may be explored for a solution:

a) Somehow create placeholders for selectors. So the user may write something like

SELECT ??, ??, myderivedcol, ??
FROM "EntityA", "EntityB", "EntityC", "EntityD"
WHERE this = ?
AND that = ?

This is actually possible to be done for Entitys since we can get from our return type what entities are there, in what order and what's their column definition. However, for Singles the user can't do this.

b) Somehow make SQL backends return the names of the columns in the result as well. Then our code would be able to re-order the column order before calling PersistEntity's code.

So b) is easier for our users in the sense that they don't need to learn about two different placeholder notations. However, a) should be more efficient and requires a lot less changes: namely only on code that was brought by this pull request. Note also that we could use the same placeholder ? for a) as well, I just don't know if it's a good idea since one is replaced by rawSql and the other by the backend, so they really are different beasts.

Unfortunately this bug does happen in practice since I've been bitten by it myself =).

What do you think?

@snoyberg
Copy link
Member

snoyberg commented Jan 1, 2012

I have no real opinions on what's the best approach here. We could in theory be even more invasive, and automatically rewrite "Entity".* to the correct list of fields. That would likely solve the problems, though it starts to get into the area of being too smart for our own good.

@gregwebs
Copy link
Member

gregwebs commented Jan 2, 2012

How about forcing the user to add new columns onto the end of the Entity?

@meteficha
Copy link
Member Author

Greg, because their order also affects the order of the fields on the Haskell side, so that's a PITA.

I'm leaning towards a). The only downside I see (namely having another kind of placeholder -- same symbol or not) does not outweights the benefits. OTOH, b)'s performance downside is a huge one, since I don't know off the top of my head an efficient method of doing this kind of reordering.

Michael, on one hand I like your suggestion because we don't need to introduce unfamiliar placeholders. OTOH, once you get where to put ?? it's more failproof since the order of entities are also taken from the type system. For example, if you don't have any Singles, than something like SELECT ??, ??, ?? FROM ... works even if you swap the order of the entities on your results.

@gregwebs
Copy link
Member

gregwebs commented Jan 2, 2012

I don't like b) as stated either until performance is first bench-marked. We are doing that in persistent-mongoDB out of necessity.

Maybe it is possible (in conjunction with migration code?) to get a mapping of column order at startup time. Rails actually does this to some extent.

@meteficha
Copy link
Member Author

I've implemented a) and sent a pull request (#35). Please take a look there and see what you think =).

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.

3 participants