[
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 http://192.220.96.201/essays/java-style/structure.html 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 interface
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.
Thanks!
-Rick
> 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: 10.6.0.0
>
> 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.
