Reviewing Remodel Branch

[Jeff Moore-4]

Hi Guys,

This is my review of the remodel branch, which I know has long since  
been merged into trunk.  Better late than never.


First, I like the new Solar_Sql_Model_Params_Fetch object and the way  
it makes the fetch parameters into an object but preserves backward  
compatibility.  I also like having Solar_Sql_Model_Params_Eager for  
the same reason.

I do there seems to be significant overlap between  
Solar_Sql_Model_Params_Fetch and Solar_Sql_Select.  It would be nice  
to find a way to merge those two classes in the future in a way that  
largely preserves backward compatibility for both.

One issue that I've noticed in the remodel changes is that the WHERE  
clause is favored for joining versus the ON clause.  This is  
problematic for many cases.  I think both need to be supported at the  
eager options level, while only the ON clause should be supported at  
the relationship definition level.  I'll explain more about this.


So first, in relationship definitions.  The relationship definitions  
have a 'where' option.  you can use that option to restrict the  
records in the foreign table that will be included in the  
relationship.  For example, consider a customer object with the  
following relationships:

        $this->_hasMany('invoices');
        $this->_hasMany('open_invoices', array('class'=>'invoices',  
'where'=>'status <> 'paid'));

in this case the 'where' option is added to the WHERE clause of the  
join.  This works fine in the _hasMany case because separate queries  
are issued.  For _hasOne and _BelongsTo relationships, though this  
introduces a problem.  Placing these conditions in the WHERE clause  
causes the LEFT join to become an INNER join.  That will change the  
results of the native fetch.  This is explained in ticket #208.

The solution to this is to always merge the 'where' conditions on a  
relationship into the 'join_cond' of the eager fetch, such that they  
are always included in the ON portion of the query and never in the  
WHERE.

I'd support changing the name of the option to 'join_cond' to make  
this clear.  Because of ticket #208, there should never be a option on  
the Solar_Sql_Model_Related that specifically targets the WHERE  
clauses of the querying being built.


Note, that in trunk, the results of the getWhereMods function are also  
added to the WHERE clauses.  Before the remodel, they were added to  
the ON clause in an eager fetch.  This also triggers the LEFT JOIN to  
INNER JOIN bug.  See ticket #214.  This could be remedied by the  
changes in the unfounded branch.  There, _modFetch and _modEagerFetch  
can target the WHERE clause and the ON clause respectively when  
necessary.  This fix requires an addition discussed next.


The former eager option 'where' has been renamed to 'join_cond.'  
That's ok.  However, there is logic that directs these conditions to  
either the WHERE clause or ON clause.  If the eager join type is  
'inner', the conditions are added to the ON clause, but if its 'left'  
its added to the WHERE clause.  Especially in the left join case, this  
invites the automatic conversion to INNER bug.

I think the solution to this is that the eager options should include  
both a 'where' option and a 'join_cond' option.  join_cond would  
always be routed to the ON clause, while 'where' would always be  
routed to the WHERE clause.  Combined with renaming 'where' to  
'join_cond' in Solar_Sql_Model_Related AND the _modFetch and  
_modEagerFetch changes, I think this will clear up alot of confusion  
about query building.


Speaking of confusion.  I find the join_only flag to be a bit  
confusing.  Perhaps reversing it would help?  So instead of

'join_only' => true

One would use

'fetch_objects' => false

Thoughts?


There are some performance regressions from remodel.  Prior to  
remodel, there was logic in the query building that could recognize  
when a join was not necessary and remove it. That logic has been  
lost.  See Tickets #210 and #213.  I think that logic should come back.


Back to confusion.  I find the 'join_flag' to be confusing.  The  
description is 'whether or not this eager should be used to control  
which parent records are selected."  May I suggest instead calling  
this 'modifies_native?"


The issue with tickets #210 and #213 are related to the join_flag /  
modifies_native.  For certain kinds of queries, like count() and  
_getNativeBySelect(), extra eager clauses have no impact, unless this  
flag is set.  If its not, they can be dropped.


One last thing.  There are a massive number of options between fetch  
options, eager options, and related options.  They interact in various  
ways.  I think in general combinations of options that don't make  
sense should raise an exception.

In general, I think that the join_flag/modifies_native should only be  
manually set.  Then, any eager options detected that would cause the  
native query to be changed without this flag set should raise an  
exception.

For another example, specifying a 'join_type' of 'inner' on a _hasMany  
will cause the native fetch to return too many records.  The example  
in the bookmarks app uses group by to mitigate that.  If you add a  
toMany eager to a fetch that does not have a group by, perhaps an  
exception should be thrown.   I think there are other cases like this.

Because of the large number of independent options without some kind  
of constraints, building queries will always be kind of mysterious  
because there are so many interactions possible in so many edge  
cases.  I don't see any removable options.  We already tried merging  
some options, which is a solution I don't think paul likes.  So,  
perhaps we should look for more opportunities to validate combinations  
of options to help remove some of the mystery?


That's what I have for now.  There are alot of changes, many of them  
subtle.  Because of the amount of change, I haven't been able to get  
our app running on trunk to track down more subtle issues.

Thoughts?


Best Regards,

Jeff
_______________________________________________
Solar-talk mailing list
Solar-talk@...
http://mailman-mail5.webfaction.com/listinfo/solar-talk

[Paul M Jones-4]

Hi all,

My response to Jeff's review of the "remodel" branch (now folded into  
the "beta" branch) follows.


* * *

 > First, I like the new Solar_Sql_Model_Params_Fetch object and the way
 > it makes the fetch parameters into an object but preserves backward
 > compatibility.  I also like having Solar_Sql_Model_Params_Eager for
 > the same reason.

Glad to hear this ...


 > I do there seems to be significant overlap between
 > Solar_Sql_Model_Params_Fetch and Solar_Sql_Select.  It would be nice
 > to find a way to merge those two classes in the future in a way that
 > largely preserves backward compatibility for both.

... and I noticed that as well.  Don't know if I want to address this  
in the near future, but I agree that it would be nice.


* * *

 > One issue that I've noticed in the remodel changes is that the WHERE
 > clause is favored for joining versus the ON clause.  This is
 > problematic for many cases.  I think both need to be supported at the
 > eager options level, while only the ON clause should be supported at
 > the relationship definition level.
...
 > The solution to this is to always merge the 'where' conditions on a
 > relationship into the 'join_cond' of the eager fetch, such that they
 > are always included in the ON portion of the query and never in the
 > WHERE.
 >
 > I'd support changing the name of the option to 'join_cond' to make
 > this clear.  Because of ticket #208, there should never be a option  
on
 > the Solar_Sql_Model_Related that specifically targets the WHERE
 > clauses of the querying being built.

Good call, and I fully agree.  To sum up:

- The $where property of Solar_Sql_Model_Related is renamed to  
$join_cond

- The relationship definition key for 'where' is also renamed to  
'join_cond'

- The 'join_cond' value in the relationship definition is always added  
to the
   JOIN conditions, not the WHERE conditions

Does that sound right?


* * *

 > The former eager option 'where' has been renamed to 'join_cond.'
 > That's ok.  However, there is logic that directs these conditions to
 > either the WHERE clause or ON clause.  If the eager join type is
 > 'inner', the conditions are added to the ON clause, but if its 'left'
 > its added to the WHERE clause.

This is an accurate assessment, and bears some explaning.  The short  
version is that I wanted to support Jeff's request for "require-no-
related" option.  (The long version follows as an aside.)

{{aside:

     As we know, a LEFT JOIN gets all native records whether or not
     they have related records; an INNER JOIN gets all native
     records that have related records (leaving out the ones that
     have no relateds). However, it's difficult to get only native
     records that have no related records. The only way I could
     think of to do that in a way that maintains some adherence to
     SQL conventions, withough inventing new terminology, was to
     use the following idiom for eagers:

         'join_type' => 'left',
         'join_cond' => 'related.id IS NULL'

     But you can't join on a NULL related.id, because it's not
     there. ;-) Thus, for all LEFT joins, the 'join_cond' is moved
     to the WHERE clause to support this kind of thing. The left
     join occurs and would normally return all natives, but the
     WHERE clause now restricts it to natives that have no related
     record.

}}

I can see how the existing behavior would be regarded as too magical,  
especially for such an uncommon use case.  As such, I am willing to  
remove this behavior entirely. That would mean all 'join_cond' always  
go to the JOIN ON conditions, never the WHERE conditions. Would that  
be helpful?


 > Especially in the left join case, this
 > invites the automatic conversion to INNER bug.
 > I think the solution to this is that the eager options should include
 > both a 'where' option and a 'join_cond' option.  join_cond would
 > always be routed to the ON clause, while 'where' would always be
 > routed to the WHERE clause.  Combined with renaming 'where' to
 > 'join_cond' in Solar_Sql_Model_Related AND the _modFetch and
 > _modEagerFetch changes, I think this will clear up alot of confusion
 > about query building.

Jeff opined months ago that an eager related fetch should always  
return results identical to a lazy related fetch.  I think allowing a  
direct 'where' param on eager fetches runs counter to that idea,  
because you can't specify a 'where' param on a lazy-fetch. (The eager  
'join_cond' param and its brethren modify which native/parent records  
are fetched, not which related records are fetched.)

I think that Jeff's initial idea, that eager-fetches and lazy-fetches  
should always return the same thing, is the right one.  Having a  
'where' param would interfere with that.  I am happy to hear how I  
might be mistaken.


* * *

 > Speaking of confusion.  I find the join_only flag to be a bit
 > confusing.  Perhaps reversing it would help?  So instead of
 >
 > 'join_only' => true
 >
 > One would use
 >
 > 'fetch_objects' => false
 >
 > Thoughts?

I thought 'join_only' made sense (and the length was symmetrical with  
the other join keys, which soothed my OCD ;-).  "Only do a join, don't  
fetch anything" seemed reasonable to translate as 'join_only'.  Anyone  
else have thoughts here?


* * *

 > There are some performance regressions from remodel.  Prior to
 > remodel, there was logic in the query building that could recognize
 > when a join was not necessary and remove it.  That logic has been
 > lost.  See Tickets #210 and #213.  I think that logic should come  
back.

Fair points; I will have to address those separately.


* * *

 > Back to confusion.  I find the 'join_flag' to be confusing.  The
 > description is 'whether or not this eager should be used to control
 > which parent records are selected."  May I suggest instead calling
 > this 'modifies_native?"

I wasn't happy with the name either, but it seemed good-enough at the  
time.  In general, developers should not have to set 'join_flag'  
themselves, except possibly in a very few cases (close to "none").  
It's more for the internal query-building process to keep track of  
which joins are flagged to go in the parent/native select, although  
you can of course set it yourself to force it on your own.


 > The issue with tickets #210 and #213 are related to the join_flag /
 > modifies_native.  For certain kinds of queries, like count() and
 > _getNativeBySelect(), extra eager clauses have no impact, unless this
 > flag is set.  If its not, they can be dropped.

Again, these are fair points, but I will have to address them  
separately.


* * *

 > One last thing.  There are a massive number of options between fetch
 > options, eager options, and related options.  They interact in  
various
 > ways.  I think in general combinations of options that don't make
 > sense should raise an exception.

To do that, we would need to know in advance "what doesn't make sense"  
and check for all of the possibilities.  I think that is a tremendous  
task that presents us with more to lose from than to gain.


 > In general, I think that the join_flag/modifies_native should only be
 > manually set.  Then, any eager options detected that would cause the
 > native query to be changed without this flag set should raise an
 > exception.

On this point I disagree; I like that the query-builder has barely  
enough knowledge to know which joins to put into the top-level parent/
native select without me having to tell it every single time.


 > Because of the large number of independent options without some kind
 > of constraints, building queries will always be kind of mysterious
 > because there are so many interactions possible in so many edge
 > cases.  I don't see any removable options.  We already tried merging
 > some options, which is a solution I don't think paul likes.  So,
 > perhaps we should look for more opportunities to validate  
combinations
 > of options to help remove some of the mystery?

"Mystery" might be more of a documentation issue than anything else,  
to which I plead guilty.




--

Paul M. Jones
http://paul-m-jones.com/




_______________________________________________
Solar-talk mailing list
Solar-talk@...
http://mailman-mail5.webfaction.com/listinfo/solar-talk

[Jon Elofson]

Hello,
Rename of where to join_cond sounds good.

So, just to be sure, you would do this?
$this->_hasMany('invoices');
$this->_hasMany('open_invoices', array('class'=>'invoices',
'join_cond'=>'status <> 'paid'));


join_only seems ok to me, after I read the comments in the code. It's
the description of what it does that helps qualify it.

join_flag. I actually still don't know what that does, even after
reading the comments. I would agree that there is still some mystery
about some of the options and that documentation is key to
demystifying them. I would be happy to help with that provided someone
can take the time to review it. Maybe a bunch of sample scenarios with
1) the SQL statement(s) that are executed, 2) the collection or record
that is returned, and 3) a bit of explanatory text.

Just a thought. Maybe I will put something in the wiki playground as
time permits. It would be beneficial to me as a learning opportunity
as well.

Jon
_______________________________________________
Solar-talk mailing list
Solar-talk@...
http://mailman-mail5.webfaction.com/listinfo/solar-talk

[Jeff Moore-4]

Hi Paul,

Thanks for the response, Paul,

Mondays are for administrivia, I will respond today, i hope.

But basically agree on everything except join_flag.

I need to address the tickets, though.

To keep you up to date, we've decided not to upgrade to a newer  
version of Solar at this time.  We've also cut scope on features that  
would cause us to diverge with where trunk is going.  Hopefully, we  
can move to that at a future date.  I'm not sure when that will be.

In the meantime, I'll work with you to try to bring trunk inline with  
what we need it to be to get there.

That's mostly the model features discussed in this review.

There may be form/grid stuff that I haven't discovered.

I also think there is some validation stuff.  I know I complain about  
it being brittle and that's unfair.  I hope to be able to spend some  
time on this aspect so I can be more specific and offer some solutions.

BTW, I think ultimately the changes made in the remodel will have made  
the model better.  I also see them as being a step toward even better  
future potential.

Thanks, for your willingness to work with us on the Solar release  
schedule.  I'm certain we won't upgrade before the end of the year,  
though.

I'd like to have a followup phone call with you when you have time.

Best Regards,

Jeff

On Oct 10, 2009, at 9:23 AM, Paul M Jones wrote:

> Hi all,
>
> My response to Jeff's review of the "remodel" branch (now folded into
> the "beta" branch) follows.
>
>
> * * *
>
>> First, I like the new Solar_Sql_Model_Params_Fetch object and the way
>> it makes the fetch parameters into an object but preserves backward
>> compatibility.  I also like having Solar_Sql_Model_Params_Eager for
>> the same reason.
>
> Glad to hear this ...
>
>
>> I do there seems to be significant overlap between
>> Solar_Sql_Model_Params_Fetch and Solar_Sql_Select.  It would be nice
>> to find a way to merge those two classes in the future in a way that
>> largely preserves backward compatibility for both.
>
> ... and I noticed that as well.  Don't know if I want to address this
> in the near future, but I agree that it would be nice.
>
>
> * * *
>
>> One issue that I've noticed in the remodel changes is that the WHERE
>> clause is favored for joining versus the ON clause.  This is
>> problematic for many cases.  I think both need to be supported at the
>> eager options level, while only the ON clause should be supported at
>> the relationship definition level.
> ...
>> The solution to this is to always merge the 'where' conditions on a
>> relationship into the 'join_cond' of the eager fetch, such that they
>> are always included in the ON portion of the query and never in the
>> WHERE.
>>
>> I'd support changing the name of the option to 'join_cond' to make
>> this clear.  Because of ticket #208, there should never be a option
> on
>> the Solar_Sql_Model_Related that specifically targets the WHERE
>> clauses of the querying being built.
>
> Good call, and I fully agree.  To sum up:
>
> - The $where property of Solar_Sql_Model_Related is renamed to
> $join_cond
>
> - The relationship definition key for 'where' is also renamed to
> 'join_cond'
>
> - The 'join_cond' value in the relationship definition is always added
> to the
>   JOIN conditions, not the WHERE conditions
>
> Does that sound right?
>
>
> * * *
>
>> The former eager option 'where' has been renamed to 'join_cond.'
>> That's ok.  However, there is logic that directs these conditions to
>> either the WHERE clause or ON clause.  If the eager join type is
>> 'inner', the conditions are added to the ON clause, but if its 'left'
>> its added to the WHERE clause.
>
> This is an accurate assessment, and bears some explaning.  The short
> version is that I wanted to support Jeff's request for "require-no-
> related" option.  (The long version follows as an aside.)
>
> {{aside:
>
>     As we know, a LEFT JOIN gets all native records whether or not
>     they have related records; an INNER JOIN gets all native
>     records that have related records (leaving out the ones that
>     have no relateds). However, it's difficult to get only native
>     records that have no related records. The only way I could
>     think of to do that in a way that maintains some adherence to
>     SQL conventions, withough inventing new terminology, was to
>     use the following idiom for eagers:
>
>         'join_type' => 'left',
>         'join_cond' => 'related.id IS NULL'
>
>     But you can't join on a NULL related.id, because it's not
>     there. ;-) Thus, for all LEFT joins, the 'join_cond' is moved
>     to the WHERE clause to support this kind of thing. The left
>     join occurs and would normally return all natives, but the
>     WHERE clause now restricts it to natives that have no related
>     record.
>
> }}
>
> I can see how the existing behavior would be regarded as too magical,
> especially for such an uncommon use case.  As such, I am willing to
> remove this behavior entirely. That would mean all 'join_cond' always
> go to the JOIN ON conditions, never the WHERE conditions. Would that
> be helpful?
>
>
>> Especially in the left join case, this
>> invites the automatic conversion to INNER bug.
>> I think the solution to this is that the eager options should include
>> both a 'where' option and a 'join_cond' option.  join_cond would
>> always be routed to the ON clause, while 'where' would always be
>> routed to the WHERE clause.  Combined with renaming 'where' to
>> 'join_cond' in Solar_Sql_Model_Related AND the _modFetch and
>> _modEagerFetch changes, I think this will clear up alot of confusion
>> about query building.
>
> Jeff opined months ago that an eager related fetch should always
> return results identical to a lazy related fetch.  I think allowing a
> direct 'where' param on eager fetches runs counter to that idea,
> because you can't specify a 'where' param on a lazy-fetch. (The eager
> 'join_cond' param and its brethren modify which native/parent records
> are fetched, not which related records are fetched.)
>
> I think that Jeff's initial idea, that eager-fetches and lazy-fetches
> should always return the same thing, is the right one.  Having a
> 'where' param would interfere with that.  I am happy to hear how I
> might be mistaken.
>
>
> * * *
>
>> Speaking of confusion.  I find the join_only flag to be a bit
>> confusing.  Perhaps reversing it would help?  So instead of
>>
>> 'join_only' => true
>>
>> One would use
>>
>> 'fetch_objects' => false
>>
>> Thoughts?
>
> I thought 'join_only' made sense (and the length was symmetrical with
> the other join keys, which soothed my OCD ;-).  "Only do a join, don't
> fetch anything" seemed reasonable to translate as 'join_only'.  Anyone
> else have thoughts here?
>
>
> * * *
>
>> There are some performance regressions from remodel.  Prior to
>> remodel, there was logic in the query building that could recognize
>> when a join was not necessary and remove it.  That logic has been
>> lost.  See Tickets #210 and #213.  I think that logic should come
> back.
>
> Fair points; I will have to address those separately.
>
>
> * * *
>
>> Back to confusion.  I find the 'join_flag' to be confusing.  The
>> description is 'whether or not this eager should be used to control
>> which parent records are selected."  May I suggest instead calling
>> this 'modifies_native?"
>
> I wasn't happy with the name either, but it seemed good-enough at the
> time.  In general, developers should not have to set 'join_flag'
> themselves, except possibly in a very few cases (close to "none").
> It's more for the internal query-building process to keep track of
> which joins are flagged to go in the parent/native select, although
> you can of course set it yourself to force it on your own.
>
>
>> The issue with tickets #210 and #213 are related to the join_flag /
>> modifies_native.  For certain kinds of queries, like count() and
>> _getNativeBySelect(), extra eager clauses have no impact, unless this
>> flag is set.  If its not, they can be dropped.
>
> Again, these are fair points, but I will have to address them
> separately.
>
>
> * * *
>
>> One last thing.  There are a massive number of options between fetch
>> options, eager options, and related options.  They interact in
> various
>> ways.  I think in general combinations of options that don't make
>> sense should raise an exception.
>
> To do that, we would need to know in advance "what doesn't make sense"
> and check for all of the possibilities.  I think that is a tremendous
> task that presents us with more to lose from than to gain.
>
>
>> In general, I think that the join_flag/modifies_native should only be
>> manually set.  Then, any eager options detected that would cause the
>> native query to be changed without this flag set should raise an
>> exception.
>
> On this point I disagree; I like that the query-builder has barely
> enough knowledge to know which joins to put into the top-level parent/
> native select without me having to tell it every single time.
>
>
>> Because of the large number of independent options without some kind
>> of constraints, building queries will always be kind of mysterious
>> because there are so many interactions possible in so many edge
>> cases.  I don't see any removable options.  We already tried merging
>> some options, which is a solution I don't think paul likes.  So,
>> perhaps we should look for more opportunities to validate
> combinations
>> of options to help remove some of the mystery?
>
> "Mystery" might be more of a documentation issue than anything else,
> to which I plead guilty.
>
>
>
>
> --
>
> Paul M. Jones
> http://paul-m-jones.com/
>
>
>
>
> _______________________________________________
> Solar-talk mailing list
> Solar-talk@...
> http://mailman-mail5.webfaction.com/listinfo/solar-talk

_______________________________________________
Solar-talk mailing list
Solar-talk@...
http://mailman-mail5.webfaction.com/listinfo/solar-talk

[Jeff Moore-4]

Oh damn.  that last wasn't supposed to go to the list :(

So sorry, feel like an email newb.

Jeff
_______________________________________________
Solar-talk mailing list
Solar-talk@...
http://mailman-mail5.webfaction.com/listinfo/solar-talk

[Jeff Moore-4]

Hi Paul,

On Oct 10, 2009, at 9:23 AM, Paul M Jones wrote:

> Good call, and I fully agree.  To sum up:
>
> - The $where property of Solar_Sql_Model_Related is renamed to
> $join_cond
>
> - The relationship definition key for 'where' is also renamed to
> 'join_cond'
>
> - The 'join_cond' value in the relationship definition is always added
> to the
>   JOIN conditions, not the WHERE conditions
>
> Does that sound right?

Sounds perfect to me.

> I can see how the existing behavior would be regarded as too magical,
> especially for such an uncommon use case.  As such, I am willing to
> remove this behavior entirely. That would mean all 'join_cond' always
> go to the JOIN ON conditions, never the WHERE conditions. Would that
> be helpful?

That's fine with one caveat.  I think this is the best behavior for  
avoiding confusion.

I don't think there is any reasonable need to support the LEFT JOIN  
case in the relationship definition.
When you are creating fetch parameters, you have access to the where  
option on the parent query, so no loss there.

The only issue, then is supporting it on eager options.  I think as  
long as the modEagerFetch method gets added with both the fetch and  
eager options parameters, then I don't see any need for where in the  
eager options, only join_cond.

Any thoughts on whether that one will end up in trunk?

>> Back to confusion.  I find the 'join_flag' to be confusing.  The
>> description is 'whether or not this eager should be used to control
>> which parent records are selected."  May I suggest instead calling
>> this 'modifies_native?"
>
> I wasn't happy with the name either, but it seemed good-enough at the
> time.  In general, developers should not have to set 'join_flag'
> themselves, except possibly in a very few cases (close to "none").
> It's more for the internal query-building process to keep track of
> which joins are flagged to go in the parent/native select, although
> you can of course set it yourself to force it on your own.

It hadn't occurred to me that join_flag was for internal use only.  
Even internally, I think that this

         if ($eager['merge'] == 'server') {
             $eager->modifiesNative(true);
         }

is clearer than

         if ($eager['merge'] == 'server') {
             $eager->joinFlag(true);
         }

I guess I'd have to hear your thoughts on tickets #210 & #213 before  
commenting on this further.  I think these are intimately tied up with  
the meaning of the join_flag.

I also wonder if #211 is caused by some kind of join_flag issue.  Now  
that I understand the remodel better, I ill spend some time tracking  
down the actual cause of that issue.

One more thing, I like the fluent interface aspects of the fetch  
parameters.  That looks really promising.

Thanks,

Jeff
_______________________________________________
Solar-talk mailing list
Solar-talk@...
http://mailman-mail5.webfaction.com/listinfo/solar-talk

[Jeff Moore-4]

On Oct 10, 2009, at 9:23 AM, Paul M Jones wrote:

>> One last thing.  There are a massive number of options between fetch
>> options, eager options, and related options.  They interact in
> various
>> ways.  I think in general combinations of options that don't make
>> sense should raise an exception.
>
> To do that, we would need to know in advance "what doesn't make sense"
> and check for all of the possibilities.  I think that is a tremendous
> task that presents us with more to lose from than to gain.

So, i've spent some time crawling around in the query building guts  
trying to figure out why a query is built one way or another.  That's  
hard to do.

There are combinations that don't make much sense.  For example:

'join_type' => 'inner' in combination with 'merge' => 'client'  This  
is all wrapped up with 'join_flag'.

I found this while looking at the line in toOne that changes the merge  
to client if the toOne related has sub-eagers.

That works ok for toMany, but breaks toOne in a way that's not obvious.

I'm not sure what the way forward is.

Jeff
_______________________________________________
Solar-talk mailing list
Solar-talk@...
http://mailman-mail5.webfaction.com/listinfo/solar-talk

[Paul M Jones-4]

On Oct 12, 2009, at 18:59 , Jeff Moore wrote:

> Hi Paul,
>
> On Oct 10, 2009, at 9:23 AM, Paul M Jones wrote:
>
>> Good call, and I fully agree.  To sum up:
>>
>> - The $where property of Solar_Sql_Model_Related is renamed to
>> $join_cond
>>
>> - The relationship definition key for 'where' is also renamed to
>> 'join_cond'
>>
>> - The 'join_cond' value in the relationship definition is always  
>> added
>> to the JOIN conditions, not the WHERE conditions
>>
>> Does that sound right?
>
> Sounds perfect to me.

Sounded perfect to me too.  Turns out it was at least partially  
wrong.  The relationship-definition conditions go in the JOIN clause  
sometimes, and in the WHERE clause sometimes.  For example:

- eager to-one: JOIN
- eager has-many: WHERE (because we fetch the eager after the native)
- eager has-many-through: the "through" conditions always go in the join
- eager has-many join_flag: JOIN for the join, then WHERE for the fetch
- lazy-load: WHERE

Cf. the new work in branches/beta, including the tests (!!!) in  
Test_Solar_Sql_Related and the corresponding mock models in  
Mock_Solar_Model.

So a simple "always and only use in the JOIN" isn't sufficient.  I'm  
considering renaming from "where" to "conditions" (instead of to  
"join_cond") to hint toward that.


> I don't think there is any reasonable need to support the LEFT JOIN
> case in the relationship definition.

It looks like LEFT JOIN has to be the default join type, to make sure  
that parent record sets are always the same regardless of eagers.  
Using "inner join" with "0=1" conditions, as noted in ticket  208 <https://solarphp.com/trac/core/ticket/208 
 >, changes the native result set, which conflicts with the goal of  
keeping native result sets the same regardless of eagers.  Let me know  
if I'm missing something here.


> The only issue, then is supporting it on eager options.  I think as
> long as the modEagerFetch method gets added with both the fetch and
> eager options parameters, then I don't see any need for where in the
> eager options, only join_cond.

That's another place where the conditions end up going in different  
places depending on the operation.  If the "where" in the relationship  
definition gets renamed to "conditions", then it would make sense to  
call the eager key "conditions" as well.



--

Paul M. Jones
http://paul-m-jones.com/




_______________________________________________
Solar-talk mailing list
Solar-talk@...
http://mailman-mail5.webfaction.com/listinfo/solar-talk

[Jeff Moore-4]

On Nov 23, 2009, at 8:48 AM, Paul M Jones wrote:

> On Oct 12, 2009, at 18:59 , Jeff Moore wrote:
>
>> Hi Paul,
>>
>> On Oct 10, 2009, at 9:23 AM, Paul M Jones wrote:
>>
>>> Good call, and I fully agree.  To sum up:
>>>
>>> - The $where property of Solar_Sql_Model_Related is renamed to
>>> $join_cond
>>>
>>> - The relationship definition key for 'where' is also renamed to
>>> 'join_cond'
>>>
>>> - The 'join_cond' value in the relationship definition is always  
>>> added
>>> to the JOIN conditions, not the WHERE conditions
>>>
>>> Does that sound right?
>>
>> Sounds perfect to me.
>
> Sounded perfect to me too.  Turns out it was at least partially  
> wrong.  The relationship-definition conditions go in the JOIN clause  
> sometimes, and in the WHERE clause sometimes.  For example:
>
> - eager to-one: JOIN
> - eager has-many: WHERE (because we fetch the eager after the native)
> - eager has-many-through: the "through" conditions always go in the join
> - eager has-many join_flag: JOIN for the join, then WHERE for the fetch
> - lazy-load: WHERE
>
> Cf. the new work in branches/beta, including the tests (!!!) in  
> Test_Solar_Sql_Related and the corresponding mock models in  
> Mock_Solar_Model.
>
> So a simple "always and only use in the JOIN" isn't sufficient.  I'm  
> considering renaming from "where" to "conditions" (instead of to  
> "join_cond") to hint toward that.

Yeah, that sounds good.  There can't be a direct mapping of object to sql concepts and still provide consistency.  There is already too much juggling of queries.  Fragments must be placed at the right location to maintain consistency.  If the fragment is over-specified (under-abstracted) in the relationship definition or query definition, then the internals cannot do the right thing.

On the other hand, if the query definition and relationship definition only allow very abstract concepts, there are queries you cannot do that you might want to do.  As we've discussed, I think the way out of this conundrum is to break apart the query building stage from the result processing stage and use a mapping description passed from the first stage to the second.  That way, each stage can evolve to support what is necessary.

I feel as long as they are coupled, there will be issues balancing back and forth between the need to represent a broad array of queries and the need to present a stable model for the purposes of writing domain logic on that model.

>> I don't think there is any reasonable need to support the LEFT JOIN
>> case in the relationship definition.
>
> It looks like LEFT JOIN has to be the default join type, to make sure  
> that parent record sets are always the same regardless of eagers.  
> Using "inner join" with "0=1" conditions, as noted in ticket  208 <https://solarphp.com/trac/core/ticket/208 
>> , changes the native result set, which conflicts with the goal of  
> keeping native result sets the same regardless of eagers.  Let me know  
> if I'm missing something here.

I think all i meant was that it wasn't necessary to have the option.  Internally, the query builder can choose as appropriate for the situation.

> The only issue, then is supporting it on eager options.  I think as
>> long as the modEagerFetch method gets added with both the fetch and
>> eager options parameters, then I don't see any need for where in the
>> eager options, only join_cond.
>
> That's another place where the conditions end up going in different  
> places depending on the operation.  If the "where" in the relationship  
> definition gets renamed to "conditions", then it would make sense to  
> call the eager key "conditions" as well.

ok.

These are just initial impressions without having run our standard set of problem queries and relationships.  (soft-delete, area restriction, large datasets, etc.)

Jeff
_______________________________________________
Solar-talk mailing list
Solar-talk@...
http://mailman-mail5.webfaction.com/listinfo/solar-talk

[Jeff Moore-4]

Thanks for your work in incorporating feedback, btw.

Jeff
_______________________________________________
Solar-talk mailing list
Solar-talk@...
http://mailman-mail5.webfaction.com/listinfo/solar-talk

[Paul M Jones-4]

On Nov 23, 2009, at 14:15 , Jeff Moore wrote:

> Thanks for your work in incorporating feedback, btw.

Dude, you are totally welcome.  I know I don't have all the best  
ideas.  Gotta have you guys to keep me honest.  :-)




--

Paul M. Jones
http://paul-m-jones.com/




_______________________________________________
Solar-talk mailing list
Solar-talk@...
http://mailman-mail5.webfaction.com/listinfo/solar-talk