empire-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Rainer Döbele <doeb...@esteam.de>
Subject re: DataType.AUTOINC Probably Needs a Change
Date Sun, 24 Jan 2010 12:29:39 GMT
Hi McKinley (or do you have a first name)

I have had a look at your patch, but I am still see a few problems.

Generally I like the idea of having a function that determines whether a column is auto generated
rather than have the same comparison on the datatype every time. But I think it should go
in the Column interface rather than in the ColumnExpr interface just like getSize() and isReadOnly().
This makes the change much less invasive too.

However, as I told you before, I am not sure what exactly we are trying to achieve.
For numeric autoinc columns there really isn't any benefit.
On contrary: What your patch is missing is that now we'd have to change all the drivers to
treat a datatype of INTEGER that is declared as Autoinc the same way as the datatype AUTOINC.

What convinced me at first, was your argument about the Uniqueidentifier type with RowGuid
set to true in SQL-Server.
However at the moment we do not support this type at all.
In order to support it, we will have to introduce a new DataType anyway.
But then how do we treat this datatype in other databases?

Another thing I would like to improve is the overload of addColumn in DBTable.
Maybe we could find a better way than inserting the Boolean between DataType and the size.
I would like to make the column definition more human readable.

In order to get it right, can you give me an answer to the following questions:
1. Apart from the Uniqueidentifier in SQL Server what exactly were your problems with the
way autoinc's were implemented? 
2. Do you see another need for an autoinc property except for the datatype INTEGER and UNIQUEID?
3. How have you implemented your support for Uniqueidentifier for SQL-Server at the moment?

Looking forward to hearing from you.


McKinley wrote:
> Re: DataType.AUTOINC Probably Needs a Change
> Here is the patch. It is clean and complete and compatible with the
> existing usage of DataType.AUTOINC. No one has to change anything in
> their code (correct me if I'm wrong).
> The patch required minor additions to more files than I initially
> expected because of the interface and abstract class depth.
> https://issues.apache.org/jira/browse/EMPIREDB-71
> Thanks,
> McKinley
> On Tue, Jan 19, 2010 at 11:15 PM, McKinley <mckinley1411@gmail.com>
> wrote:
> > Adding one boolean and its setter and getter and changing only 6 or
> > fewer instances of lines like col.getDataType()==DataType.AUTOINC to
> > col.isAutoIncrement() fixes every DataType bug or missing feature
> > related to AUTOINC for free. It is 12 lines of code divided by 5
> > files.
> > The meta data on the patch will be bigger than the patch for the DML
> > part of this change. The DDL fixes are more about adding more
> > information to support currently unsupported declarations than fixing
> > bugs.

View raw message