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

Getting rid of self string concatinated SQL statements #12502

Closed
DeepDiver1975 opened this issue Nov 28, 2014 · 10 comments
Closed

Getting rid of self string concatinated SQL statements #12502

DeepDiver1975 opened this issue Nov 28, 2014 · 10 comments

Comments

@DeepDiver1975
Copy link
Member

DeepDiver1975 commented Nov 28, 2014

we better use query builder or move into the orm segment

@karlitschek
Copy link
Contributor

Only after serious performance tests that show that we are not getting slower because of that.

@PVince81
Copy link
Contributor

We'll actually get faster and better with this, considering the following:

Currently querying the file cache needs to go to the Cache class which does a "SELECT" on oc_filecache.
Now, how can an external app like "metadata/tags/favorite" extend that select to include additional fields like tags and join on the tags table ? How can we even add filters in the "WHERE" clause ?

We can't.

Currently we're doing the joins and sort on the PHP side which is ugly and inefficient. See an example here: https://github.com/owncloud/metadata/pull/2/files#diff-7d68f805cd62ca74aea358f46e62a20dR47
To be able to return "tags" together with the file information I need to query the tags for every file separately and then inject them into the result using a PHP loop. I'm pretty sure that having a SQL JOIN would be more efficient than that (that's where we can be faster)

Now you could say I could try and extend the Cache class to allow apps to hook into the SQL query and add additional fields and stuff. But do we want to manually concatenate many pieces of SQL strings together ? I'm not sure, and there's a risk to reach this level of ugliness und unreadability like in share.php: https://github.com/owncloud/core/blob/master/lib/private/share/share.php#L1278

If Doctrine provides a way to build SQL queres in a modular way that would also allow apps to elegantly plugin their own pieces of the statement (fields, where, sort), then I think we should give it a try.

@karlitschek
Copy link
Contributor

Interesting example but also a cornercase. Adding an abstraction layer doesn't make things faster in 99% of all cases. The best we can get is to have only a small performance penalty.
Just look at the performance data of other software who moved to ORM.
You get the optimal performance if you understand how a database works and which tables, rows and indexes exists. Than you can write the queries in the optimal way.
I know that this kind of low level opimisation is uncool today. A lot of people don't even learn SQL anymore.

This said: i'm open to move to ORM in the long term for selected areas. IF we understand the performance impact and we look at the generated SQL.

@PVince81
Copy link
Contributor

I think there is a big difference between the query builder an ORM.

If the query builder is just a helper to create SQL expressions, then it shouldn't have any performance impact (unless it is stupid and adds cruft in the SQL strings, that's what we need to check). In the worst case we can then write our own. Goal is to avoid code that looks like the share.php example I posted above and make it possible to plugin external apps.

I would be against having an ORM though which is a higher level of abstraction.

@DeepDiver1975
Copy link
Member Author

I totally agree that introducing query builder is a much lesser effort compared to introduce orm.

What we have to keep in mind regarding ORM is the fact than (in theory) using ORM will allow us to move to nosql databases which is in terms of scalability the as-of-today approach - just like using object store for blob data.

(We might even want to think about moving our structured data into the object store ????)

@DeepDiver1975
Copy link
Member Author

#13278 shows how querybuilder can be used

@PVince81
Copy link
Contributor

The decision so far was to use the QueryBuilder everywhere.

@DeepDiver1975
Copy link
Member Author

@PVince81 need for change or are we okay with querybuilder?

@PVince81
Copy link
Contributor

Query builder was fine so far, let's keep it. Not sure how many more queries we need to adjust.

@PVince81
Copy link
Contributor

PVince81 commented Nov 2, 2018

We should actually rather use the entity mapper when possible.
If there is no mapper and time is short, usage of the query builder for old code is ok.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants