[
https://issues.apache.org/jira/browse/DERBY-3330?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Mike Matrigali updated DERBY-3330:
----------------------------------
here are some initial comments on the derby-3330v4.diff patch
overall:
o almost unique, doesn't seem like a good name for this. And I didn't see
good documentation in the code explaining what this means. Unfortunately
could not think of something less wordy than AllowMulipleNullsInUnique.
o not sure what you are using for tab/spaces, see derby web site for following
existing conventions in the code. may seem minor but inconsistent tab/indent
makes stuff unreadable. Derby code that has tab indentation expects 4 spaces
for those tabs.
java/engine/org/apache/derby/impl/sql/compile/CreateIndexNode.java
o no comments explaining new parameter and what false means.
CreateindexConstantAction also has no comments about the parameter either.
java/engine/org/apache/derby/impl/sql/execute/CreateIndexConstantAction.java
o inconsistent formatting with rest of file for new CreateIndexConstantAction
routine, so does not indent same as rest.
o Why did you add whole new routine rather than add single parameter to
existing routine and update other references. I think it is really error
prone to have 2 routines with same name and just differ by 12 or 13
params.
o no comments about almostUnique, some doc in the CreateIndexConstantAction
call and some comment following the existing code when property is set up
would be appropriate.
o This line looks unused:
Properties sortProperty = null;
it looks later like you use "properties" but other code seems to think that
this may be null sometimes.
o the tabbing/indentation inconsistency makes the sort part almost unreadable.
no comments for added sort logic.
java/engine/org/apache/derby/impl/sql/compile/TableElementList.java
o bad indentation
o looks like you removed check for null but left comments that we would check
for null.
o again no added comments for all changes to this file.
o need some explanation of what the added code is doing here. What is the
purpose of the else part of if (constraintDN.constraintType ==
DataDictionary.UNIQUE_CONSTRAINT)?
java/engine/org/apache/derby/impl/sql/execute/IndexChanger.java
o no comments in changed code
java/engine/org/apache/derby/impl/sql/execute/GenericConstantActionFactory.java
o bad indentation
o no comments
java/engine/org/apache/derby/impl/sql/catalog/DataDictionaryImpl.java
o bad indentation
o todo left in code, looks like needs to be resolved before checkin
java/engine/org/apache/derby/impl/store/access/sort/ExternalSortFactory.java
o bad indentation
o need some documentation on what changes to mergesort are doing.
java/engine/org/apache/derby/impl/store/access/btree/BTree.java
o bad indentation
o no comments for any change to file
java/engine/org/apache/derby/impl/store/access/btree/BTreeController.java
o a bunch of imports added that look unnecessary, ie. Time, TimeStampe,
Calendar, ...
o brace style inconsistent with rest of routine.
o getPreviousRecord()
o what is the expected behavior on latch waits?
o Does not follow latch/latch deadlock avoidance protocol - must not wait on
latch while holding latch while traveling left in tree.
o no handling of getting and releasing latches during search.
o see B2IRowLocking3.java!searchLeftAndLockPreviousKey for example of code
that searches left in btree.
o What kind of locking will the various isolation levels need for your
checking
of left and right keys? Is holding the latch while checking good enough? I
don't know right now.
o note the trickiest part of this is if you have to give up the latch while
waiting then you have to restart entire search of the tree to find where to
do the insert because you don't have latch.
o remember that you may have to search left many pages to find a data row.
o i don't think you should be searching for "undeleted", deleted records
still count toward the integrity of the tree. Without getting locks it
is impossible to know if the records marked deleted are committed deleted
or not. Imagine if you skip a marked deleted record that would have caused
a constraint violation, and then that xact backs out it would then mark
the row as valid and now you would have 2 records that fail the constraint.
getNextRecord()
o what is expected behavior of latch waits?
o what is expected behavior if routine has to visit N right leaves (ie. will
routine hold all latches until end of transaction)?
o as in getPreviousRecord() i think it should not special case deleted rows
o remember you may have to search right many pages to find row in edge case.
compareLeftAndRightSiblings()
o likely code to deal with lost latches would go into this routine and
callers.
o no need to call runtime_mem.get_template(getRawTran()) twice. Once you
call it you have a template. Just reuse the template.
o there should be some comments somewhere explaining why just checking left
and right siblings works for what you are trying to achieve.
o i have to think about it some more but the places you added the
if (compareLeftAndRightSiblings(rowToInsert,
insert_slot, targetleaf))
return ConglomerateController.ROWISDUPLICATE;
don't seem optimal. I would like to see the code only called in the
almost unique case, so we don't affect the behavior of existing indexes at
all.
java/engine/org/apache/derby/impl/store/access/btree/index/B2I.java
o it is critical to properly document changes to ondisk format of store objects,
doesn't look like any work here done. I know from existing comments that
upgrade still does not work so maybe you are planning more here.
o The model has been to add new stuff at the end rather than in the middle.
o so far I haven't seen what is going to stop this from being created in soft
upgrade.
java/engine/org/apache/derby/modules.properties
o did you consider just altering the existing sort to take an additional
startup parameter rather than extending and creating a new module to sort?
just would be interested to know why one approach vs. the other.
java/engine/org/apache/derby/catalog/types/IndexDescriptorImpl.java
o bad indent in places
o as todo's point out, need soft upgrade support
> provide support for unique constraint over nullable columns
> -----------------------------------------------------------
>
> Key: DERBY-3330
> URL: https://issues.apache.org/jira/browse/DERBY-3330
> Project: Derby
> Issue Type: New Feature
> Components: Store
> Affects Versions: 10.4.0.0
> Environment: all
> Reporter: Anurag Shekhar
> Assignee: Anurag Shekhar
> Attachments: derby-3330-testcase.diff, derby-3330.diff,
> derby-3330v2.diff, derby-3330v3.diff, derby-3330v4.diff,
> FunctionalSpec_DERBY-3330.html
>
>
> Allow unique constraint over nullable field. Right now derby support unique
> constraint only over not null columns.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
