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

Boris Kirzner borisk at mainsoft.com
Thu May 26 08:02:10 EDT 2005


Hello Suresh
There some things that looks unclear for me in the last patch... Can you 
explain the points below ?

Thanks,
Boris

suresh <sthangavel at novell.com> wrote:

>Author: suresh
>Date: 2005-05-25 06:57:45 -0400 (Wed, 25 May 2005)
>New Revision: 44988
>
>Modified:
>   trunk/mcs/class/System.Data/System.Data/ChangeLog
>   trunk/mcs/class/System.Data/System.Data/DataRow.cs
>   trunk/mcs/class/System.Data/System.Data/DataTable.cs
>   trunk/mcs/class/System.Data/Test/System.Data/ChangeLog
>   trunk/mcs/class/System.Data/Test/System.Data/DataTableLoadRowTest.cs
>Log:
>	* System.Data/DataTable.cs: Reworked DataTable.LoadDataRow method after
>	  regressions caused by Index redesign.
>	* System.Data/DataRow.cs: Load : reworked.
>
>	* Test/System.Data/DataTableLoadRowTest.cs: Added  additional cases for
>	  AutoIncrementTest to gauge any side effect with auto
>	  incrementing in case of upsert.
>
>
>
>Modified: trunk/mcs/class/System.Data/System.Data/ChangeLog
>===================================================================
>--- trunk/mcs/class/System.Data/System.Data/ChangeLog	2005-05-25 09:55:24 UTC (rev 44987)
>+++ trunk/mcs/class/System.Data/System.Data/ChangeLog	2005-05-25 10:57:45 UTC (rev 44988)
>@@ -1,3 +1,9 @@
>+2005-05-25  Sureshkumar T  <tsureshkumar at novell.com>
>+
>+	* DataTable.cs: Reworked DataTable.LoadDataRow method after
>+	regressions caused by Index redesign.
>+	* DataRow.cs: Load : reworked.
>+
> 2005-05-25 Konstantin Triger <kostat at mainsoft.com>
> 
> 	* ISafeDataRecord.cs: Added GetDateTimeSafe method, the interface was made derived from IDataRecord
>
>Modified: trunk/mcs/class/System.Data/System.Data/DataRow.cs
>===================================================================
>--- trunk/mcs/class/System.Data/System.Data/DataRow.cs	2005-05-25 09:55:24 UTC (rev 44987)
>+++ trunk/mcs/class/System.Data/System.Data/DataRow.cs	2005-05-25 10:57:45 UTC (rev 44988)
>@@ -590,14 +590,14 @@
>                         _table.ChangingDataRow (this, DataRowAction.Commit);
> 			CheckChildRows(DataRowAction.Commit);
> 			switch (RowState) {
>-				case DataRowState.Unchanged:
>+                        case DataRowState.Unchanged:
> 					return;
> 			case DataRowState.Added:
> 			case DataRowState.Modified:
>-					if (Original >= 0) {
>-						Table.RecordCache.DisposeRecord(Original);
>-					}
>-					Original = Current;
>+                                if (Original >= 0) {
>+                                        Table.RecordCache.DisposeRecord(Original);
>+                                }
>+                                Original = Current;
> 				break;
> 			case DataRowState.Deleted:
> 				_table.Rows.RemoveInternal (this);
>@@ -1566,57 +1566,65 @@
>                 ///    mentioned in the DataTable.Load (IDataReader, LoadOption) method.
>                 /// </summary>
>                 [MonoTODO ("Raise necessary Events")]
>-                internal void Load (object [] values, LoadOption loadOption, bool is_new)
>+                internal void Load (object [] values, LoadOption loadOption)
>                 {
>+                        Index index = null;
>                         DataRowAction action = DataRowAction.Change;
> 
>-                        int temp = Table.RecordCache.NewRecord ();
>-                        for (int i = 0 ; i < Table.Columns.Count; i++)
>-                                SetValue (i, values [i], temp);
>+                        int temp = -1;
> 
>-                        if (is_new) { // new row
>-                                if (HasVersion (DataRowVersion.Proposed) || RowState == DataRowState.Detached)
>-                                        Proposed = temp;
>-                                else
>-                                        Current = temp;
>-                                return;
>-                        }
>-
>                         if (loadOption == LoadOption.OverwriteChanges 
>                             || (loadOption == LoadOption.PreserveChanges
>                                 && RowState == DataRowState.Unchanged)) {
>+				temp = Table.CreateRecord (values);
>+                                if (HasVersion (DataRowVersion.Original) && Current != Original)
>+                                        Table.RecordCache.DisposeRecord (Original);
>                                 Original = temp;
>-                                if (HasVersion (DataRowVersion.Proposed))
>-                                        Proposed = temp;
>-                                else
>-                                        Current = temp;
>+                                // update the pk index
>+                                index = Table.GetIndex(Table.PrimaryKey,null,DataViewRowState.None,null,false);
>+                                if (index != null)
>+                                        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.

>                                 action = DataRowAction.ChangeCurrentAndOriginal;
>                                 return;
>                         }
> 
>                         if (loadOption == LoadOption.PreserveChanges) {
>-                                if (RowState != DataRowState.Deleted) {
>-                                        Original = temp;
>-                                        action   = DataRowAction.ChangeOriginal;
>-                                }
>+				if (RowState == DataRowState.Deleted)
>+					return;
>+				temp = Table.CreateRecord (values);
>+				if (HasVersion (DataRowVersion.Original) && Current != Original)
>+					Table.RecordCache.DisposeRecord (Original);
>+				Original = temp;
>+				action   = DataRowAction.ChangeOriginal;
>                                 return;
>                         }
>                                 
>-                        bool not_used = true;
>                         // Upsert
>  
>
The following code runs also for loadOption other that Upset ( 
PreserveCurrentValues for example). Is this correct?

>                         if (RowState != DataRowState.Deleted) {
>-                                int index = HasVersion (DataRowVersion.Proposed) ? _proposed : _current;
>-                                if (Table.CompareRecords (index, temp) != 0) {
>-                                        if (HasVersion (DataRowVersion.Proposed))
>-                                                Proposed = temp;
>-                                        else
>-                                                Current = temp;
>-                                        not_used = false;
>-                                }
>+                                int rindex = HasVersion (DataRowVersion.Proposed) ? Proposed : Current;
>+				temp = Table.CreateRecord (values);
>+				if (Table.CompareRecords (rindex, temp) != 0) {
>+                                        if (HasVersion (DataRowVersion.Proposed)) {
>+                                                Table.RecordCache.DisposeRecord (Proposed);
>+                                                Proposed = -1;
>+                                        }
>+                                        
>+                                        // update the pk index
>+                                        index = Table.GetIndex(Table.PrimaryKey,null,DataViewRowState.None,null,false);
>+                                        if (index != null)
>+                                                index.Update (this, temp);
>+
>+                                        if (Original != Current)
>+                                                Table.RecordCache.DisposeRecord (Current);
>+                                        Current = temp;
>+                                } else
>+					Table.RecordCache.DisposeRecord (temp);
>                         }
>-                                
>-                        if (not_used)
>-                                Table.RecordCache.DisposeRecord (temp);
>+
>                 }
> #endif // NET_2_0
> 	}
>
>Modified: trunk/mcs/class/System.Data/System.Data/DataTable.cs
>===================================================================
>--- trunk/mcs/class/System.Data/System.Data/DataTable.cs	2005-05-25 09:55:24 UTC (rev 44987)
>+++ trunk/mcs/class/System.Data/System.Data/DataTable.cs	2005-05-25 10:57:45 UTC (rev 44988)
>@@ -1199,31 +1199,45 @@
> 		public DataRow LoadDataRow (object [] values, LoadOption loadOption)
> 		{
>                         DataRow row  = null;
>-                        bool new_row = false;
>                         
>                         // Find Data DataRow
>                         if (this.PrimaryKey.Length > 0) {
>-				int newRecord = CreateRecord(values);
>-				try {
>-					Index index = GetIndex(PrimaryKey,null,DataViewRowState.OriginalRows,null,false);
>-					int existingRecord = index.Find(newRecord);
>+                                Index index = FindIndex (PrimaryKey,null,DataViewRowState.OriginalRows,null);
>+				if (index == null)
>+					index = new Index (new Key(this, PrimaryKey,null,DataViewRowState.OriginalRows,null));
>  
>
This index is used only for lookup - am I right?

>+
>+				object [] keys = new object [PrimaryKey.Length];
>+				for (int i=0; i < PrimaryKey.Length; i++)
>+					keys [i] = values [PrimaryKey [i].Ordinal];
>  
>
This assignment will raise an exception if PrimaryKey[i].Ordinal exeeds 
values.Length. Should probably check for this and raise the 
corresponding exception (the row can not be added if not all the primary 
keya values exists and the missing PK columns are not autoincrement).

>+
>+				int existingRecord = index.Find(keys);
>+				if (existingRecord >= 0)
>+					row = RecordCache[existingRecord];
>+				else {                                           
>+					existingRecord = _primaryKeyConstraint.Index.Find(keys);
> 					if (existingRecord >= 0)
> 						row = RecordCache[existingRecord];
>-					else {
>-						existingRecord = _primaryKeyConstraint.Index.Find(newRecord);
>-						if (existingRecord >= 0)
>-							row = RecordCache[existingRecord];
>-					}
> 				}
>-				finally {
>-					RecordCache.DisposeRecord(newRecord);
>-				}
>                         }
>                                 
>                         // If not found, add new row
>-                        if (row == null) {
>-                                row = this.NewRow ();
>-                                new_row = true;
>+                        if (row == null 
>+                            || (row.RowState == DataRowState.Deleted
>+                                && loadOption == LoadOption.Upsert)) {
>  
>
According to MSDN Upsert should preserve an Original version. Are you 
shure Deleted row always misses an Originlal version? If no, it should 
be preserved.

>+                                row = NewNotInitializedRow ();
>+                                row.ImportRecord (CreateRecord(values));
>+
>+                                if (EnforceConstraints) 
>+                                        // we have to check that the new row doesn't colide with existing row
>+                                        Rows.ValidateDataRowInternal(row); // this adds to index ;-)
>+                                     
>+                                Rows.AddInternal(row);		
>+	
>+                                if (loadOption == LoadOption.OverwriteChanges ||
>+                                    loadOption == LoadOption.PreserveChanges) {
>+                                        row.AcceptChanges ();
>+                                }
>+                                return row;
>                         }
> 
>                         bool deleted = row.RowState == DataRowState.Deleted;
>@@ -1231,21 +1245,8 @@
>                         if (deleted && loadOption == LoadOption.OverwriteChanges)
>                                 row.RejectChanges ();                        
> 
>-                        row.Load (values, loadOption, new_row);
>+                        row.Load (values, loadOption);
> 
>-                        if (deleted && loadOption == LoadOption.Upsert) {
>-                                row = this.NewRow ();
>-                                row.Load (values, loadOption, new_row = true);
>-                        }
>-
>-                        if (new_row) {
>-                                this.Rows.Add (row);
>-                                if (loadOption == LoadOption.OverwriteChanges ||
>-                                    loadOption == LoadOption.PreserveChanges) {
>-                                        row.AcceptChanges ();
>-                                }
>-                        }
>-
>                         return row;
> 		}
> 
>
>Modified: trunk/mcs/class/System.Data/Test/System.Data/ChangeLog
>===================================================================
>--- trunk/mcs/class/System.Data/Test/System.Data/ChangeLog	2005-05-25 09:55:24 UTC (rev 44987)
>+++ trunk/mcs/class/System.Data/Test/System.Data/ChangeLog	2005-05-25 10:57:45 UTC (rev 44988)
>@@ -1,3 +1,9 @@
>+2005-05-25  Sureshkumar T  <tsureshkumar at novell.com>
>+
>+	* DataTableLoadRowTest.cs: Added  additional cases for
>+	AutoIncrementTest to gauge any side effect with auto
>+	incrementing in case of upsert.
>+
> 2005-05-20  Sureshkumar T  <tsureshkumar at novell.com>
> 
> 	* DataRowCollectionTest.cs: Added a test to check Rows.Add (values
>
>Modified: trunk/mcs/class/System.Data/Test/System.Data/DataTableLoadRowTest.cs
>===================================================================
>--- trunk/mcs/class/System.Data/Test/System.Data/DataTableLoadRowTest.cs	2005-05-25 09:55:24 UTC (rev 44987)
>+++ trunk/mcs/class/System.Data/Test/System.Data/DataTableLoadRowTest.cs	2005-05-25 10:57:45 UTC (rev 44988)
>@@ -271,6 +271,16 @@
>                         Assert.AreEqual (25, dt.Rows [3] [0], "#2 current should be ai");
>                         Assert.AreEqual (25, dt.Rows [3] [0, DataRowVersion.Original], "#3 original should be ai");
> 
>+			dt.LoadDataRow (new object [] {25, 20, "mono test"}, LoadOption.Upsert);
>+			dt.LoadDataRow (new object [] {25, 20, "mono test 2"}, LoadOption.Upsert);
>+			dt.LoadDataRow (new object [] {null, 20, "mono test aaa"}, LoadOption.Upsert);
>+			
>+			Assert.AreEqual (5, dt.Rows.Count, "#4 has not added a new row");
>+			Assert.AreEqual (25, dt.Rows [3] [0], "#5 current should be ai");
>+			Assert.AreEqual (25, dt.Rows [3] [0, DataRowVersion.Original], "#6 original should be ai");
>+
>+			Assert.AreEqual (30, dt.Rows [4] [0], "#7 current should be ai");
>+
>                 }
>         }
> }
>
>_______________________________________________
>Mono-patches maillist  -  Mono-patches at lists.ximian.com
>http://lists.ximian.com/mailman/listinfo/mono-patches
>  
>



More information about the Mono-devel-list mailing list