Re: Joins -- Details

Hi Anthony,

I have made quite a few modification following your remarks. See my
comments below...

> 
> In SQuery, I suspect that "Table" should be renamed to be "Alias" --
> it does not actually represent a table. (or maybe SAlias if you prefer
> to import the inner class directly into SDriver.) 

Ok, I renamed Table to SAlias. I'll use SAlias for Table in the rest of
this mail...

> 
> I do not understand the relationship between Table, SQuery, and Join.
> Table and Join appear to be one-to one. Except for the main table. But
> then there is also a SQuery.mainAlias which I would think is
> redundant. 

A SQuery has N SAlias.
A SQuery has N-1 Join.
A Join has two SAliases, one for each side of the join, plus the
SFieldReference that makes the link.
SQuery.mainAlias is only a string to be able to find the main SAlias in
the list.

> 
> I strongly suspect that you only need one inner class, "Alias". An
> instance is created for the top level table, but with its reference
> left null. Instances are then created for each joined table. Related
> fields are then removed from SQuery.
> 
I think going this way will make things much more complex, as we will
end up with a linkedlist strucutre. Traveling this structure will
requiere than we reinvent the wheel of java collection API. Having a
java.util.List a SAlias, and a java.util.List of joins (the relations
betwen them) seems simpler to me.

> I think that the language is a bit confused. You have SFieldReferences
> which have a refing and a refed SRecord, always representing a many to
> one. But then Join has a referingTable which may be a refing or a
> refed depending upon which way the reference goes.
> 
> I think that you need different words, eg. fromAlias and toAlias.
> (Words are very important in complex code like this.)
> 
Agreed. I changed the words and used toAlias for the table that is
joined and is not yet in the query, and fromAlias for the table that is
already in the query and holds one of the sides of the join.

> (Do you really need to be able to one-to-many joins? Aren't they just
> many-to-one joins back to front? Would make the code simpler.)
> 
I agree One-to-many joins add complexity in the code.
They are useful in the case when you have one header table, with several
line tables. If we only allow many-to-one joins, we are unable to
retrieve a header and its different line types. For example, say I want
to retrieve a PaySlip with its details and its employee in one go. I
can't do that with many-to-one only joins.

> addJoin, doJoin probably belong in the Join class just to structure
> the code a bit.
> 
Right. I moved doJoin to Join constructor.
addJoin on the other hand seems to belong to SQuery.

> More internal documentation on SQuery as to what the data structures
> mean would be helpful. Maybe an example query and then what goes
> where. 
> 
> I don't understand what currentRsindex means. Should be using a
> LinkedHashMap for "table"? Can you handle multi column keys? What does
> "RS" stand for? Looks dubious.

currentRsIndex is an instance field of SQuery that holds the current
number of fields that are to be retrieved when the query will be
executed. Each SAlias will have its own firstFieldIndex when created,
computed from currentRsIndex. This is used later to find the fields we
want to retrieve from the resultSet. (what we where doing before was
iterate over every field of the select list for each record we wanted to
retrieve : this solution was quite ineffective, and not suited for table
that were added multiple times.
> 
> JOIN API
> =======
> 
> The order of the parameters .join("dept", "manager",
> Employee.DEPARTMENT) is a bit confusing. dept-empee-dept.

Well, it is really 'join( to "dept" , from "manager" , using "a
reference than makes the link between dept and manager" and that happens
to belong to manager)'

>  Maybe
> .join("dept", Employee.DEPARTMENT, "manager")
> Or
> .join("Employee.DEPARTMENT, "manager", dept")
> 

Also consider english is not my mother tongue, so parameters order is a
bit less natural to me (in french we tend to have another order than in
english for the words in the sentence...). So I really don't know :)

> There is also far too many constructors for SQuery. Suggest take alias
> out and replace with setAlias. I think also create setSelectMode and
> setQueryMode and deprecate the corresponding constructors. (The set
> methods should return this.)
> 
Done for setQueryMode.
SelectMode is more problematic, as it is used to construct selectList
and will affect the way currentFieldIndex is used.

> S ALIASED FIELD
> =============
> 
> The first thing that I note is that SDriver.selectSQL still contains 
> for (SFieldScalar sfld : from.getSelectList()) 
> 
> I would think that once you introduce aliases you need to have them
> everywhere. So if you retrieve employees and their managers, you would
> need to retrieve both employee.Name and manger.Name, say. So we would
> need to create a SAliasedField class. Ouch!

No. That what the currendRsIndex (renamed to currendFieldNumber) is
for : to match the fields retrieved from the database to the SAlias they
belong to. See sessionHelper.retrieveRecord()
> 
> COMPARE/NULL KEYS INDEX
> =====================
> 
> I do not understand why you needed these. 

In fact, to make SRecordInstance Comparable, we need a way to impose an
order on them. Ordering records based on their primary key(s) value
seemed the way to go. This order should be consistant with equals (it is
not mandatory but strongly advised).
SRecordInstace.equals() has a special behaviour to handle null primary
keys (for generated keys while not in session) : two instances with null
keys are NOT equal. So they must have a defined order, and we cannot
just rely on comparing two nulls for that

(See also the following excerpt from Comparable javadoc : Note that null
is not an instance of any class, and e.compareTo(null) should throw a
NullPointerException even though e.equals(null) returns false.)

So we must handle the case...


> 
> I am concerned about inconsistent behaviors with null keys index as
> the keys are assigned. It does not feel right at all.

Oooops. This is a bug ! I added a method to reset this field when a key
is generated (on flush). I think this should do the trick.
> 
> Do you really need to sort on primary keys? And if you do, what is
> wrong with just having the null keys sorted to the beginning in update
> order?

It could be a possibility, but I choosed to put nulls last for two
reasons :

1. some sql put nulls last be default (incl. Postgresql, DB2, Oracle).
2. nulls key will probably be newly added records with sequences (or
select max...), so at the end they will probably end up having a greater
primary key than the one that already have a key.

>  (The Java sorts should be stable, ie. preserve the order of records
> with keys of equal value.)

Yes, but keys with null value are not equal... They are null, so not
equal to anything.

> 
> OTHER
> =====
> 
> I would prefer "queryRecordsInDataSet" to be called something like
> "findAllRecords". Later I would like to be able to apply SQueries to
> SDataSets when disconnected, so would prefer to reserve the word
> "query" for that. 

Done.

> 
> I have used SException.Error for user errors, can be helpful if we
> have just an exception and no stack trace.
> Certainly I would be careful about reusing Java exceptions -- the
> following could be misleading:-
> ClassCastException("Only homogeneous records (same meta) can be
> comprared");
> 
> But certainly please always include extra information that could help
> a user, eg. the record instances that did not compare. Everything has
> a toString so that should be easy. Need not be pretty.

Changed to SException.Error and more helpful error message.

> 
> =======
> 
> Let's deal with the issues in this email, and then I'll have another
> careful read looking for bugs.
> 
> Thanks for all the work you have done,
> 
> Anthony
> 
> At 12:51 AM 21/03/2009, you wrote:
> >Hi Anthony,
> >
> >I've come up with a working implementation of "complex" joins in
> >simpleorm that can do the following:
> >
> >- aliasing tables
> >- joining the same table multiple times
> >- left joins
> >- one to many joins
> >- join on joined tables (not only on the main table of the query)
> >
> >I have added tests, that work on hsql, postgresql and oracle.
> >
> >There is still a point that bothers me about semantics...
> >
> >Right now, ses.query returns a List, which is actually a Set that
> >retains the order. A LinkedHashSet could have been more accurate, but
> >it's an implementation, not an interface.
> >
> >That said, having many to one joins opens the door for requests that
> >will return several time the same instance of the main table of the
> >query, so the question is, should we :
> >
> >1) return a List with several times the same record ?
> >2) return a List which would really be a set retaining the order of
> >insertion (but then an order by on a joined table would be
> problematic,
> >as if we have R1,R2,R1,R3, the set would be R1,R2,R3) ?
> >3) should we change the API :-( to return a LinkedHashSet ?
> >4) should we add a method querySet and offer the alternative (either
> a
> >List with potentially several time the same record and true order by,
> or
> >a set with semi-order by)...
> >
> >Right now I have opted for 2), but I'm not sure.
> >What is your opinion on this ?
> >
> >Franck
> 
> Dr Anthony Berglas, [email protected] Mobile: +61 4 4838 8874
> Just because it is possible to push twigs along the ground with ones
> nose
> does not necessarily mean that is the best way to collect firewood.
> 
> 
> 
> 
> 




------------------------------------

Yahoo! Groups Links

<*> To visit your group on the web, go to:
    http://groups.yahoo.com/group/SimpleORM/

<*> Your email settings:
    Individual Email | Traditional

<*> To change settings online go to:
    http://groups.yahoo.com/group/SimpleORM/join
    (Yahoo! ID required)

<*> To change settings via email:
    mailto:[email protected] 
    mailto:[email protected]

<*> To unsubscribe from this group, send an email to:
    [email protected]

<*> Your use of Yahoo! Groups is subject to:
    http://docs.yahoo.com/info/terms/