Use CallableResult for solving N+1 problem

[danut007ro]

This pull request aims to add a possibility to postpone rendering of a cell until the end. Using this, one can have a DataLoader that batches all calls that are needed to render a cell and make a single call instead of one per cell.

[curry684]

@shades684 seems fine to me, can you foureye this?

[curry684]

@danut007ro can you elaborate on the specific problem you are trying to solve? We are reviewing the change but are not really sure it's the right solution.

[danut007ro]

Hello,

In my specific case I need to load some entities from another database connection based on an id from current entity. I have an entity with a integer field named city_id that is actually from that second database connection. Instead of showing city_id in the datatable, I want to show it's name, so I need to load those entities. With current implementation I need to load entities one by one, but I want to be able to load them all at once with a single request.

Basically I need to modify the $resultSet after it is constructed in https://github.com/omines/datatables-bundle/blob/master/src/DataTable.php#L286

I'm using this feature together with https://github.com/overblog/dataloader-bundle and it seemed the simplest and most extendable way of implementing this feature.

Hope it's clearer now. I'm open to any other suggestions also. Thanks.

[curry684]

Ok we were guessing it would be something like that. The proposed solution seems to be quite complicating for such an edge case.

In the specific case - why not preload a local id-indexed array (likely even from cache) of the cities before rendering the table?

[danut007ro]

You mean preloading all the cities? That is pretty huge in my case. I'm imagining that at some point I might need an API call or something like this.

I can think on other solution also but this seem more intrusive:

• have a dataTransformer in DataTable class, just like current transformer
• let AbstractAdapter::getData() call that transformer after building all the rows with $rows as parameter and pass it's result in ArrayResultSet constructor.

Basically it would be like a row transformer, but it will apply to all the data. How does this sound compared with my current proposal?

[danut007ro]

@curry684 actually I believe this approach is simpler than the current proposal... But I'm waiting for your input before working on it. Thanks

[curry684]

We discussed it at length here and decided not to go forward with this PR, as it complicates and slows matters on behalf of a very exotic edge case. You are effectively breaking the bundle's fundamental determination to work on lazy-loaded rows as you want to do batch post-processing on the intermediate result set.

The most efficient way we see to achieve that is actually to subclass your own ORMAdapter and override only those lines translating the Traversable ORM output to a result set:

https://github.com/omines/datatables-bundle/blob/cc6abafda2d4a811bf91148e1fe7b2d30cc08086/src/Adapter/Doctrine/ORMAdapter.php#L195-L200

You may also need to override ORMAdapter::mapPropertyPath for correct results, but they are both valid extension points.

[danut007ro]

@curry684 so even the transformer approach that I proposed isn't valid? That looked more valid since it works like the current row transformer. I don't really like copy-pasting the getResults() method to a custom adapter and modifying it a little bit.

If this isn't going to be accepted (in either form) then I guess it can be closed. Thank you.

[curry684]

We considered that option, although we would implement it as an event, but concluded it wouldn't help you out enough as you'd need to intervene in the passing of iterators... and their position which would be unpredictable.

You have to keep in mind that much of this bundle was at this point designed 'not to blow up your server'. Hence all the Traversables and Iterators, and the EntityManager::detach calls that enable paging of thousands of entities in a performant way. We simply have no 'hook point' at which we reliably have access to 'the entire result' when using the ORMAdapter, as it was designed not to.

Adding such hook points would invariably end up with us having to keep the entire result set in memory at some point, which would likely break existing implementations as it would increase memory usage significantly.

Which makes the only obvious solution to subclass the ORMAdapter in a supported extension point to override that part of the design philosophy.

Like I said, your case is really exotic ;) The bundle was simply fundamentally designed to always do all its work at the row level. Your use case invariably requires breaking out of that as you seek to optimize for vertically recurring data.

[danut007ro]

Got it, thank you for the very detailed explanation. Will go ahead and extend the ORMAdapter like you proposed and see how it goes, since it's a pretty straight forward solution.