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 a RAW JOIN source to the query #163

Closed
wants to merge 4 commits into from
Closed

Add a RAW JOIN source to the query #163

wants to merge 4 commits into from

Conversation

moiseevigor
Copy link
Contributor

Hi, I've added the raw_join function to the idiorm class. I find it very useful on optimizing the query and selecting the extra data from composite tables. Let me know if you find it appropriate, it actually copy/pasted version of the function _add_join_source without distinction between $join_operator and $table and skiping the _quote_identifier.

@lrlopez
Copy link
Contributor

lrlopez commented Oct 24, 2013

@moiseevigor,
Hi Igor,

I'm trying to understand your proposal. Can you give an example?

@moiseevigor
Copy link
Contributor Author

Hi! Here is the example https://github.com/moiseevigor/onelife-slim/blob/master/app/models/UserCountries.php#L38 The theory comes from correlated subquery I suppose: http://en.wikipedia.org/wiki/Correlated_subquery

@tag
Copy link
Contributor

tag commented Oct 24, 2013

So, why is this better than raw_query()?

@lrlopez
Copy link
Contributor

lrlopez commented Oct 24, 2013

Oh, I understand now... Thanks!

@tag, if you use raw_query() all the other query building methods will be ignored. Anyway, the use case of raw_join() is, perhaps, too specific to cope with Jamie's 80%/20% philosophy

@moiseevigor
Copy link
Contributor Author

@lrlopez Thank you! By the way if you have a list of apps using Idirom the Onelife is heavely depending on ;)
@tag No problem with raw_query(), looks more ellegant to me. And you?

Model::factory('UserCountries')
    ->select("user_countries.*")
    ->select("country_codes.name")
    ->select("country_codes.name_short")
    ->select("user_country_image.*")
    ->inner_join('country_codes', array(
        'country_codes.code2', '=', 'user_countries.country_code2'))
    ->raw_join('
            INNER JOIN (
                    SELECT *
                    FROM user_socialdata_parse
                    WHERE
                        user_socialdata_parse.user_id = :user_id
                    AND is_enable = 1
                    AND user_socialdata_parse.lat IS NOT NULL
                    ORDER BY RAND()
                )', 
            array('user_country_image.country_code2', '=', 'user_countries.country_code2')
            'user_country_image',
            array('user_id', $user_id))
    ->find_many();

@moiseevigor
Copy link
Contributor Author

Just noticed that certainly this pull request lacks the parameter part array('user_id', $user_id) should be done similar as in raw_query().

@treffynnon
Copy link
Collaborator

Unfortunately it is also missing tests and documentation. Do you have the time to update the pull request with the parameter binding, tests and documentation?

@moiseevigor
Copy link
Contributor Author

I'll try to complete it these days.

@treffynnon
Copy link
Collaborator

I like the look of the way this is going. Do you think you'll be able to have something ready for January?

@ghost ghost assigned treffynnon Nov 26, 2013
@moiseevigor
Copy link
Contributor Author

Hi, it is in my weekend roadmap for a month now. I've seen this issue in a 1.5.0 milestone due in 2 months. I'll certainly will come back to you with either result in a week from now ... Thanks!

@moiseevigor
Copy link
Contributor Author

Let's try this one. Let me know if it works for you.

$table .= " {$table_alias}";
}

$this->_values = $parameters;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks too crude, and I think it breaks when multiple raw_joins are presents and maybe in other cases. So maybe we need to make merge but with index shift.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know how you get on with this.

@moiseevigor
Copy link
Contributor Author

@treffynnon Now should work with multiple raw_joins. Added one more test for that.

@treffynnon
Copy link
Collaborator

Merged into develop, thanks. Please could I have some help with testing @tag @lrlopez @moiseevigor

@treffynnon treffynnon closed this Apr 26, 2014
treffynnon added a commit that referenced this pull request Apr 26, 2014
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.

4 participants