Add support for polymorphism

Created an attachment (id=922)
Initial test cases

Please expand this patch into src/examples for simplicity sake .. . Whilst
this is not complete, it shows where we shoudl be heading wrt to supporting
polymorphism.

Created an attachment (id=926)
Initial (documenting) patch for src/main

• Adds several attributes/methods to ClassDescriptorImpl so that a class
  descriptor stores information about which class it extends and by what
  classes it's being extended.
• Adds code (debug statements) to SQLEngine.buildFinder() to travers extendedBy
  relations as well, and insert left outer joins (in form of log.debug()
  statements) for the classes found.

RFC: Polymorphisum support for castor
(Result of skiing meeting Januar 2005)

Example mapping
===============

<class name="myapp.Product" identity="id">
<description>Product definition</description>
<map-to table="prod"/>
<field name="id" type="integer">
<sql name="id" type="integer" />
</field>
<field name="name" type="string">
<sql name="name" type="char" />
</field>
<field name="price" type="float">
<sql name="price" type="numeric" />
</field>
</class>

<class name="myapp.Computer" extends="myapp.Product" identity="id">
<description>Computer definition, extends product</description>
<map-to table="comp"/>
<field name="id" type="integer">
<sql name="id" type="integer" />
</field>
<field name="cpu" type="string">
<sql name="cpu" type="char"/>
</field>
</class>

<class name="myapp.Notebook" extends="myapp.Computer" identity="id">
<description>Notebook definition, extends Computer</description>
<map-to table="nb"/>
<field name="id" type="integer">
<sql name="id" type="integer" />
</field>
<field name="weight" type="float">
<sql name="weight" type="numeric"/>
</field>
</class>

Basic concept
=============

For classes without extends their classname together with their identity
are used for unique identification of the object. If the class extends
another we should use the classname of the extended class together with
the identity. If the extended class also extends another the previous
rule needs to be applied recursively until the top class of the extend.
For above example classes this means:

original class | top class
----------------+---------------
myapp.Product | myapp.Product
myapp.Computer | myapp.Product
myapp.Notebook | myapp.Product

The unique identification of the object will be used as key for caching.

Database operations
===================

1. db.load(Class, Object)
-------------------------

a. Cache lookup
When using unique identification from above the object can be retrieved
from cache independed if its a top, intermediate or leaf of the extend.

b. Retrieve object from persistent storage
If object is not in cache it should be retrieved from persistent storage
by the sql query:

select prod.id, prod.name, prod.price, comp.id, comp.cpu, nb.id, nb.weight
from ((prod
left join comp on prod.id=comp.id)
left join nb on prod.id=nb.id)
where (prod.id=$)

c. Object creation
Dependend on the returned id columns we can decide which object needs to
be constructed:

prod.id | comp.id | nb.id | class
---------------------+----------------
id | null | null | myapp.Product
id | id | null | myapp.Computer
id | id | id | myapp.Notebook

Other id combinations are invalid and should throw an exception.

d. Result
If object is loaded by:

db.load(myapp.Product.class, new Integer(1));

the object is always valid and returned as is, but when loaded by:

db.load(myapp.Notebook.class, new Integer(1));

we should throw an exception (ClassCast) if the object is not a Notebook.

2. db.delete(Object)
--------------------

An object that extends another needs to be deleted in all tables begining
at the table storing the leaf (Notebook) to the top (Product) recursively.

3. db.create(Object)
--------------------

An object that extends another needs to be created in all tables begining
at the table storing the top (Product) to the leaf (Notebook) recursively.

4. db.execute(OQLQuery)
-----------------------

The oql query:

select p from myapp.Product p where p.price>1000

need to be transformed to the sql query:

select prod.id, prod.name, prod.price, comp.id, comp.cpu, nb.id, nb.weight
from ((prod
left join comp on prod.id=comp.id)
left join nb on prod.id=nb.id)
where (prod.price>1000)

But with the oql query:

select c from myapp.Computer c where c.price>1000

the join needs to be changed from left outer to inner:

select prod.id, prod.name, prod.price, comp.id, comp.cpu, nb.id, nb.weight
from ((prod
inner join comp on prod.id=comp.id)
left join nb on prod.id=nb.id)
where (prod.price>1000)

For a oql query of Notebook's the second join needs to be changed
respectively. The object creation is similar to 1c.

Refering to an extended object with 1 to 1 relation
===================================================

To be defined out of handwriten concept paper.

Refering to an extended object with 1 to many relation
======================================================

To be defined out of handwriten concept paper.

Created an attachment (id=927)
Initial patch fro src/main

Added some more code to SQLEngine.buildFinder, using addOuterJoin() for the
first time. Still undecisive what to do about the fields required.

Created an attachment (id=928)
Updated patch

This new patch includes test cases as well source code modifications. I've
added Car and Truck classes (incl. mapping definitions), and two test methods
to load a Truck instance and a Car instance that happens to be a Truck
instance.

I have extended SQLEngine.buildFinder() to use LEFT OUTER JOINs for all classes
that extend a given class to be included in the SQL generated. Have a look, and
let me know what you think.

To do:

• Modify SQLEngine.load() to analyse the ResultSet returned by
  Statement.execute() for any 'occurrence' of extending entities, and as a result
  try to load the correct class, i.e. a leaf of the class hierarchy.

Here's my currently biggest problem: the SQL statement we use for loading a
class only loads non-identity fields. In the case, where we are loading a class
that is part of an extend relation, this is not sufficient, as any such field
can be declared as 'nullable'. In other words, we need to find a way to include
the identity fields as well, at least of the 'extending' classes. Before trying
to add the relevant code, I'd like to hear opinions.

Is there a particular reason why we are using

ResultSet.getXXX (int)

rather than

ResultSet.getXXX (String)

using the column name as stored in FieldInfo.ColumnInfo ? If we were to use the
latter approach, add ing the code described above (to handle the inclusion of
identity fields into the load statement) would be quite easy.

Created an attachment (id=929)
Updated patch (2)

I have added code to SQLEngine.buildFinder() to include identity columns of
extending classes/tables as well. For this, I had to add the ColumnInfo
interface to Persistence.java and some other minor things.
I have added code to SQLEngine.load() to analyse the complete set of
(extending) ids for entries that are not null. As we ascend the hierarchy of
potential candidates (towards a leaf), we memorize the last JDO class
descriptor where the value of its id was not null. This holds (most likely) the
type of the java class we actually need to load.

Created an attachment (id=931)
Updated patch (3)

Added code to SQLEngine.load() to implement loading the actual 'leaf' object of
the extends hierarchy identified by the id value provided. For this, I slightly
had to change the signature of the load() method to allow changing the size of
the Object array.

Note: The code added at this state is admittedly quite ugly, and has some
serious flaws with regards to compound identities. Iow, if you feel like trying
things out, please stick with the samples provided .. .

I had some more 'looks' at SQLEngineload and its calling stack, namely
ClassMolder.load() and TransactionContext.load(). As it stands right now,
TransactionContext.load() creates an instance of the requested class using e.g.
ClassMolder.newInstance(), and then passes this object down the call stack.

Here's my approach: by basically creating an 'enclosing structure' to pass down
the object created (and potentially more information), I'll be able to 're-
create' this object instance as required further downstream. As long as this
object extends the requested object type, this should not be a problem.

I'll provide a patch soon, but would appreciate any feedback already now .. .

Hi Werner, had a look at your problem this evening but got another idea how this
may be handled.

1. Object to load is created in TransactionContext.load() <688-701>.

— begin snap —

if ( objectToBeLoaded != null )
object = objectToBeLoaded;
else {
if ( _instanceFactory != null )

else

}
entry = addObjectEntry( oid, object );
oid = engine.load( this, oid, object, suggestedAccessMode, _lockTimeout, results );

// rehash the object entry, because oid might have changed!
entry = rehash( object, oid );

— end snap —

Move this code from TransactionContext.load() over LockEngine.load() to
ClassMolder.load(). I am not sure this will be possible but we should give it a try.

2. Also move your new code to find the extend class from SQLEngine.load()
<1105-1137> to ClassMolder.load(). This also seams to be possible and if both
works you are able to create to right object for the first time.

Hope this will help and not confuse you as its a difficult to describe in a
few words.

Tried to move object creation code from TransactionContext.load() to
LockEngine.load() as suggested at comment 11 with the result that all test cases
except TC47 pass. Did not find out the reason why TC47 failed now.

Created an attachment (id=940)
Patch as described at comment 11

Did not manage to get TC47 working, but here is the patch what I tried.

> Move this code from TransactionContext.load() over LockEngine.load()
> to ClassMolder.load(). I am not sure this will be possible but we
> should give it a try.
Hmm .. not sure here, as each of the Classes mentioned has been designed with a
specific responsibility in mind. I'll give it a try, though, and see where it
takes me.

FYI, a quick update in other words .. .

I am already capable of loading classes using Database.load() with single and
compound identities, with Database.load() returning the actual leaf classes
rather than the class type specified in the call to Database.load(). Iow,

LaptopMulti laptop = (LaptopMulti) db.load (Computer.class, new Complex (new
Identity (1), new Identity (1));

works fine, as well as

Laptop laptop = (Laptop) db.load (Computer.class, new Identity (1));

My problem currently is that I am not able to create a patch as I do not have
connectivity to the CVS repository right now.

• • • Bug 1893 has been marked as a duplicate of this bug. ***

Created an attachment (id=958)
Refactored test from bug 1893

After refactoring there are no more failures because I fixed sequence of object
creation. But problems will arise when testing to load created objects. Can be
used as use case for polymorphism.

Ralf, can I assume that the last attachment was meant to go somewhere elese ?

Created an attachment (id=959)
Updated patch (4)

This patch adds (complete?) support for polymorphism when using
Database.load(). It desperately needs some refactoring, btu I'd like to hear
opinions first with regards to the approach chosen. Updated examples will
follow en unsegundo.

Created an attachment (id=960)
Updated examples (2)

Updated examples to showcase teh use of Database.load with various extend
hierarchies.

Werner your assumption is wrong. As bug 1893 is a duplicate of bug 1881 I
intensionally added its test cases hare after cleaning them up.

Thanks, Ralf.

Just to let you know that the refactored test case you attached from bug 1893
are fully functional.

I am just looking at ParseTreeWalker, and I wonder what's a good place to add
code to deal with extend relations, as clearly ParseTreeWalker does not deal
with such relations at all at the moment. addJoinsForPathExpression() looks
like a good place to me .. ?

Actually, there's code in ParseTreeWalker.getFieldAndClassDesc() that deals
with EXTEND relations. But somehow this code seems to be quite buggy to me.
Right after ...

// prepare the return value
retVal = new Object[]

;
if (cd != clsDesc) {
...
}

we add inner joins if the field required was found in a parent class. But
strangely enough, we do not add joins for ALL extend relations, in case there's
more than one .. . Maybe I should prepare a test case for this ....

What's actually the difference between the members _sqlLoad und _sqlFinder in
SQLEngine. _sqlLoad seems to be storing a SQL statement for loading a complete
object from the database, but what about _sqlFinder ?

• • • Bug 1137 has been marked as a duplicate of this bug. ***

Created an attachment (id=964)
Refactored test from bug 1137

Test to query extended classes.

• • • Bug 1197 has been marked as a duplicate of this bug. ***

Created an attachment (id=965)
Refactored test from bug 1197

Test to load objects of extended classes in one transaction or by 2 parallel
threads.

Ralf, just added the patch from bug 1997 to my test suite, and things run
without a problem. I just fixed the (imho) last bug that was left over from my
'hacking' session last week, but as it stands right now, I have got code ready
to be reviewed (and most likely refactored) ... .

Created an attachment (id=966)
Updated examples (3)

Updated working examples, incl. (for the first time) several calls to OQLQuery.

Created an attachment (id=967)
Updated patch (5)

Fully working patch for adding polymorphism, based upon the test cases attached
to this bug report. I have not managed to verify whether the test cases are
still executing as normal.

I just came to realize that we have some 'silent' listeners here .. . Which,
to be honest, I really appreciate. Would anybody be able and willing to help us
a bit and test what we have so far ?

It actually looks like att test cases bug TC06 (Deadlock) run successfully. And
I actually do have an idea why TC06 might not finish ... .

Unfortunately, please ignore my last comment. There's actually more test cases
that don't complete successfully, mostly around loading dependent obejcts.

Down to two failing test only (TC30 and TC47). Apparently, there was some real
mess in SQLQuery.loadMultiField() left from ... <blush> a copy-and-paste job.

Created an attachment (id=975)
Updated patch (6)

This patch basically tursn all CTF tests funcitonal (but TC06, but that seems
to be a problem by design).

Created an attachment (id=976)
Updated examples (4)

Updated examples, incl. all tests as posted by Ralf.

Created an attachment (id=977)
Updated tests.

Some unstructured changes to allow running test caes from within Eclipse.

All, any opinions welcome. And this includes comments/suggestions about
potential refactoring targets.

Btw, after creating the database, please execute the following query:

update owner set product = 1 where id = 1

And don't ask me why .... . After executing that query, all tests run fine ...

Created an attachment (id=980)
Updated examples (5)

Updated examples, incl. yet more tests added to TestPolymorphism. In addition,
I have moved all tests from test1997 and test1893 in the polymorphism package
(incl. the DDLs and the mappings). Plus there's a test suite now named
AllTests.

I have just done a bit of profiling, and in general, it looks like we spend a
lot of time in LockEngine.load(), ClassMolder.load, ClassMolder.loadFields(),
SQLEngine.load(), etc. Parts of this is due to establishing JDBC connections,
but most of it is a result of inefficient code, I guess. All these for () loops
wuth switch statemets don't really make the story any faster .. . Looks like
it is about time to start refactoring ....

Created an attachment (id=981)
Thread call graph

This thread call graph (courtesy of Eclipse Colourer) quite nicely shows where
a lot of time is spent (ignoring DatabaseRegistry.createConnection(), of
course).

Be really careful with that section - that's never been good anyways, and there's all kinds of ugly stuff
in there.

I'm temtped to say create a new bug and close this one off; then we'll look at each class in turn and see
if there's something we can do to end the loop-walking that doesn't involve getting everything to do
the loop walking. My goal of removing all the loops from my previous attempt at a TransactionManager
rewrite make me believe that they're all interdependent, and that getting rid of loopwalking is going to
need some pretty fundamental, strategic changes to the way we examine and process status
information.

Inefficient code down in those areas is why Thomas and I have talked about
refactoring everything around the ClassMolder/FieldMolder area. I just sent an
email with Thomas' refactoring docs attached.

Thanks, Bruce. Gregory, what's your comment referring to specifically ?

Gregory ?

Gregory, I can now see where you are coming from, as that's what we have been trying to resolve last weekend. Point taken ..

Gregory, I'd really like to see a separate bug report for the TransactionContext(impl) rewrite, as I'd like to discuss making a new preview release available, etc. Please, pretty please. Please ?

Based upon my work to add support for polymorphism, I think that this code really is not used at all, and hence should/could be removed.

If there's code in there that you think isn't being used, IMO, comment it out; we're in desperate need of reducing the complexity of this software, and knowing what doesn't need to get refactored out would be a huge boon.

(Sorry for spamming this bug; I misread on the previous patch comments, now deleted.)

We're going to have to work through each of these classes and normalise their behavior. In each, it's likely that we can make use of the ObjectTracker to track the state in a way that should allow us to get rid of the loopwalking - with the potentially damaging side-effects that occur when you get it wrong, as you've now witnessed while we were working on the other patch.

I say we find the next-biggest-pig and shoot it in the head. In all likelihood, that's going to mean probably starting at the top and working our way in, rather than starting at the bottom (sqlengine) and working our way up, and doing so (ideally) in the same way we have with TransactionManager - clean up the codebase so it makes sense, and then once we have something clean and behaves in a sane manner, we can start moving responsibilities around in the codebase and try to improve their APIs and interlocking (which at the moment is way, way too deeply interconnected).

> If there's code in there that you think isn't being used, IMO,
> comment it out; we're in desperate need of reducing the
> complexity of this software, and knowing what doesn't need
> to get refactored out would be a huge boon.
I'll definitely get this commented/removed once polymorphism has been added to the codebase. If you don't mind, I'd like to see this delayed by a week or so, as I feel like we should provide for a maintenance release (0.9.6.1) within 14 days or so.

Updated patch too allow for changes to TransactionContext after introducing ObjectTracker.

To-Do:

• move tests from src/examples/polymorphism to src/tests/jdo (or a sub-package thereof).
• check whetehr test suite runs
• checkstye
• documentation
• release notes

Final patch, incl. small modifications to src/tests/jdo/CallSql.java to cater for new select statement syntax, new src/tests/ctf/jdo/tc9x package with several new tests for polymorphism, etc. Code is checkstyled, and all CTF tests (incl. the new ones) complete sucessfully.

To-do:

a) Transfer SQL script changes to all remaining databases & test on those.
b) Code review ...

At a first test i found following problems:
1. PTF fails.
2. CTF TC97 fails with following stacktrace:

java.sql.SQLException: Base table or view not found message from server: "Table 'test.order_product' doesn't exist"
at com.mysql.jdbc.MysqlIO.checkErrorPacket(MysqlIO.java:1997)
at com.mysql.jdbc.MysqlIO.sendCommand(MysqlIO.java:1167)
at com.mysql.jdbc.MysqlIO.sqlQueryDirect(MysqlIO.java:1278)
at com.mysql.jdbc.Connection.execSQL(Connection.java:2251)
at com.mysql.jdbc.PreparedStatement.executeQuery(PreparedStatement.java:1586)
at org.exolab.castor.jdo.engine.SQLEngine.load(SQLEngine.java:1180)
at org.exolab.castor.persist.ClassMolder.loadFields(ClassMolder.java:750)
at org.exolab.castor.persist.ClassMolder.load(ClassMolder.java:792)
at org.exolab.castor.persist.LockEngine.load(LockEngine.java:368)
at org.castor.persist.TransactionContext.load(TransactionContext.java:718)
at org.castor.persist.TransactionContext.load(TransactionContext.java:554)
at org.exolab.castor.jdo.engine.DatabaseImpl.load(DatabaseImpl.java:351)
at org.exolab.castor.jdo.engine.DatabaseImpl.load(DatabaseImpl.java:315)
at ctf.jdo.tc9x.TestPolymorphism.testLoadLaptop(TestPolymorphism.java:85)
at ctf.jdo.tc9x.TestPolymorphism.runTest(TestPolymorphism.java:58)
at junit.framework.TestCase.runBare(TestCase.java:127)
at junit.framework.TestResult$1.protect(TestResult.java:106)
at junit.framework.TestResult.runProtected(TestResult.java:124)
at junit.framework.TestResult.run(TestResult.java:109)
at junit.framework.TestCase.run(TestCase.java:118)
at harness.CastorTestCase.run(CastorTestCase.java:154)
at junit.framework.TestSuite.runTest(TestSuite.java:208)
at junit.framework.TestSuite.run(TestSuite.java:203)
at harness.TestHarness.run(TestHarness.java:140)
at junit.framework.TestSuite.runTest(TestSuite.java:208)
at junit.framework.TestSuite.run(TestSuite.java:203)
at harness.TestHarness.run(TestHarness.java:140)
at Main.run(Main.java:152)
at junit.textui.TestRunner.doRun(TestRunner.java:116)
at junit.textui.TestRunner.doRun(TestRunner.java:109)
at junit.textui.TestRunner.run(TestRunner.java:72)
at Main.main(Main.java:226)

Updated patch (modified src/tests/ctf/jdo/tc9x/mapping.xml to cater for renamed tables).

Updated patch, fixing the problem with ArrayIndexOutOfBoundExceptions when running the PTF:

Current result of performance test suite.

This patch includes three small performance improvements:

a) Introduced a SQLEngine cache in BaseFactory that will be used in BaseFactory.getPersistence(ClassDescriptor) to store ClassDescriptor --> SQLEngine mappings once a SQLEngine has been generated.
b) Moved code to calculate the number and collection of extending class descriptors for a given class from SQLEngine.load() to SQLEngine.<init>, thus avoiding this code to be executed on every call to SQLEngine.load().

The bad news is that your changes didn't make any difference for PTF but, what may be more important, I found the problem. It had been the logging statments in the ClassMolder.load() methods. Therefore i removed them and also most of the others you intruduced e.g. in SQLEngine. Now the Polymorphism patch has very small influence on PTF.

In addition I reviewed the whole main code and didn't found issues except some small fixes included in the new patch. The missing parts IMO are:

a) PTF results for 20000 objects
b) Review of test cases
c) SQL scripts for all the other engines

Added release notes and copied changes to src/tests/jdo/mysql.sql to all other <database>.sql. files.

Updated patch with finished PTF results and reviewed CTF test cases. Found 2 situations where I am not sure if we should add more tests:

1. test of polymorphism when using keygen
2. test long transactions

are missing. In addition I fixed issues in hsql.sql script and tried to run CTF tests on hsql. I had been very surrprised to see that most of the tests that could successfully be executed before applying this patch now fail. In most cases they throw a SQLException 'column not found:7' which brings me to the conclusion that there most still be something wrong in SQLEngine that needs to be resolved before commit. As hsql isn't the most important db engine to support we at minimum need to execute tests on most of the other supported db engines.

I'll try to find the time to execute tests against oracle and postgres.

Werner, found the problem why CTF fails at hsql. Patch now also passes CTF tests with hsql in addition to mysql.

Something is going horrobly wrong with oracle. At SQLEngine.loadSingleField() (line 2281) the variable 'columnTableName' is always empty and the for loop therefore never finishes. In addition we now get a FATAL exception according to the wrong KeyGen that had been a WARN before.

The over all concequence is that NO test case of CTF can be executed and I don't realy know what to do.

Added test case for using MAX key generator with polymorphism.

Problem introduced by issue CASTOR-1018.

Fixed ORACLE problem with one exception: it is not possible to use LIMIT clause at extended classes.
New patch is tested against mysql and oracle. Previous tests on other db's need to be executed again.

Added yet another test case (TC94) to test Database.update(Object) with polymorphism included. So far, I have only added the test case to the 'mysql' category of src/tests/tests.xml.

Patch updated after commit of CASTOR-1110.

Updated patch, incl. changes to db2.sql to get the CTF test suite running for the new test cases TC94 to TC99. Having said that, there's one or two minor issues with some SQL statements that are a result of committing bug CASTOR-1110.

Final patch with some additional code cleanups at test objects. Will commit that in a minute.