incubator-empire-db-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 20:18:59 GMT
Hi McKinley,

before you continue, I just want to let you know that I have already done some changes here
that I have not yet submitted.
At the moment I am thinking about replacing the boolean Required parameter by an enum and
provide a new function called isAutoGenerated().
This could then be used for the timestamp column as well (which is also auto-generated) and
the uniqueidentifier.

However again it is not that simple.
In order to use the Statement.RETURN_GENERATED_KEYS from JDBC correctly I still need to know
whether it is of dataType AutoInc since this does not work for uniqueidentifier types with
ROWGUIDCOL set.
e.g. in DBRowSet:
                    if (col.isAutoIncrement() && empty) 
                    { // Set Autoinc value if not already done
                        if (db.getDriver().isSupported(DBDriverFeature.SEQUENCES)==false)
                        {  // Post Dectect Autoinc Value
                           setGenKey = new DBSetGenKey(fields, i);
                           continue;
                        }
                        // Get Next Sequence value
                        fields[i] = value = col.getRecordDefaultValue(conn);
                        empty = ObjectUtils.isEmpty(value);
                    }
This code does not work for anything else than the AUTOINC datatype that we already support
as far as I can see.
Or in other words: at the moment it looks that even with such a new property we would still
need to check against the AUTOINC datatype for the above example. I am surprised that you
have not come across this problem yet.

Also I am having problems with your case statements example (see below).
The whole idea is that the database driver must be able to decide what the best (or only)
numeric type for AUTOINC columns is, whereas with other columns the user decides on scale
an precision. I doubt that there are any databases natively supporting anything else than
integer (non-fractional) numbers and it does makes absolutely no sense to limit the number
of digits. 
Hence I do not agree that AUTOINC is simply INTEGER plus + " IDENTITY". 
And since we already have three numeric types INTEGER, DECIMAL and DOUBLE there is no reason
why we should not take on AUTOINC as one of its own as well. Again: For DDL generation the
driver MUST decide on the best type and for DML it does not matter!

At the moment the support of MSSQL's uniqueidentifier is my benchmark i.e. only if we can
support this correctly a change makes sense to me.
And the goal has to be to have the Basic Sample running with it - including database creation
(although I probably won't check this in).
If anyone knows another database with support for any other auto generated values rather than
numeric integers then please let me know.

@McKinley: Give me another day or two and I will come up with a suggestion.

Regards
Rainer


McKinley wrote:
> Re: DataType.AUTOINC Probably Needs a Change
> 
> On Sun, Jan 24, 2010 at 12:29 PM, Rainer Döbele <doebele@esteam.de>
> wrote:
> > 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.
> 
> I agree with this to the degree that I have thought about it which is
> not much. My changes are invasive, but I wanted to err on the side of
> putting the declarations at the same level as DataType type. I figured
> that if it was at that same level I wasn't going to remove the auto
> inc meta data from anyone else's usage. If you cannot anticipate why
> it should be so high up the chain, it can move down.
> 
> > 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.
> 
> I intentionally did not do a patch for the drivers right now so that I
> didn't submit too much at once. My patch does not support DDL in the
> drivers right now, but if a developer continues to use AUTOINC they
> will still have all the same behavior because AUTOINC hasn't changed.
> 
> Now, on the question of having to change the drivers, they have to be
> changed any way. They need to be changed to support decimal auto inc.
> I should probably submit a patch for the SQL Server driver at least so
> that everyone can visualize. But a short really lazy example follows.
> 
> CASE: INTEGER
>      // Some Integer stuff
> CASE: AUTOINC
>      // Most of the same stuff from INTEGER
>      // plus
>      + " IDENTITY" // This will not work on 'uniqueidentifier' in MSSQL
> CASE: DECIMAL
>      // Oh, no! our identity column could be DECIMAL,
>      // but we are not going to get any of the AUTOINC stuff here,
>      // and we have to repeat all INTEGER and DECIMAL bussiness
>      // under the AUTOINC case too.
> CASE: UNKNOWN
>      // Default for uniqueidentifier
> 
> Now, in the above you can see that the AUTOINC case needs to get
> loaded with any specifics from different datatypes. That is one way to
> do it, but it can be moved from that method and established from the
> outside.
> 
> String someDDL = "create table " + getTableName() +  " ( ";
> for(c : columns) {
>      c.getName() + " " + getDataTypeInfo(c.getDataType()) + // Where
> AUTOINC is or used to be
>      getAutoIncInfo(c.getDataType(), c.isAutoIncrement()) // Will
> return ' IDENTITY' in the case of INTEGER or DECIMAL only.
> }
> 
> In the above example a getAutoIncInfo method would just have things
> that might need to be done on for the intersection of certain data
> types and the auto inc attribute.
> 
> > 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?
> 
> That is a good question, but the most important support for Guid is
> not for migration, but that a column in a primary key can insert
> without having to specify a value.
> 
> > 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.
> 
> I kept it near to DataType, but I had no other attachment to its
> placement. I wish that there was a more readable way to establish
> final values in Java object constructors.
> 
> > 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?
> 
> In my example above, it has more to do with int and decimal not having
> to overload the auto inc case. That is going to get messy any way. It
> will just get worse with uniqueidentifier.
> 
> > 3. How have you implemented your support for Uniqueidentifier for
> SQL-Server at the moment?
> 
> I have not done DDL work with it, but the JDBC driver returns a String
> by defualt when read. For inserts Empire-db thinks it is OK because at
> the command level it passes all the checks of being in the primary key
> and being auto inc.
> 
> What I will do next is write a patch for the SQL Server driver for the
> fixes for AUTOINC for INTEGER and DECIMAL in the current style. Part
> of this patch will include an interception of AUTOINC from not being
> UNKNOWN for the benefit of validation at the command/record level.
> Also, I'll write a patch using a separate autoinc attribute too. We
> can see what the impact is.
> 
> Thanks for the feedback,
> 
> McKinley

Mime
View raw message