Tomasz Judycki's todo list

[thomas]

Last week Tomasz Judycki posted a to-do list of issues he would like to work on. I'm adding in more details towards the specifics here.
The original post in the forums is at http://www.sqlpower.ca/forum/posts/list/2903.page


1.1. Adding foreign key constraint must also create index on foreign key field

CREATE INDEX orders_fk1 ON orders (customer_id); -- need to be done
ALTER TABLE orders ADD CONSTRAINT customers_orders_fk FOREIGN KEY (customer_id) REFERENCES customers (customer_id); -- done already by FE

In my forum post I made some comments on this issue. The main consideration I want to give to this is for existing data models that already have the index created and this change would cause the indexes to be doubled for some users. I also may be mistaken, it's been a while since I've done a lot with PostgreSQL, but I don't remember the index being mandatory to create a foreign key constraint. If the index isn't mandatory in PostgreSQL to create a foreign key then I don't think we should make the creation of the index forced when forward engineering. I think one way this feature could be added that could help more users than just for PostgreSQL is to have an option in the configuration that will create index objects when the foreign keys get created.

Tomasz, I'm interested in your thoughts and insight into this issue. If you were going to look at how to add in indexes when foreign keys are created there is an event system in Architect that you can use.
In Architect there is an object tree that makes up the model that drives the UI and functionality in Architect. You can find the root of the tree in the ArchitectProject class and the tree is made up of different objects including SQLDatabase, SQLTable, SQLColumn, etc. All of the objects in the tree implement SPObject which provides tree methods like getChildren and includes an event system. This event system in the SPObject is what you would look into connecting with. When a foreign key is created an object is added to each of the tables to make the relationship, see the SQLRelationship and SQLImportedKey classes. If you made a listener that added an index to the foreign key table when the relationship was added it should give the index you are looking for. It would also give the ability to turn this feature off, by not including the listener. By creating the index when the relationship is created we would also have control over manipulating parts of the index, like the name, before forward engineering. The listener option also lets you add it after reverse engineering so you don't create extra indexes when reverse engineering.
Mind you this listener idea is just the first idea I have for a solution, there may be better ones.



1.2. If a field is INTEGER and auto-increment = yes then generate it as SERIAL

instead of:

CREATE SEQUENCE customers_customer_id_seq;
CREATE TABLE customers (
customer_id INTEGER NOT NULL DEFAULT nextval('customers_customer_id_seq'),
...
);
ALTER SEQUENCE customers_customer_id_seq OWNED BY customers.customer_id;

generate:

CREATE TABLE customers (
customer_id SERIAL NOT NULL,
...

As noted in the forum post this is in our bug database as http://trillian.sqlpower.ca/bugzilla/show_bug.cgi?id=1557. I think the forum post is incorrect in saying that this bug seems to be fixed because bug 1557 is still open at the time of writing this (it's status is new as opposed to resolved). If you work on this issue the hint I can give is to look at the commit http://code.google.com/p/power-architect/source/detail?r=2080 which, according to the bug, is the change that broke the SERIAL part. In particular the PostgresDDLGenerator class is the class you would be looking to fix. The change unfortunately fixes more than one issue, which we try to avoid to reduce confusion, and the change is meant to fix "generating two default values whenever the column is marked as serial". Unfortunately this commit message also doesn't refer to a bug number to relate the fix so we don't have the steps to reproduce this immediately.


1.3. Correctly handle 0-or-1 to many relationship

Let say you have a customers table and want to add customer_types table with non identifying relation to customers. Not every customer must be linked to a type so in relation properties you mark cardinality for PK table customer_types as "Zero or one" and allow nulls in customers.customer_type_id:

This is a general shortcoming of our validation mechanism. In the older versions of Architect there was validation during forward engineering but it was located in many different locations so it was difficult to update the validation and configure it for different uses. Eventually we came up with the idea of having validation that the user could manage and we could have specific validation for different database platforms, modelling styles, etc. This validation became our critics in the code base and is contained in the packages ddl.critic and ddl.critic.impl. (At one point we thought it would be neat to have users be able to write their own critics in some type of scripting language.) You will find the critic message is given by the RelationshipMappingTypeCritic and could be solved by improving the logic around when to give the criticism.



I know there are more items on this to-do list but hopefully this gives you some insight into the structure of Architect.
Tomasz, if there's a particular item or two on the list you are going to tackle first and want more details on that you can follow up with that and we can discuss the details.

Thomas O'Brien

[Tomasz Judycki]

Hi Thomas,

> > 1.1. Adding foreign key constraint must also create index
> > on foreign key field
> >
>

> I also may be mistaken, it's been a while since I've done a lot
> with PostgreSQL, but I don't remember the index being mandatory
> to create a foreign key constraint.

No, index isn't mandatory but it's recommended especially when the referenced table contains a lot of records or when referenced/referencing columns are changed frequently.

I agree that automated index creation should be implemented in user interface and of course must be optional. I'll try to play with listeners following your suggestions. However it would be nice to have a kind of logical connection between foreign key and corresponding index that was automatically created. In case of relationship removal a foreign key is removed and index should be removed as well. If we don't have such connection then we have to guess which index is to be removed. We may use name pattern for such guess so may be it's not a big problem.

> > 1.2. If a field is INTEGER and auto-increment = yes then
> > generate it as SERIAL
>

> If you work on this issue the hint I can give is to look at the commit
> http://code.google.com/p/power-architect/source/detail?r=2080 which,
> according to the bug, is the change that broke the SERIAL part. In particular
> the PostgresDDLGenerator class is the class you would be looking to fix.
> The change unfortunately fixes more than one issue, which we try to avoid
> to reduce confusion, and the change is meant to fix "generating two default
> values whenever the column is marked as serial".

I checked that and it seems to me that you are doing it wrong. I know how to make current version of Architect to generate two default values for column marked as serial but instead of fixing it I will try to suggest complete solution that will properly handle serial's in forward/backward engineering and data model comparing.

> > 1.3. Correctly handle 0-or-1 to many relationship
>

> This is a general shortcoming of our validation mechanism. [...]

> This validation became our critics in the code base and is contained
> in the packages ddl.critic and ddl.critic.impl.

> [...] You will find the critic message is given by the

> RelationshipMappingTypeCritic and could be solved by improving
> the logic around when to give the criticism.

I've found that already and corrected that particular validation.

> 1.6. There is also some inconsistency with comments/remarks between RE and CMD

CompareSQL, lines 255 and 258: sourceTable and targetTable should be replaced - this is a simple mistake. There's another issue concerning trailing spaces: it seems that RE trims spaces while CMD does not.

Could you please answer some questions:

1. How to add a completely new critic? I've created a new class and added it into STARTING_CRITICS in CriticManager but it's not enough.
2. How to mark that such critic class should be applied to particular database only, e.g. to PostgreSQL?
3. Could you briefly explain how Power Architect retrieve schema definition from database while

Tomasz Judycki

[Tomasz Judycki]

Last part was missing, should be:

3. Could you briefly explain how Power Architect retrieve schema definition from database while forward engineering and model comparing?

Tomasz Judycki

[thomas]

No, index isn't mandatory but it's recommended especially when the referenced table contains a lot of records or when referenced/referencing columns are changed frequently.

I agree that automated index creation should be implemented in user interface and of course must be optional. I'll try to play with listeners following your suggestions. However it would be nice to have a kind of logical connection between foreign key and corresponding index that was automatically created. In case of relationship removal a foreign key is removed and index should be removed as well. If we don't have such connection then we have to guess which index is to be removed. We may use name pattern for such guess so may be it's not a big problem.

One way you could do this would be to extend SQLIndex and add a new property to that class which points to the relationship. Extending SQLIndex should let you have your direct reference to the relationship while not needing a completely new object on the SQLTable.

1. How to add a completely new critic? I've created a new class and added it into STARTING_CRITICS in CriticManager but it's not enough.

I just added a new critic to check the steps. What I did was:

• Create a new class that extends CriticAndSettings in the critic.impl package.
• Add the critic to the STARTING_CRITICS in the CriticManager.

The constructor of my critic looks like:
    public TestCritic() {
        super(StarterPlatformTypes.GENERIC.getName(), "Test Critic");
    }
So in the Validation Manager the critic appeared at the end of the "Generic" list.

For my test I didn't actually implement anything in the criticize method so it won't generate any criticisms.
 

2. How to mark that such critic class should be applied to particular database only, e.g. to PostgreSQL?

Take a look at the CriticAndSettings.StarterPlatformTypes.isAssociated method. You will see the calling method CriticManager.criticize uses the name provided to the critic to decide if it should run for a specific database.
While some of the grouping of the critics may seem odd we wanted to leave room for new critics and groupings. One enhancement was to have users be able to create and add critics without changing Architect code. The idea we had was users could create a critic in a new project, create a jar containing their critic and then add that critic, through some config file, to Architect. We never got to creating the config file so users could add critics later (it's also why the list is called STARTING_CRITICS for when Architect is first run without configuration).
 

3. Could you briefly explain how Power Architect retrieve schema definition from database while forward engineering and model comparing?

I'm not sure if I understand what you are asking. What information are you trying to get about the schema?

Thomas O'Brien
 

[Tomasz Judycki]

3. Could you briefly explain how Power Architect retrieve schema definition from database while forward engineering and model comparing?

I'm not sure if I understand what you are asking. What information are you trying to get about the schema?

This is very basic question. To make backward (not forward) engineering Architect should open connection with target database and retrieve definition of tables, fields, indexes etc. I can't find it in source code because I'm not familiar with Swing. Yet.

Regards,

Tomasz Judycki

Textus Virtualis
Al. Wilanowska 313
02-665 Warszawa
tel/fax (48 22) 879 82 00
http://www.tv.com.pl

[thomas]

I think where you may want to start looking is in the SQL Power Library either in the ca.sqlpower.sql or ca.sqlpower.sql.wrapper package.

To give some direction lets say you have a database object, a SQLDatabase, and you want to get the schema. Well getSchemaByName for SQLDatabase has its first line as populate() which loads information from the database. The populate method calls the populateImpl in SQLDatabase which uses SQLSchema.fetchSchemas. The fetchSchemas method uses the given DatabaseMetaData to get a connection to the database and makes different requests.

If you're looking for something Swing related you could look at the CompareDMPanel class. This class creates the panel that you get when you press the compare DM button. There is an inner class to this class called StartCompareAction whose method actionPerformed is what gets invoked when you press the start button in the compare DM panel.

Hopefully this helps answer your question.

Thomas O'Brien

[Tomasz Judycki]

Thomas, thanks for all your help. I'm almost done with most of my
changes but I've just noticed some serious problems with index
comparison. Compare Data Model between current model and physical
database (PostgreSQL) generates script containing e.g.:
CREATE INDEX customers_pk ON customers.public.customers ( customer_id );
...
DROP INDEX customers.public.customers_pk CASCADE;

It creates an index and then drop it. Do you feel that index comparison
works properly? Nobody ever reported any problems?

By the way: I've asked about adding new critics because I tried to do it
but it didn't work. From your answer I got to know that I'm on a right
way so I've double check it and found the problem: new critics appears
in a new model immediately but it would newer appear in an existing
model because it must be included in critic-manager settings in
.architect file. I've added manually:
<critic-settings
class="ca.sqlpower.architect.ddl.critic.impl.RelationshipCardinalityCritic"
severity="ERROR"/>
to my model file and it works!

[Thomas O'Brien]

Hi Tomasz,

I'm glad things are going well for you.

I don't remember anyone reporting a problem in the past where an index was created then deleted. I took a look at our bug list but couldn't find anything related to indexes and compare DM. From my memory I thought the compare DM feature was working fairly well, but there's always room for unusual cases in all software.

If your compare DM was creating and then dropping an index that is unexpected behaviour.
Took a quick look at the ca.sqlpower.architect.diff package, specifically at the SQLIndexComparator. I'm not sure about the details about the change but the behaviour you see may be caused by this comparator being set to true for comparing by UUID. This property gets set in the swingui class CompareDMPanel.StartCompareAction.actionPerformed (find the useUUID variable). Based on comments it looks like that part may have been added for Liquibase XML output.

I'm glad you were able to get your critic working.

Thomas

--
You received this message because you are subscribed to the Google Groups "Architect Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to architect-developers+unsub...@googlegroups.com.
To post to this group, send email to architect-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/architect-developers.
For more options, visit https://groups.google.com/groups/opt_out.

--
Thomas O'Brien
www.sqlpower.ca
SQL Power Group Inc.  |  4950 Yonge St.  |  Suite 1900  |  Toronto,
Ontario M2N 6K1  |  416.221.4220 x224

[Tomasz Judycki]

Hi,

I've checked CompareSQL.generateIndexDiffs() and SQLIndexComparator already and I can't understand how is it possible to compare indexes using attributes like uniqueness and ignoring table name. In my opinion index should be compared table by table and not database wide. May be I'm missing something...

Have a look at attached files:
b_new.architect and b_old.architect - two, similar models, differences only in indexes
b_old_2_new.jpg - this is how I set CDM
b_old_2_new.txt - this is result of comparison

I've created models and compared them using original, untouched SQL Power Architect 1.0.6.

While studying sources I've found this:

CompareSQL, line 595:

        if (targetIter.hasNext()) {
                    targetInd = targetIter.next();
                } else {
                }

It seems like else block should contain:
                    targetContinue = false;
but adding this line is not enough to correct CDM.

Regards,

Tomasz Judycki

Textus Virtualis
Al. Wilanowska 313
02-665 Warszawa
tel/fax (48 22) 879 82 00
http://www.tv.com.pl

W dniu 2013-09-18 16:28, Thomas O'Brien pisze:

To unsubscribe from this group and stop receiving emails from it, send an email to architect-develo...@googlegroups.com.
To post to this group, send email to architect-...@googlegroups.com.

Visit this group at http://groups.google.com/group/architect-developers.
For more options, visit https://groups.google.com/groups/opt_out.

--
Thomas O'Brien
www.sqlpower.ca
SQL Power Group Inc.  |  4950 Yonge St.  |  Suite 1900  |  Toronto,
Ontario M2N 6K1  |  416.221.4220 x224

--
You received this message because you are subscribed to the Google Groups "Architect Developers" group.

To unsubscribe from this group and stop receiving emails from it, send an email to architect-develo...@googlegroups.com.
To post to this group, send email to architect-...@googlegroups.com.

[Tomasz Judycki]

Hi again,

A lot of things happened since my last emails but now I'm working on Power Architect again. I've already written a completely new index diff generator as exiting doesn't work properly. I'm almost done with my improvements but now I've got a strange problem in reverse engineering with zero-or-one to many relationship. Have a look at example (this is PostgreSQL syntax):

CREATE TABLE company (
  company_id SERIAL NOT NULL,
  company_name VARCHAR NOT NULL,
  CONSTRAINT company_pk PRIMARY KEY (company_id)
);

CREATE TABLE person (
  person_id SERIAL NOT NULL,
  company_id INTEGER NULL,
  CONSTRAINT person_pk PRIMARY KEY (person_id)
);

CREATE INDEX person_fk1 ON person (company_id);

ALTER TABLE person ADD CONSTRAINT company_person_fk
  FOREIGN KEY (company_id) REFERENCES company (company_id);

Relation company_person_fk is zero-or-one to many: a person may be linked to company or not, as person.company_id is nullable. I modified reverse engineering to recognise such relation as zero-or-one (pk cardinality) but person.company_id is still marked not null in PlayPen and obviously in forward engineering as well. In other words: reverse engineering + forward engineering will change person.company_id from nullable to not null. It happens in SQLColumn.assignTypes(), in line 679 (Revision 2005):
    type.setMyNullability(null);
while working on company.company_id and not on person.company_id. It seems like exported/imported keys share the same userDefinedSQLType object. Is it done on purpose? Is it a side effect? Is it an error? I've commented that out and it seems working fine but I'm not sure if it's a good solution.

Regards,

Tomasz Judycki

Textus Virtualis
Al. Wilanowska 313
02-665 Warszawa
tel/fax (48 22) 879 82 00
http://www.tv.com.pl

W dniu 2013-09-18 18:42, Tomasz Judycki pisze:

[jon...@tom.com]

/src/main/java/ca/sqlpower/architect/ddl/critic/impl/RelationshipMappingTypeCritic.java

line 48

         if (!(so instanceof ColumnMapping)) return Collections.emptyList();

         ColumnMapping cm = (ColumnMapping) so;

         List<Criticism> criticisms = new ArrayList<Criticism>();

         if (ArchitectUtils.columnsDiffer(cm.getFkColumn(), cm.getPkColumn())) {

             final SQLColumn parentColumn = cm.getPkColumn();

             final SQLTable parentTable = parentColumn.getParent();

             final SQLColumn childColumn = cm.getFkColumn();

 

But in src/main/java/ca/sqlpower/architect/ArchitectUtils.java line 142 columnsDiffer

     * Checks if the definitions of two columns are materially different.

     * Some data types (for example, DECIMAL and NUMERIC) are essentially

     * the same.  Also, the precision and scale values on DATE columns are

     * not of much consequence, but different databases report different

     * values.

 

So, perhaps function "columnsDiffer" is not suitable for RelationshipMappingTypeCritic

 

jon...@tom.com

 

2014-05-28

 

---

发件人： Tomasz Judycki

发送时间： 2014-03-17  19:07:22

收件人： architect-developers

抄送：

主题： Re: Tomasz Judycki's todo list

[Tomasz Judycki]

Well, I'm not going to handle that problem. I just want to use Architect with PostgreSQL and it seems working well.

Regards,

Tomasz Judycki

Textus Virtualis
Al. Wilanowska 313
02-665 Warszawa
tel/fax (48 22) 879 82 00
http://www.tv.com.pl

W dniu 2014-05-28 16:03, jon...@tom.com pisze:

For more options, visit https://groups.google.com/d/optout.

[Tomasz Judycki]

Hello!

I seems that I'm done with my changes. Do you want me to share it with other developers? I'm using power-architect.googlecode.com/svn/trunk/ - it is still version 1.0.6 while you've published 1.0.7 already.

If you want me to share my improvements then I need some help with implementing configuration settings properly. I put some parameters into pl.ini file however I don't know how to pass parameters into sqlpower_library - I guess this library is used in another projects so I don't want to modify it to much.

Regards,

Tomasz Judycki

Textus Virtualis
Al. Wilanowska 313
02-665 Warszawa
tel/fax (48 22) 879 82 00
http://www.tv.com.pl

W dniu 2014-03-17 12:05, Tomasz Judycki pisze:

For more options, visit https://groups.google.com/d/optout.

[Thomas O'Brien]

Hi Thomasz,

Normally when a new developer wants to share a change we ask them to share a patch of the changes. I'm not sure what IDE you use but with Eclipse you can create a patch by right-clicking on a project and go to team/create patch. When a patch is shared we at SQL Power normally review it to make a decision on adding the change to the trunk of the project. We may also make comments related to our code style guide or issues we see in the change.

If you made changes to the sqlpower_library it is also open source at https://code.google.com/p/sqlpower-library . If you make changes to the library we will review them before adding them to the trunk and would make sure they don't impact the other projects. If you're looking at adding to the pl.ini file you may want to look at the sqlpower_library class PlDotIni which is involved with loading the file.

[Tomasz Judycki]

Hi Thomas,

I'm using Eclipse so I could create a patch as you described. I've got sqlpower_library from repository as well. I've studied PlDotIni already and use it for storing parameters however I'm not sure if it's a proper way. I'd like to add parameters for skipping name qualifications in generated SQL or creating foreign key index along with creating relationship but if I put these parameters into pl.ini they would be located somewhere in PostgreSQL section:

[Database Types_5]
JDBC Driver Class=org.postgresql.Driver
.....
ca.sqlpower.sqlobject.SQLIndex.IndexType_2=RTREE
ca.sqlpower.architect.profile.ProfileFunctionDescriptor_25=serial,serial,4,false,false,false,false,false,false,false,false
skipNameQualification=true
createFkForRelationship=true
ca.sqlpower.sqlobject.SQLTypePhysicalProperties_0=fc731466-848e-411a-82af-e7f5e1d504bb,BYTEA,NOT_APPLICABLE,NOT_APPLICABLE

Would it be correct? Is there a better place to put such parameters?

And again: what about sources of version 1.0.7? Are they available?

Regards,

Tomasz Judycki

Textus Virtualis
Al. Wilanowska 313
02-665 Warszawa
tel/fax (48 22) 879 82 00
http://www.tv.com.pl

W dniu 2014-06-02 22:50, Thomas O'Brien pisze:

[Thomas O'Brien]

Hi Tomasz,

From what you describe it sounds like placing the parameters in the database type would be fine. Normally for parameters in that section we try to have the properties configured through the UI in the JDBC Driver properties window (can be reached from the JDBC Drivers button on the Database Connection Manager). I've looked briefly at other places where we might want to have these properties instead but couldn't find something better.

The source for the 1.0.7 version is the latest code in the trunk of the Architect project so if you updated recently you would be at the 1.0.7 version.

[Tomasz Judycki]

Hi again!

Find out attached patches with my changes to Architect and SQLPower Library. I've used updated "trunk" from http://power-architect.googlecode.com/svn and http://sqlpower-library.googlecode.com/svn

I'm also attaching my pl.ini file to show my configuration of new parameters for PostgreSQL:
useRobustIndexDiff=true
platformDefaultIndexType=BTREE
platformDefaultAscDesc=ASCENDING
ignoreQualifier=true
ignoreFilterCondition=true
skipNameQualification=true
createFkIndexForRelationship=true
fkIndexNamePattern=%s_fk%d

With my changes SQL Power Architect really supports PostgreSQL, uses current versions of Java and is easier to use.

Do you need detailed explanation of my changes? Do you want me to modify something?

By the way: note my new email address.

Regards,

Tomasz Judycki

Atende Medica Sp. z o.o.
Al. Wilanowska 313
02-665 Warszawa
tel/fax (48 22) 879 82 00
http://www.atendemedica.pl 

W dniu 2014-06-03 o 20:55, Thomas O'Brien pisze:

[Tomasz Judycki]

Hi!

Do you plan to review my modifications? If you are too busy (I'd perfectly understand that) then I could publish my patch and ask community to verify it. What do you think about it?

Regards,

Tomasz Judycki

Atende Medica Sp. z o.o.
Al. Wilanowska 313
02-665 Warszawa
tel/fax (48 22) 879 82 00
http://www.atendemedica.pl 

W dniu 2014-12-30 o 15:35, Tomasz Judycki pisze:

[Thomas O'Brien]

Hi Tomasz,

At current I don't have time to review your modifications due to other projects and priorities. If you want to publish your patch to the community that's fine with me.

-Thomas