Renaming tables and columns

15 views
Skip to first unread message

Thomas Kellerer

unread,
Feb 18, 2010, 1:31:02 PM2/18/10
to Architect Developers
Hi,

during my testing of the Liquibase DDL Generator I noticed that
renaming a table or column is detected as a DROP/CREATE from within
PA.

After looking at the code it seems that a name change can easily be
detected by looking at the UUID attribute of the SQLObject.

As far as I can tell it is "quite" easy to integrate into the diff
process by:

1) in CompareSQL when detecting a name change, check if the UUIDs of
the two tables are identical. If they are treat this as a name
change.
2) enhance DiffType to include a NAME_CHANGE value.
3) enhance DiffChunk to also be able to store the "old" object
4) in the generators detect the chunk.getType() == NAME_CHANGE and
react properly.
5) enhance DDLGenerator to define new methods: renameTable(old, new),
renameColumn(old, new), renameRelationship(old, new) (and possibly
renameIndex() as well)

I have already applied this for my liquibase DDL Generator and the
"English Language" formatter to detect table name changes. It should
be quite easy to add that to the existing DDL generators.

There is one thing I'm not really sure about though:

In CompareSQL I'm currently advancing the "other" when a name change
is detected.

The old code was something like:

if (comparator.compare(sourceTable, targetTable) < 0) {
// add a diff chunk LEFT_ONLY
}
sourceTable = (SQLTable) sourceIter.next();

Now if it's a name change I had to add a targetIter.next() as well.
Otherwise the targetTable would have been recorded with an add table.
Is that the correct way?

I'm also wondering if the UUID for this purpose is really correct.
Or will that fail in some cases I can't see right now?

If there are no objections to this approach I would add the necessary
methods to the DDL generators and commit that together with my
Liquibase enhancement

Regards
Thomas

There are two things that I'm

Thomas Kellerer

unread,
Feb 18, 2010, 2:58:25 PM2/18/10
to Architect Developers
OK after doing some more testing (especially with more complex
datamodels) it doesn't seem that easy.

The iterators that are used to compare tables/columns are doing that
in an alphabetical order. If renaming the table/column puts it into a
different place in that list, it will still be assumed to be a new
column/table.

So (as I really want to detect name changes ;) ) I think the
generateColumnDiffs() and generateTableDiffs() should run through the
list sorted by UUID rather than the name. That would make the
comparison robust enough agains changes in the order due to renames,
but as far as I can tell it would still detect new/removed table or
columns.

Regards
Thomas

Joey Silva

unread,
Feb 18, 2010, 2:58:56 PM2/18/10
to Architect Developers
Hi Thomas,

Coincidentally, I have actually been working on changes to the
CompareDM to display changes to properties of the objects (not just
showing fact that they were modified). This involves side-stepping the
already in place CompareSQL class in favour of another Differ class
that had already been created, which is better suited for calculating
property changes.

However, I'm only doing this for the English descriptions, so I have
somewhat split the execution of CompareDM depending on the output
format. This means while I have almost finished implementing what you
are working on for English descriptions, DDL generation will remain
the same, and therefore still require your changes.

I am hesitant about your suggestion of using an "old" object field in
the DiffChunk class, because of the way I have implemented my changes.
I have created a DiffInfo class which will be used in the DiffChunk as
the data field, as opposed to SQLObject (DiffChunk<DiffInfo>). This
data field contains a representation of the SQLObject (Strings for its
name and type), and includes a list of property changes.

Since you seem to only need the old name of the object for your
purposes, not a reference to it, perhaps you could implement a String
field in DiffChunk, which would be able to be used more generally. For
example, if you called it originalName, it could be used in place of
data.getName() in the English generation so that it would display the
original name, regardless of changes to the name that might or might
not have been made (it would maybe look something like this: "Table
<orginalName> has been modified -> name changed to <newName>").

I am still working on my changes, and unfortunately cannot commit
without breaking a lot of stuff, but hopefully I will be finished
before 7 today.

Regarding your questions about CompareSQL, I don't know the class well
enough to answer them.


On Feb 18, 1:31 pm, Thomas Kellerer <google-gro...@sql-workbench.net>
wrote:

Thomas Kellerer

unread,
Feb 18, 2010, 3:09:56 PM2/18/10
to Architect Developers
Hi

> I am hesitant about your suggestion of using an "old" object field in
> the DiffChunk class, because of the way I have implemented my changes.
> I have created a DiffInfo class which will be used in the DiffChunk as
> the data field, as opposed to SQLObject (DiffChunk<DiffInfo>). This
> data field contains a representation of the SQLObject (Strings for its
> name and type), and includes a list of property changes.
>
> Since you seem to only need the old name of the object for your
> purposes, not a reference to it, perhaps you could implement a String
> field in DiffChunk, which would be able to be used more generally. For
> example, if you called it originalName, it could be used in place of
> data.getName() in the English generation so that it would display the
> original name, regardless of changes to the name that might or might
> not have been made (it would maybe look something like this: "Table
> <orginalName> has been modified -> name changed to <newName>").

Hmm, I think this could work. Some DBMS (e.g. MySQL and SQL Server if
I'm not mistaken) need a full column definition even when renaming the
column. But that could be taken from the reference to the "new" column
as well (or probably has to, as the definition can change just as well
together with a name)

So I wouldn't mind that solution.

Don't worry about committing in-compatible changes. I'll see how my
stuff fits into the new structure once you commit your things.
I think I'll need to do some more testing and debugging in order to
fully understand the algorithm that is used to detect the changes.

Regards
Thomas


Thomas Kellerer

unread,
Feb 19, 2010, 4:26:49 PM2/19/10
to Architect Developers
> So (as I really want to detect name changes ;) ) I think the
> generateColumnDiffs() and generateTableDiffs() should run through the
> list sorted by UUID rather than the name. That would make the
> comparison robust enough agains changes in the order due to renames,
> but as far as I can tell it would still detect new/removed table or
> columns.

As far as I can tell sorting the diff-lists by UUID rather than name
makes the detection of renames possible (and stable). All other
changes still seem to be covered.

I had to change SQLRelationshipComparator as well. Checking if the
constraints reference to the same tables is now done through the UUID
as well, otherwise all constraints for tables that have been renamed
are reported as beeing changed even though they haven't.

Does anyone see any problems with this approach?

Thomas

Jonathan Fuerth

unread,
Feb 19, 2010, 4:38:02 PM2/19/10
to architect-...@googlegroups.com
On Fri, Feb 19, 2010 at 4:26 PM, Thomas Kellerer <google...@sql-workbench.net> wrote:
As far as I can tell sorting the diff-lists by UUID rather than name
makes the detection of renames possible (and stable). All other
changes still seem to be covered.

I had to change SQLRelationshipComparator as well. Checking if the
constraints reference to the same tables is now done through the UUID
as well, otherwise all constraints for tables that have been renamed
are reported as beeing changed even though they haven't.

Does anyone see any problems with this approach?

I'm excited about this functionality. We've been considering it for years, but simply never had a concrete reason to implement it, so it always remained a distant wish.

I do have a few concerns about the UUID approach, but I haven't looked deeply enough at the problem to know if they're valid. Hopefully you can reassure me. :)

What about the ordering of dependencies: could it now happen that a column gets dropped before the foreign key constraint that references it?

Does everything still work as expected when comparing a project to a physical database (or a physical database to itself?)

Have a great weekend.

-Jonathan

Thomas Kellerer

unread,
Feb 19, 2010, 4:44:48 PM2/19/10
to Architect Developers
On Feb 19, 10:38 pm, Jonathan Fuerth <fue...@sqlpower.ca> wrote:
> I do have a few concerns about the UUID approach, but I haven't looked
> deeply enough at the problem to know if they're valid. Hopefully you can
> reassure me. :)

> What about the ordering of dependencies: could it now happen that a column
> gets dropped before the foreign key constraint that references it?

Do you have any more concerns? If so, please let me know, so I can
test more specific things as well.

> Does everything still work as expected when comparing a project to a
> physical database (or a physical database to itself?)

Yes I will need to do more testing. So far I have only been comparing
two PA projects but comparing databases was the next step (maybe I can
also find the reason why sometimes two identical columns are reported
as different. There has been a thread regarding this on the PA forum).

I was wondering if the algorithm used to compare the list of objects
could be made more compact. I was thinking about putting the objects
into two maps where the key is the UUID. While traversing one map, the
key is looked up in the other. If it's found, the two objects are
compared. If it's not found it's added as DiffType.LEFT_ONLY. The the
object is removed from the map. Anything that's left in the "right"
map is then added as DiffType.RIGHT_ONLY

But for the time being (and until I have tested and understood the
current approach) I'll stick to the current iterator approach.

> Have a great weekend.
It's gonna a be very rainy and unfriendly weather this weekend, so I
guess I have some time for PA improvements ;)

Regards
Thomas

Jonathan Fuerth

unread,
Feb 19, 2010, 5:23:59 PM2/19/10
to architect-...@googlegroups.com
On Fri, Feb 19, 2010 at 4:44 PM, Thomas Kellerer <google...@sql-workbench.net> wrote:
On Feb 19, 10:38 pm, Jonathan Fuerth <fue...@sqlpower.ca> wrote:
> What about the ordering of dependencies: could it now happen that a column
> gets dropped before the foreign key constraint that references it?

Do you have any more concerns? If so, please let me know, so I can
test more specific things as well.

I guess one other thing for that list would be the interaction between adding and removing tables and creating and dropping FK constraints on those tables. Clearly, order matters.
 

> Does everything still work as expected when comparing a project to a
> physical database (or a physical database to itself?)

Yes I will need to do more testing. So far I have only been comparing
two PA projects but comparing databases was the next step (maybe I can
also find the reason why sometimes two identical columns are reported
as different. There has been a thread regarding this on the PA forum).


Right. I can give you a hint there: it often has to do with meaningless mutations to data types, such as DECIMAL vs. NUMERIC. Other problem areas involve JDBC drivers reporting different precision values for DATE and TIME (again, these differences are meaningless).

The doColumnDatatypesDiffer() method was my attempt to smooth some of these things out, however I now believe that approach is too weak to quell all spurious differences. It does equate DECIMAL with NUMERIC, which helps quite a bit, but there are a lot of special cases which can actually be significant on one platform and meaningless on the next.

If you take a look at it, think big. As much as I try to avoid unnecessary complexity, I think we a more sophisticated approach to this problem, and I think it will (unfortunately) involve a lot of special case configuration per database platform.
 
I was wondering if the algorithm used to compare the list of objects
could be made more compact. I was thinking about putting the objects
into two maps where the key is the UUID. While traversing one map, the
key is looked up in the other. If it's found, the two objects are
compared. If it's not found it's added as DiffType.LEFT_ONLY. The the
object is removed from the map. Anything that's left in the "right"
map is then added as DiffType.RIGHT_ONLY

But for the time being (and until I have tested and understood the
current approach) I'll stick to the current iterator approach.

The current approach is pretty standard (it's the same basic technique used in line-by-line text comparison programs like Unix diff(1), and the database "merge" join strategy). Again, the issue with the alternative you suggest (which is essentially a database "hash join") is that I don't think it could possibly work when comparing two physical databases to each other: those have to match based only on names. The UUIDs are insignificant.
 

> Have a great weekend.
It's gonna a be very rainy and unfriendly weather this weekend, so I
guess I have some time for PA improvements ;)


Sorry to hear that! We're expecting relatively good weather in Toronto, so I'll have a chance to get some fresh air.

Take care,

Jonathan

Thomas Kellerer

unread,
Feb 19, 2010, 5:30:40 PM2/19/10
to architect-...@googlegroups.com
Hi Jonathan,

> I guess one other thing for that list would be the interaction between
> adding and removing tables and creating and dropping FK constraints on
> those tables. Clearly, order matters.

Yes, that was on my list of things to check.



> Right. I can give you a hint there: it often has to do with meaningless
> mutations to data types, such as DECIMAL vs. NUMERIC. Other problem
> areas involve JDBC drivers reporting different precision values for DATE
> and TIME (again, these differences are meaningless).
>
> The doColumnDatatypesDiffer() method was my attempt to smooth some of
> these things out, however I now believe that approach is too weak to
> quell all spurious differences. It does equate DECIMAL with NUMERIC,
> which helps quite a bit, but there are a lot of special cases which can
> actually be significant on one platform and meaningless on the next.

I have also seen this with VARCHAR columns, so there might be more to this...

> The current approach is pretty standard (it's the same basic technique
> used in line-by-line text comparison programs like Unix diff(1), and the
> database "merge" join strategy). Again, the issue with the alternative
> you suggest (which is essentially a database "hash join") is that I
> don't think it could possibly work when comparing two physical databases
> to each other: those have to match based only on names. The UUIDs are
> insignificant.

A right, when comparing databases the UUID wouldn't be there. I didn't think about that.
That makes my current solution worthless for that scenario as well.

I guess we'll have to distinguish between Model vs. Model comparison and comparisons where a "real" DB is involved then.

I'll see if I can change the logic depending on the comparison type. For tables this is quite easy as far as I can tell either sort by name or sort by UUID. The rest of the algorithm should still work then.

It will be more tricky to detect in the SQLRelationshipComparator though.

Regards
Thomas

Thomas Kellerer

unread,
Feb 20, 2010, 11:27:45 AM2/20/10
to architect-...@googlegroups.com
Hi Joey,

> I am hesitant about your suggestion of using an "old" object field in
> the DiffChunk class, because of the way I have implemented my changes.
> I have created a DiffInfo class which will be used in the DiffChunk as
> the data field, as opposed to SQLObject (DiffChunk<DiffInfo>). This
> data field contains a representation of the SQLObject (Strings for its
> name and type), and includes a list of property changes.
>
> Since you seem to only need the old name of the object for your
> purposes, not a reference to it, perhaps you could implement a String
> field in DiffChunk, which would be able to be used more generally. For
> example, if you called it originalName,

I thought about this a bit more, and I think it is necessary to keep the reference to the complete SQLObject in the DiffChunk. Otherwise the DDL generators won't be able to create a correct SQL for the change.

To rename a table, it might be necessary to have the complete schema and catalog information to generate a fully qualified name for the old table.

The same might be true for columns, indexes or foreign keys. Some database can only rename a column if the column's type is also specified so for that we will always need a full reference to the column object.

So I think DiffChunk should keep the "data" reference and (in case of a rename) a reference to the original definition.
I don't mind using set/getOriginalData() in DiffChunk though ...


Regards
Thomas

Jonathan Fuerth

unread,
Feb 20, 2010, 11:37:46 AM2/20/10
to architect-...@googlegroups.com
On Friday, February 19, 2010, Thomas Kellerer

<google...@sql-workbench.net> wrote:
> The doColumnDatatypesDiffer() method was my attempt to smooth some of
> these things out, however I now believe that approach is too weak to
> quell all spurious differences. It does equate DECIMAL with NUMERIC,
> which helps quite a bit, but there are a lot of special cases which can
> actually be significant on one platform and meaningless on the next.
>
>
> I have also seen this with VARCHAR columns, so there might be more to this...

Yikes! That does sound worse than I imagined.

> A right, when comparing databases the UUID wouldn't be there. I didn't think about that.
> That makes my current solution worthless for that scenario as well.
>
> I guess we'll have to distinguish between Model vs. Model comparison and comparisons where a "real" DB is involved then.

I hope not.

What about adding a subroutine within the differ that does a UUID
lookup /after/ it determines that an object (table or column) is not
the same on both sides? In the physical database case, this will not
cause problems because the uuid lookup will always fail. But in the
playpen-to-playpen case, it should reliably detect rename operations.

Do you think this could work?

-Jonathan

Thomas Kellerer

unread,
Feb 20, 2010, 12:11:40 PM2/20/10
to architect-...@googlegroups.com
Hi Jonathan,

Jonathan Fuerth wrote on 20.02.2010 17:37:
>> The doColumnDatatypesDiffer() method was my attempt to smooth some of
>> these things out, however I now believe that approach is too weak to
>> quell all spurious differences. It does equate DECIMAL with NUMERIC,
>> which helps quite a bit, but there are a lot of special cases which can
>> actually be significant on one platform and meaningless on the next.
>>
>>
>> I have also seen this with VARCHAR columns, so there might be more to this...
>
> Yikes! That does sound worse than I imagined.

The problem is, that I can't reliably reproduce it.

But I remember it happening with Oracle, so it _could_ be caused by a VARCHAR2 vs. VARCHAR "thing"

I had a look a ArchitectUtils.columnsDiffer() and I have seen some potential pitfalls with the current implementation (that I have run into for my WbSchemaDiff in SQL Workbench already ;).

There are more datatypes than just Types.DATE or Types.INTEGER that don't need scale or precision (the various integer flavors, clobs, blobs and so on). And there are some datatypes than only need precision (VARCHAR, CHAR, ...).

So I created two methods needsScale() and needsPrecision() to be used instead of targetType == Types.DATE to makes this more robust in case a DBMS actually returns something for precision e.g. on a varchar column (I think HSQLDB does/did that)


>> I guess we'll have to distinguish between Model vs. Model comparison and comparisons where a "real" DB is involved then.
>
> I hope not.
>
> What about adding a subroutine within the differ that does a UUID
> lookup /after/ it determines that an object (table or column) is not
> the same on both sides?

That's where I started, but the problem is that in order to detect "equal" objects, the iterators must be ordered by the UUID the position in the list defines if two objects are compared (at least that's how I understand the algorithm)

Currently I do use a "useUUID" flag to use a different ordering of the lists. SQLRelationshipComparator also needs to distinguish UUID "identity" and name "idenitity" as it checks if both constraints reference the same tables. If one of the tables has been renamed, it will report the FK as changed (which is not true). That could be a place where the flag is not needed and simply first check the names of the tables then, if they are not the same, check the UUIDs of the tables.

The main reason for having the flag, was to make sure it's not breaking existing code

Currently all existing unit test (for the diff) run successfully (and of course the new ones that use UUID based comparison).

As far as I can tell, the unit tests do cover most of the cases, but only with a limited number of tables.

As I currently have a medium sized model (~100 tables) that is constantly changing this is a good playground for testing the changes. So I'll play around with my current implementation in the next week to see how stable it is.


Regards
Thomas


Joey Silva

unread,
Feb 22, 2010, 1:04:17 PM2/22/10
to Architect Developers
Hey Thomas,

I had to change my original approach, but I finished my changes to
CompareSQL and related UI classes. Hopefully I merged your changes in
correctly.

I don't know if you're still working on this, but if you are, feel
free to ask me any questions you have regarding what I've changed/
added.

-Joey

Thomas Kellerer

unread,
Feb 22, 2010, 3:13:48 PM2/22/10
to architect-...@googlegroups.com
Hi Joey,

wow, quite a change. It will take a while to merge my changes back in.

I have two questions though:

- What's the intended difference between DiffType.MODIFIED and the new DiffType.SQL_MODIFIED?


- And there is one thing that I was already wondering about (and which is still the case)

In CompareSQL the basic algorithm is:

if compare(source, target) < 0
add with "left only"
source = next object from source list;

if compare(source, target) > 0
add with "right only"
target = next object from target list;

if compare(source, target) = 0
...

What I am wondering about is whether the side effect that each if can compare a different pair of objects is intended.

When trying to discover name changes this gives me a major hardache (as some objects would appear twice in the list).

So I basically changed the algorithm to


int compareResult = compare(source, target)

if (compareResult < 0)
....

if (compareResult > 0)
...

then after all compares are done, get the "next objects"

Regards
Thomas

Joey Silva

unread,
Feb 22, 2010, 3:50:25 PM2/22/10
to Architect Developers

On Feb 22, 3:13 pm, Thomas Kellerer <google-gro...@sql-workbench.net>
wrote:

> - What's the intended difference between DiffType.MODIFIED and the new DiffType.SQL_MODIFIED?

I used this for cases where I can show changes between properties of
objects in English, but not in SQL script.
For example, I will say a column's type is DiffType.MODIFIED if its
auto increment changes or a remark changes because this difference
won't have SQL script code generated, though.
However, if a column had its precision or type changed, that is
DiffType.SQL_MODIFIED since there is code in place to generate SQL
script for that difference.

This way, if you change a column's remark, for example, it won't
generate SQL code to modify the column, since that will not make any
actual changes.

I think, but am not sure that most if not all of the stuff I am
indicating as just DiffType.MODIFIED could have meaningful SQL script
generated, but it is not currently implemented in CompareDM's SQL
output method. So haivng the two classifications may be unneccessary,
since it might be possible to have all kinds of modifications generate
SQL script. However, I'm a little strained for time, so I have done it
this way.

> - And there is one thing that I was already wondering about (and which is still the case)

> ...

Sorry, I don't know if that is intended or not.

Jonathan Fuerth

unread,
Feb 22, 2010, 4:03:44 PM2/22/10
to architect-...@googlegroups.com
On Mon, Feb 22, 2010 at 3:50 PM, Joey Silva <silva.jo...@gmail.com> wrote:

This way, if you change  a column's remark, for example, it won't
generate SQL code to modify the column, since that will not make any
actual changes.

I think, but am not sure that most if not all of the stuff I am
indicating as just DiffType.MODIFIED could have meaningful SQL script
generated, but it is not currently implemented in CompareDM's SQL
output method. So haivng the two classifications may be unneccessary,
since it might be possible to have all kinds of modifications generate
SQL script. However, I'm a little strained for time, so I have done it
this way.

Remarks actually do cause changes that should be reflected in the DDL script: we do forward-engineer remarks, and we ought to update them when they change. I understand it's outside the scope of your current change, but changing remarks is not hard to do--do you think you could squeeze this in?

Maybe a better example of things that don't affect DDL would be colour and line style (solid vs. dashed).
 
-Jonathan

Thomas Kellerer

unread,
Feb 22, 2010, 4:19:58 PM2/22/10
to architect-...@googlegroups.com
Jonathan Fuerth wrote on 22.02.2010 22:03:
> Remarks actually do cause changes that should be reflected in the DDL
> script: we do forward-engineer remarks, and we ought to update them when
> they change. I understand it's outside the scope of your current change,
> but changing remarks is not hard to do--do you think you could squeeze
> this in?

The remarks generation is still in there as far as I can tell.
If the diff type is SQL_MODIFIED CompareDMFormatter calls DDLGenerator.modifyColumn() or modifyComment()


I wonder if the new "property change detection" would include the name/phyiscal name of an object. If that is the case then when using my UUID based identity check will include a property change for the name as well. I will need to test this.


As a side note:
The longer I work with CompareSQL and CompareDMFormatter, the more I think it would be a good idea to pass the information _what_ has changed on to the generator as well.

A change like e.g. NOT NULL to NULL can currently only be reflected by repeating the complete (new) column definition (because modifyColumn() is called - but without the information about the old column).
If the individual changes were passed as well, the DDL generator could instead create a DROP CONSTRAINT which is more efficient than doing an ALTER COLUMN.

Regards
Thomas

Thomas Kellerer

unread,
Feb 22, 2010, 4:36:08 PM2/22/10
to architect-...@googlegroups.com
> As a side note:
> The longer I work with CompareSQL and CompareDMFormatter, the more I
> think it would be a good idea to pass the information _what_ has changed
> on to the generator as well.
>
> A change like e.g. NOT NULL to NULL can currently only be reflected by
> repeating the complete (new) column definition (because modifyColumn()
> is called - but without the information about the old column).
> If the individual changes were passed as well, the DDL generator could
> instead create a DROP CONSTRAINT which is more efficient than doing an
> ALTER COLUMN.

One thing that goes along with this problem is, what to do if a column is renamed, it's remark is changed and it's datatype is changed.

Currently these are three different DiffType values and would/should result in three different methods to be called in the DDLGenerator, but as every changed object is only added once to the diff list, only one change will be "recorded".

Maybe DiffChunk should have a list of DiffType values instead of a single one?
Then CompareDMFormatter could generate multiple calls for each DiffType

Regards
Thomas

Jonathan Fuerth

unread,
Feb 22, 2010, 6:02:01 PM2/22/10
to architect-...@googlegroups.com
On Mon, Feb 22, 2010 at 4:36 PM, Thomas Kellerer <google...@sql-workbench.net> wrote:
One thing that goes along with this problem is, what to do if a column is renamed, it's remark is changed and it's datatype is changed.

Currently these are three different DiffType values and would/should result in three different methods to be called in the DDLGenerator, but as every changed object is only added once to the diff list, only one change will be "recorded".

Maybe DiffChunk should have a list of DiffType values instead of a single one?
Then CompareDMFormatter could generate multiple calls for each DiffType


That sounds like a superior solution to me.

Joey, how would such a change affect your recent work?

-Jonathan

Joey Silva

unread,
Feb 22, 2010, 6:20:42 PM2/22/10
to Architect Developers
On Mon, Feb 22, 2010 at 4:36 PM, Thomas Kellerer <google-gro...@sql-

workbench.net> wrote:
> One thing that goes along with this problem is, what to do if a column is
> renamed, it's remark is changed and it's datatype is changed.

> Currently these are three different DiffType values and would/should result
> in three different methods to be called in the DDLGenerator, but as every
> changed object is only added once to the diff list, only one change will be
> "recorded".

Sounds like a good idea. I have a suggestion for an alternate means,
in case you like it better: keep the main DiffType field, and add a
list of modification types as a new field. (It would be a new enum
type, with constants for remarks, name, type (etc) changes.

On Feb 22, 6:02 pm, Jonathan Fuerth <fue...@sqlpower.ca> wrote:

> Joey, how would such a change affect your recent work?

It would be easy to merge.

Thomas Kellerer

unread,
Feb 24, 2010, 3:19:39 AM2/24/10
to architect-...@googlegroups.com
Joey Silva, 23.02.2010 00:20:

>> Currently these are three different DiffType values and would/should result
>> in three different methods to be called in the DDLGenerator, but as every
>> changed object is only added once to the diff list, only one change will be
>> "recorded".
>
> Sounds like a good idea. I have a suggestion for an alternate means,
> in case you like it better: keep the main DiffType field, and add a
> list of modification types as a new field. (It would be a new enum
> type, with constants for remarks, name, type (etc) changes.

What you mean is essentially to track each property that has changed.

I think this is a good idea because e.g. for Liquibase changing NOT NULL to NULL should result in a <dropNotNullConstraint> tag instead of repeating the whole column definition. Currently this is not possible (because there is no way for the generator to know _what_ has changed when modifyColumn() is called.

Why not simply store the list of PropertyChange objects that we already have in the DiffChunk?


Btw: my changes to detect a rename seem be pretty stable now. Any objections if I commit those changes? It will make life a bit easier for me, because I don't have to merge sources each time you commit something ;)

Regards
Thomas

Jonathan Fuerth

unread,
Feb 24, 2010, 11:03:56 AM2/24/10
to architect-...@googlegroups.com
On Wed, Feb 24, 2010 at 3:19 AM, Thomas Kellerer <google...@sql-workbench.net> wrote:
I think this is a good idea because e.g. for Liquibase changing NOT NULL to NULL should result in a <dropNotNullConstraint> tag instead of repeating the whole column definition. Currently this is not possible (because there is no way for the generator to know _what_ has changed when modifyColumn() is called.

Why not simply store the list of PropertyChange objects that we already have in the DiffChunk?

I asked Joey that too. I think the answer is that we wanted to be able to reuse the same diff API to show differences between revisions on the enterprise server. When performing those diffs, we have information about all the property changes, but no references to the actual objects in question (i.e. we don't diff on live SQLObjects--we compare repository revisions directly). So we wouldn't be able to fill in the all-important source field on the property change events.

Joey, feel free to correct me. :)


Btw: my changes to detect a rename seem be pretty stable now. Any objections if I commit those changes? It will make life a bit easier for me, because I don't have to merge sources each time you commit something ;)

 Yes, please do.

-Jonathan

Thomas Kellerer

unread,
Feb 24, 2010, 11:08:47 AM2/24/10
to architect-...@googlegroups.com
Hi Jonathan,

> Why not simply store the list of PropertyChange objects that we
> already have in the DiffChunk?
>
>
> I asked Joey that too. I think the answer is that we wanted to be able
> to reuse the same diff API to show differences between revisions on the
> enterprise server. When performing those diffs, we have information
> about all the property changes, but no references to the actual objects
> in question (i.e. we don't diff on live SQLObjects--we compare
> repository revisions directly). So we wouldn't be able to fill in the
> all-important source field on the property change events.

Fine with me. I just thought we could save some CPU cycles as we already have that list

> Btw: my changes to detect a rename seem be pretty stable now. Any
> objections if I commit those changes? It will make life a bit easier
> for me, because I don't have to merge sources each time you commit
> something ;)
>
>
> Yes, please do.

OK, fine. I'll do it later in the evening

Regards
Thomas

Joey Silva

unread,
Feb 24, 2010, 11:20:22 AM2/24/10
to architect-...@googlegroups.com
No, what Thomas is saying would work, I think. I misunderstood you when you first said what you wanted, and I thought you were going to implement a list of DiffType instead of a single DiffType.

I think you are right. Iterating through the already constructed list of property changes is probably the better solution.

--
You received this message because you are subscribed to the Google Groups "Architect Developers" group.
To post to this group, send email to architect-...@googlegroups.com.
To unsubscribe from this group, send email to architect-develo...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/architect-developers?hl=en.

Thomas Kellerer

unread,
Feb 24, 2010, 5:05:38 PM2/24/10
to Architect Developers
Hi Jonathan,


> > Btw: my changes to detect a rename seem be pretty stable now. Any objections
> > if I commit those changes? It will make life a bit easier for me, because I
> > don't have to merge sources each time you commit something ;)
>
>  Yes, please do.

OK done.

I also applied a small fix to the display of the english
descriptions.
When clicking on the scrollbar buttons (up/down) the display was only
scrolled by 1 pixel, which is clearly too small ;) Now the buttons
will scroll approx. one line of text.

There is one formatting problem with the english descriptions.
If more than one column is reported for a table the second and third
are indented too much. You can see a sample screenshot here:

http://www.sql-workbench.net/compare-indent.png

Regards
Thomas

Thomas Kellerer

unread,
Feb 24, 2010, 5:11:39 PM2/24/10
to architect-...@googlegroups.com
One thing I forgot to mention:

I added a new checkbox to the CompareDMPanel: "Include Indexes"

As far as I can tell CompareSQL handles index diff quite nicely so I decided that it will be a good thing to let the user choose if he/she wants to include them in the model diff.

One thing that is missing, is that the selection for index comparison is currently not stored in the CompareDMSettings.
(Btw: the selected schema does not seem to be stored as well when comparing against an Oracle database)

Feel free to remove that option if you don't think index comparison is stable enough.

Regards
Thomas

Jonathan Fuerth

unread,
Feb 24, 2010, 5:20:42 PM2/24/10
to architect-...@googlegroups.com
On Wed, Feb 24, 2010 at 5:11 PM, Thomas Kellerer <google...@sql-workbench.net> wrote:
One thing I forgot to mention:

I added a new checkbox to the CompareDMPanel: "Include Indexes"

As far as I can tell CompareSQL handles index diff quite nicely so I decided that it will be a good thing to let the user choose if he/she wants to include them in the model diff.

Ok, great. I'm very happy to see that gap filled in!
 
One thing that is missing, is that the selection for index comparison is currently not stored in the CompareDMSettings.

Honestly, if it works, I'd rather not even have the option. Just do it all the time.
 
(Btw: the selected schema does not seem to be stored as well when comparing against an Oracle database)

Right. I believe it actually does store the schema (and catalog) but due to some sloppy thinking around the worker threads that actually populate those dialogs, the defaults get applied /before/ the worker threads populate the combo boxes. Obviously, the correct approach would be to apply the defaults in the cleanup phase of the workers that populate the combo boxes.

-Jonathan
Reply all
Reply to author
Forward
0 new messages