[ 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.