ActiveRecord: add support to get result of findAll() and relations as arrays #531

Closed
cebe opened this Issue Mar 20, 2012 · 27 comments

Comments

Projects
None yet
8 participants
Owner

cebe commented Mar 20, 2012

The possiblity to get related records as array or do queryAll() instead of findAll will improve performance on many places where you do not need the power of activerecord and need to handle great amount of records.
This becomes handy for example in ajax requests where you only want to return a set of record's attributes but do not want to create a new layer alongside AR. Simply want to do a Post::model()->commented()->queryAll().
Also when handling great amounts of data for example you have a group and want to get all 5000 related users to check if an attribute matches some specific criteria it would be overkill to instantiate 5000 activerecords including metadata and other objects to only get the one attribute you care about.
An API could be

queryAll()
queryAllByPk()
queryAllByAttributes()

and for relations

getRelatedAsArray() // or something like that, need to find a good name for that.

@ghost ghost assigned qiangxue Mar 20, 2012

cebe, we needed a queryAll() method so I hacked this together. It's a simple tweak of the findAll() method that does not turn the results into object, but instead returns the raw results.

<?php

class BaseModel extends CActiveRecord {

    ....

    /**
     * A mixture of `findAll` and `query`, where the results are not loaded into a class,
     * but instead we get the raw values selected by from mysql as a key->value array.
     * 
     * @example
     * <code>
     *     // Create criteria
     *     $critera = new CDbCriteria();
     *     
     *     // add stuff to criteria...
     *     
     *     // Merge the criteria.
     *     $model->getDbCriteria()->mergeWith($criteria);
     *     
     *     // Query for the results.
     *     $results = $model->queryAll();
     * </code>
     * 
     * @return array An array of arrays loaded from MySQL.
     */
    public function queryAll($condition='',$params=array()) {
        $criteria=$this->getCommandBuilder()->createCriteria($condition, $params);
        $this->applyScopes($criteria);
        $command=$this->getCommandBuilder()->createFindCommand($this->getTableSchema(),$criteria);
        $results = $command->queryAll();
        return $results;
    }
Owner

cebe commented Mar 22, 2012

thanks @somethingkindawierd .

btw #545 addresses some of the use cases of this improvement, but not the main case.

Contributor

luislobo commented Apr 30, 2012

I second this one. Several times I find myself converting array models to arrays. May be a new parameter to current methods or an option, or new methods.

mdomba added a commit to mdomba/yii that referenced this issue May 10, 2012

Member

mdomba commented May 10, 2012

@qiangxue @samdark

As AR is very memory consuming but at the same time really helpful... this option would be really handy...

I created a prof of concept for my idea.

NOTE: The above commit works only if there are no relations included!

All following examples returns normal arrays:

$criteria = new CDbCriteria;
$criteria->compare('id', $this->id);
$criteria->compare('name', $this->name, true);
$criteria->compare('category_id', $this->ac1);
$criteria->asArray = true;
print_r( Company::model()->findAll($criteria) );
...
$criteria = new CDbCriteria;
$criteria->compare('id', $this->id);
$criteria->compare('name', $this->name, true);
$criteria->compare('category_id', $this->ac1);
print_r( Company::model()->asArray()->findAll($criteria) );
...
print_r( Category::model()->findAll(array('order' => 'id ASC','asArray'=>true) );
...
print_r( Category::model()->asArray()->findAll(array('order' => 'id ASC') );
Owner

samdark commented May 10, 2012

@mdomba I think that's a good feature but not sure if this belongs to criteria. Probably we can have a separate flag that will be turned on/off via Category::model()->asArray(true)/Category::model()->asArray(false).

Member

mdomba commented May 10, 2012

Thought of that, too.

I decided for the criteria as there is even the "index" property that is not strictly related to a criteria

Having a separate flag could mean that a developer can forget to turn it off if he uses an model instance and in that case could produce unexpected errors on next findXX()

Owner

qiangxue commented May 10, 2012

What about relational query? If we support asArray(), it's very natural for people to request for it in relation query, which is much harder to implement.

Member

mdomba commented May 10, 2012

Yes I agree, it would be great to have that option too but I see it too hard for the relational query as it's too tied to AR...

that's why I wrote it's just a concept ;)

Owner

cebe commented Jun 1, 2012

I'd like to work on this issue over the next two-three weeks, please post your ideas if there are any so that I am able to see situations and use cases from a wider point of view. Thanks!

Owner

cebe commented Jun 7, 2012

btw: agree with @samdark that this does not belong to CDbCriteria since you can use criteria to build normal querys in other contexts then AR where asArray does not have any meaning.

cebe added a commit to cebe/yii that referenced this issue Jun 11, 2012

issue #531 added asArray property to CActiveFinder
added asArray property to activeFinder to support returning query
results as arrays instead of records.
To fully support this, the API of CActiveRecord needs to be adjusted to
make use of this feature, also some places marked with @todo are not implemented yet.

This commit is ment to be a request for comments.

cebe added a commit to cebe/yii that referenced this issue Jun 12, 2012

removed todo annotations
code already did what it should

related to #531

cebe added a commit to cebe/yii that referenced this issue Jun 12, 2012

added missing implementation of getPrimaryKey
needed a getPrimaryKey() for array of attributes.

related to #531

cebe added a commit to cebe/yii that referenced this issue Jun 12, 2012

Owner

cebe commented Jun 12, 2012

Just finished implementation of asArray for CActiveFinder. Need to find a suiteable API for CActiveRecord now to allow getting results of find*() methods and also relations as array.
Here is a diff about the changes to CActiveFinder:
cebe/yii@master...531-activerecord-as-array

cebe added a commit to cebe/yii that referenced this issue Jun 12, 2012

issue #531: implemented asArray for CActiveRecord
Allows returning arrays as result of find methods.
details discussed in issue #531
Owner

cebe commented Jun 12, 2012

implementation is complete, opened a pull request, looking forward for comments :-)

Owner

cebe commented Jun 22, 2012

updated my pull request #819, I'd like to invite all of you who are interested to review my code to ensure best quality.
@somethingkindawierd @luislobo @mdomba @samdark @qiangxue

Member

klimov-paul commented Jul 6, 2012

If you are so worry about the memory usage and performance of the database queries, why are you using active record at all?
If you will use DAO layer: “CDbCommand” and “CDbCommandBuilder”, the resources saving will be tremendous comparing the “Active Record” usage.
As I understand in this issue, you wish to use the “CActiveRecord” and “CActiveFinder” query composing logic to perform queries without instantiating a new “CActiveRecord” objects, which of course saving the program resources. But it seems for me, you try to do it in too complex way.
I think the best solution will be granting the “CActiveRecord” ability to optionally return the built SQL query source code instead of running it.
So we can use something like following:


$finder = CActiveRecord::model(‘Item’);
$sql = $finder->with(‘category’)->active()->buildSql();
$itemsAsArray = Yii::app()->getDb()->createCommand($sql)->queryAll();

Such solution allows you to use “CActiveRecord” and “CActiveFinder” query capabilities to fetch the SQL query data in array.
For me this looks more simple.

Owner

cebe commented Jul 6, 2012

If you are so worry about the memory usage and performance of the database queries, why are you using active record at all?

It is not about using asArray in all of the applications code to be performance optimized. In most cases I need my AR object to handle things. I love AR and want to use it in any place of my application. But there are places where I only need the fetched data, where I am also unable to re-use my instantiated objects.
I do not want to create crappy workarounds to keep things fast in places like the following example:
Imagine you have 5 or more dropdownfields using code like this:

CHtml::listData(Post::model()->hasComments()->isNotNew()->findAll(), 'id', 'title')

would be just this and much faster:

CHtml::listData(Post::model()->hasComments()->isNotNew()->asArray()->findAll(), 'id', 'title')

I can do that in my view files, it's not too complicated, but I would not create a db query command there and fetch the data.

If my whole application is built up with AR I do not want to go there and change all the places back to DAO that are too slow while optimizing. There are only a few places where asArray is necessary, but you do not want to duplicate all your logic there.

Btw: your example would not be possible to implement since CActiveFinder can do multiple queries to fetch related data. It would be limited to queries that do not use with and that is the simplest part of implementing asArray as this part can be done with only a few changes to CActiveRecord::query() method. The code you can see in the diff is mostly all you have to do to implement asArray for non relational queries.

Member

klimov-paul commented Jul 7, 2012

@cebe, I see your point and I understand your use case.
Although it seems something is not well here.
You propose to create a method “CActiveRecord::asArray()”, which calling changes the result types of other methods of “CActiveRecord”. So normally “CActiveRecord::find()” return a “CActiveRecord” instance, but if “asArray()” is invoked before it will return “CActiveRecordArray” instance or even simply array.
For me this makes too much complexity and confusion. You allowed everything mess up!
Do not forget that “asArray()” will be considered in CDbCriteria as “scope”. So criteria configuration may looks like:


$criteria = array(
    ‘scopes’=>array(‘asArray’)
);

I have a lot of experience of creating the components (CComponent, CApplicationComponent descendants), which wraps there logic around the active record models. For example: the component, which handles the site language switching, finds the available languages in database through the active record model “Language”. For such components I usually make a field “searchCriteria”, which allows adjusting active record find criteria “from outside”, making the component more flexible.
So example of component configuration is following:


‘components’=>array(
    …
    ‘lang’=>array(
        ‘class’=>‘MyLanguageManager’,
        ‘modelClassName’=>’Language’,
        ‘modelSearchCriteria’=>array(
                ‘scopes’=>array(‘activeOnly’)
        ),
    ),
    …
),

With your solution the search criteria may also contain the scope “asArray” as well. And what should I do inside my component now? I can not rely the result of “CActiveRecord::find()” to be a model instance anymore.
Passing the model search criteria is a common practice in Yii, you can see for example “CUniqueValidator” and “CExistValidator”.

You can not use the scope type method to resolve your use case. Array result switch can be applied only by the final method:


$itemsAsArray = CActiveRecord::model(‘Item’)->active()->findAllAsArray();

This I can agree.

Owner

cebe commented Jul 9, 2012

First some clarification:

So normally “CActiveRecord::find()” return a “CActiveRecord” instance, but if “asArray()” is invoked before it will return “CActiveRecordArray” instance or even simply array.

CActiveRecordArray is only used internally by CActiveFinder. asArray() will always make the result be a simple array which is equal to CActiveRecord::attributes plus relations added by with in the criteria. No CActiveRecordArray object will ever be seen outside of CActiveFinder.

You propose to create a method “CActiveRecord::asArray()”, which calling changes the result types of other methods of “CActiveRecord”.

This method is supposed to be used in a chain like this so you realy know what will be the result:
$posts = Post::model()->asArray()->findAll() - will give you an array of arrays
$posts = Post::model()->findAll() - will give you an array of records
also every find*() will reset the state of asArray back to false so if you call the two lines above directly after another the second will be array of records again.

Do not forget that “asArray()” will be considered in CDbCriteria as “scope”. So criteria configuration may looks like:

Okay, see your point, it is possible to add asArray to criteria scope but that is also possible with all other AR methods that are not considered to be used as a scope. We could add a note to the docblock that it is not supposed to be used as a scope though.

As I can see from a quick look into the code adding asArray as scope to criteria will have no effect on the find*() result at least for CActiveFinder. Will check that in detail.

It is possible to set $criteria->scopes = array('asArray') with and without CActiveFinder.
And btw: setting $criteria->index is also a modification of find*()-methods output, not that hard, but it is.

$itemsAsArray = CActiveRecord::model(‘Item’)->active()->findAllAsArray();

Agree that this could be an alternative solution but what about relations? We would need an alternative syntax for calls like this:
$posts = $author->asArray()->posts

Member

klimov-paul commented Jul 10, 2012

For me this issue still breaks the layers: active record supposed to work with active record, while simple arrays should be handled by DAO layer or some another layer. It would be best, if there is some entity, which performs all active record find logic in terms of simple arrays, while “CActiveRecord” just wraps around it, but I can not explicitly determine it.

Anyway, while so many people, including Yii core team, say this is a good enhancement and can be implement in this way, I can not argue.

Contributor

creocoder commented Aug 2, 2012

Forgive me if I do not politically correct, I will say that I think is a useless feature, which is just trying to patch up some of the problems with the architecture. Why do we need half-measures? AR solves part of its problems. DAO for everything else. I am for uncompromising solutions.

Anyway, while so many people, including Yii core team, say this is a good enhancement and can be implement in this way, I can not argue.

What a politically correct man we have here ;)

Owner

cebe commented Aug 3, 2012

@creocoder why should I rewrite complex logic I have already written and tested based on AR just for one or two cases where I need performance? The best example here is CHtml::listData why should I duplicate the same logic I have in AR with scopes and the like also in dao to avoid instantiating models that are not used?
Btw: I like non politically correct discussions, say what you think about it :)

Member

klimov-paul commented Aug 3, 2012

Actually a best solution for “CHtml::listData()” is a creation of specific entity, which should return the content for it.
In the forums and some other sources people usually calls such solution a “Lookup”.
You can see a particular example here (although I dislike this one):
http://code.google.com/p/yii/source/browse/trunk/demos/blog/protected/models/Lookup.php?r=2148

When you have created a separated entity, which handles your “list data”, you can advance it as you like, without breaking anything. Such advancement can include usage of DAO instead of AR.

For this case the best solution is creation of the application component (CApplicationComponent extension), which should have a couple of “lookup style” methods, like “listActiveCategoty”, “listAllCategory” and so on. At the start point you can use AR to satisfy these methods declaration and complete a unit test for your component. At later stage, you can do refactoring and replace AR with DAO, without changing anything in your view files. If you have a unit tests written for the lookup component this will be easy.

Allowing the active record to query arrays sounds like “corner cut” solution. It is of course an easy way, but is it the right one?

Contributor

creocoder commented Aug 3, 2012

@klimov-paul I belive Lookup is antipattern and application component i think not good idea too.

@cebe I do not think using code like this in views is good programming practice:

CHtml::listData(Post::model()->hasComments()->isNotNew()->asArray()->findAll(), 'id', 'title')

I prefer use fat models with methods like:

CHtml::listData(Post::model()->findAllForFilter(), 'id', 'title')

or even

Post::model()->getListDataForFilter()

Once you decide that this part of application have not good perfomance you just can change fat model method implementation without touching any view files. For example you can switch from AR to DAO in this method. But I bet $100 you will not, because its not first place where we need make any optimizations :)

Using this concept is more agile. What if tomorrow we need transformate category list:

  • Action
  • RPG
  • Quest
  • Logic

to

  • Action (30)
  • RPG (45)
  • Quest (20)
  • Logic (11)

What you will do with you code like this in views:

CHtml::listData(Post::model()->hasComments()->isNotNew()->asArray()->findAll(), 'id', 'title')

?

Nothing, even asArray() will not help you ;-) Only the correct application design can help.

Owner

cebe commented Aug 3, 2012

@creocoder agree that there are many styles one can prefer and there are pros and cons on both sides.
When you write your fat model methods they will in most cases look the same: You will fetch data and modify it to a format you need. In my opinion this is a place where a framework could come in and do this work for you so you can get the data you want from the framework by directly telling it how you need it.

About optimization: Have you seen my comparision made here: #819 (comment) ? I used the asArrray approach I've written in that pull request in one of my applications and it really increased performance by 4-5 times. Think this is relevant, so these $100 are mine ;)

Contributor

creocoder commented Aug 3, 2012

@cebe About optimization. This is only little application part optizimation. I was bet on that this part is not main if look at application overall. Did you use any cache in you application before using asArray approach? Are you sure asArray approch + good caching technique will increase perfomance of any application by even 1.1-2 times? I do not think so :)

And other side. Why use asArray() as approach that for example can increase only by 50/60/70% (for example) and not use DAO in this application parts which can increase pefomance to max %?

I personally prefer to use cache, DAO, other fundamental approaches and not prefer using magic spells like "asArray() please boost this app part a little" ;-)

Owner

cebe commented Aug 3, 2012

@creocoder It is also about code maintainance if you have about 50 models(and this is only the core of my application) who are using behaviors and scopes and the like you always have to duplicate code when you want to optimize things with DAO.
Sure it is not as fast as it could be, but it is an abstraction that helps to keep that code clean and handleable.
Also OOP is not as fast as method orientated php, but you are able to handle the amount of code with it.

I am using many behaviors that attach functionallity to the ActiveRecords such as translation, nested tree and so on which would be very complex to optimize switching to dao. Of course we could do that to get maximum performance but we will likely get to a point where this code becomes unmanageable.

Contributor

creocoder commented Aug 3, 2012

@cebe Then CHtml::listData() example is not so good example for discussion. This is not even perfomance bottleneck. Show another examples where asArray() could be useful.

About code dublication. I show example with lists. When today list is standart and tomorrow you can not make this list with standart code. This is standart part of application where remove code dublication is worst thing you can do. Refactoring is not only about dublication elimination. Refactoring it is smart process. I show you another list and hope you will get idea:

  • Jack (moderator)
  • Petr (manager)
  • Masha (administrator)

if you have about 50 models
you always have to duplicate code when you want to optimize things with DAO

Some code dublication at start stage. 0% code dublication at final stage. As i say lists is not about:

  • one
  • two
  • three

In practice it is almost always unique cases.

when you want to optimize things with DAO

Do you remember my bet? I bet you do not want usually optimize things with DAO or with magic spell "asArray()". Only if you have absolutely nothing to do. This is not n1 problem with perfomance. And when time will come and there will be situation when really nothing to do, i make coffie and my choose will be DAO, not magic spell. Because i prefer to solve problems fundamentally. If after optimization i see that there is some code dublication i will decide what to do. But this is another stage - refactoring. Refactoring with knowledge: "Tomorrow this list can be not simple".

You think DAO in some model places is ugly? Try look at DAO on the other side. Its like DAOsism :)

P.S. I hope Yii 2.0 AR will be good and without even lite architectural defects.

Contributor

luislobo commented Aug 7, 2012

I think asArray is useful. I certainly do caching, lots of it, and also find myself converting all those fat AR collections to arrays to make my cached data smaller.
I think arguing about using or not using asArray is like arguing if caching should be available for AR models with the cache() function. Certainly it's better to cache the smallest info as possible, but, hey, its an option. And it's good to have it when I needed it.
I think asArray is good enough to be used in places, knowing the pro's and con's.

@samdark samdark closed this Sep 8, 2012

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