[Mono-devel-list] Re: [Mono-patches] r44988 - in trunk/mcs/class/System.Data: System.Data Test/System.Data

[Konstantin Triger]

Hello Suresh,

I think that fixing a potential IndexOutOfRange exception should also be 
added to the list
+                               for (int i=0; i < PrimaryKey.Length; i++)
+                                       keys [i] = values [PrimaryKey 
[i].Ordinal]; <- exception

In addition, I have some questions too:

You asked me what is the best style to perfrom a search here (DataTable.cs):
+                                Index index = FindIndex 
(PrimaryKey,null,DataViewRowState.OriginalRows,null);
+                               if (index == null)
+                                       index = new Index (new Key(this, 
PrimaryKey,null,DataViewRowState.OriginalRows,null));
+
+                               object [] keys = new object 
[PrimaryKey.Length];
+                               for (int i=0; i < PrimaryKey.Length; i++)
+                                       keys [i] = values [PrimaryKey 
[i].Ordinal];
+
+                               int existingRecord = index.Find(keys);
+                               if (existingRecord >= 0)
+                                       row = RecordCache[existingRecord];
+                               else {
+                                       existingRecord = 
_primaryKeyConstraint.Index.Find(keys);

And I responded this:
If all you need is to find a row, it's better to add an overload for 
"public DataRowCollection.DataRow Find (object[] keys) " taking 
DataViewRowState.
ie "internal DataRowCollection.DataRow Find (object[] keys, 
DataViewRowState state)"
Both LoadDataRow and the existing Find() method will call it. All you 
will need is retrieving key values from your values.

Why did not you like it?


In this code (DataRow.cs):
+                                // update the pk index
+                                index = 
Table.GetIndex(Table.PrimaryKey,null,DataViewRowState.None,null,false);
+                                if (index != null)
+                                        index.Update (this, temp);

1. In general it's seems cleaner to write: 
Table.PrimaryKeyConstraint.Index.Update(...). This is the way you 
yourself got the PK constraint index in DataTable.
2. Don't you think that all the indices including the record should be 
updated too? Otherwise, if there is a DataView "viewing" this row, it 
will be outdated.

Regards,
Konstantin Triger



Sureshkumar T wrote:

>>                           index.Update (this, temp);
>>    
>>
>>>+
>>>+                                if (HasVersion (DataRowVersion.Current))
>>>+                                        Table.RecordCache.DisposeRecord (Current);
>>>+                                Current = temp;
>>> 
>>>
>>>      
>>>
>>Is there some special reason for first updating Original version, then 
>>updating an index, and then updating the Current version? Since index 
>>built on RowState.None, it uses Default (and not updated) value in index 
>>update.
>>    
>>
>
>yes, refer msdn DataTable.Load (object [], LoadOption) method for the
>matrix of initial & states.  First thing, I had to do was remove the
>regressions caused by your indexes redesign pathes. So, my patch is
>mainly a incremental patch, with the following things to be done yet.
>
>      * Since I am changing Original & Current Records, I have to update
>        all the indexes which could get invalidated for that row. Best
>        way is to update the respective indexes in the Original/Current
>        property setters. Let me know if you have some other index
>        updating mechanism. Ideally, Original re-assigning also should
>        update indexes. I'll be doing that fix later. Otherwise, we can
>        rebuild indexes at the end of LoadDataRow method.
>      * RowState.None is probably an oversight as that is how you are
>        dealing with indexes with PK in other parts.  This custom
>        updating would be removed if above point is completed.
>
>Thanks & Best Regards,
>suresh.
>_______________________________________________
>Mono-devel-list mailing list
>Mono-devel-list at lists.ximian.com
>http://lists.ximian.com/mailman/listinfo/mono-devel-list
>  
>