db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Rick Hillegas (JIRA)" <j...@apache.org>
Subject [jira] Commented: (DERBY-712) Support for sequences
Date Tue, 26 May 2009 19:32:45 GMT

    [ https://issues.apache.org/jira/browse/DERBY-712?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12713202#action_12713202

Rick Hillegas commented on DERBY-712:

Hi Suran,

Thanks for the first rev of the patch. Lots of good stuff. Here are some comments. Despite
all of the great work here, I think that another patch will be needed and that in turn will
probably need some adjusting. Don't be discouraged, this is an iterative process and your
foundation here is solid.

o In a couple of the files, you have removed the imports of individual classes and replaced
them with a bulk import of all the classes in a package. Perhaps your IDE did this for you.
The convention in Derby is to declare each import individually. I have weak religion on this
style topic, but there are some reasons for this convention, which you can find at http://stackoverflow.com/questions/147454/why-is-using-a-wild-card-with-a-java-import-statement-bad/147532
and I recommend sticking with the Derby
style at least for your initial submissions.

o There are funny line endings for the new files in the patch. I think this may be because
you haven't set the new files to have native eol-style. Before generating the patch, you need
to issue the following command on each of the new files:

  svn propset svn:eol-style native $NEW_FILE

o I think that PermDescriptor should extend PermissionsDescriptor. This will make the code
easier to understand and it will probably make some existing logic just work for the USAGE
privilege when you code support for USAGE. Consult RoutinePermsDescriptor for the pattern
to follow.

o Similarly, I suspect that SYSPERMSRowFactory should extend PermissionsCatalogRowFactory
and follow the pattern of SYSROUTINEPERMSRowFactory.

o For some reason one of your new classes, SYSSEQUENCESRowFactory, appears in the patch as
a diff from SYSROLESRowFactory rather than as a new class. This makes it impossible to apply
the patch and it makes the patch hard to read.

o Following the existing pattern in DataDictionaryImpl, dropSequence() should be dropSequenceDescriptor()
and the argument should be a SequenceDescriptor. I don't think that this drop code is going
to do the right thing. It looks to me as though it is trying to use the sequence name (a String
datum) as a sequence id (a UUID datum) in order to drop the sequence by using the unique index
on sequence id. Take a look at DataDictionaryImpl.dropTrigger() for the pattern to follow.

o Similarly, getSequenceDescriptor(String sequenceName) should be getSequenceDescriptor(String
sequenceName, SchemaDescriptor sd). This is because the sequence name is not globally unique,
it is only unique within a given schema. The code here is not going to work. It is trying
to use the sequence name as a UUID in order to look up the sequence descriptor using the index
on sequence id. Instead, it needs to use the second index on SYSSEQUENCES, the one which has
a name and a UUID key. Again, see getTriggerDescriptor(String name, SchemaDescriptor sd) for
the pattern to follow.

o I don't understand how DataDictionaryImpl.dropPerm()  and getPermDescriptor() will be used.
I suspect that if you make PermDescriptor follow the pattern of RoutinePermDescriptor, then
you will not need these methods--you will need other methods!

o In SYSPERMSRowFactory, it looks as though you have allocated UUIDs for 3 indexes but you
have only defined one of them. I suspect you will need more than 1 index. In any event, the
index you have defined is the first index, not the 3rd index as indicated in the list of UUIDs.

o Don't forget to write header comments for the new methods which you add to the DataDictionary

o I think that FileInfoDescriptor would be a good pattern to follow for SequenceDescriptor.
In particular, I think you will want SequenceDescriptor to implement UniqueSQLObjectDescriptor.
I don't understand why SequenceDescriptor needs a writeExternal() method. Will these descriptors
be persisted in any way other than being flattened into tuples in SYSSEQUENCES? I understand
the need for the getXXX() methods, but why are the setXXX() methods necessary? I would eliminate
them unless these objects really need to be mutable.

o On second thought, I don't think you need a Formatable id for the the SequenceDescriptors
in StoredFormatIds. I apologize for misleading you there. I think you just need a Formatable
id for the SequenceDescriptorFinder.

o Have you tried running the regression tests with this patch? Usually when someone adds a
new catalog, this breaks the tests which track the shape of the system schema. You probably
need to submit some updated tests and/or test canons with this patch.


> Support for sequences
> ---------------------
>                 Key: DERBY-712
>                 URL: https://issues.apache.org/jira/browse/DERBY-712
>             Project: Derby
>          Issue Type: New Feature
>          Components: SQL
>         Environment: feature request 
>            Reporter: Tony Dahbura
>            Assignee: Suran Jayathilaka
>             Fix For:
>         Attachments: catalogs_a.patch, SequenceGenerator.html
> Would like to see support added for sequences.  This would permit a select against the
sequence to always obtain a ever increasing/decreasing value.  The identity column works fine
but there are times for applications where the application needs to obtain the sequence number
and use it prior to the database write.  Subsequent calls to the table/column would result
in a new number on each call.
> SQL such as the following:
> SELECT NEXT VALUE FOR sequence_name FROM sometable ; would result in a next value.

This message is automatically generated by JIRA.
You can reply to this email to add a comment to the issue online.

View raw message