[Mono-dev] RE: [Mono-devel-list] System.Data : DataTable.Select performance

[Boris Kirzner]

Hello Suresh,

> @@ -193,9 +206,14 @@
>   				return false;
>   			}
> -			if (_filter != filter)
> -				return false;
> +			if (_filter != null) {
> +				if (!_filter.Equals (filter))
> +					return false;
> +			}
> +			else if (filter != null)
> +					return false;
> 
> =======================================================
> please review this conditional checking.
> =======================================================

I've reviewed it and it seems ok. Can you point to some specific problem
in this code?

> 
>  > @@ -640,6 +640,10 @@
>  > Table.RecordCache.DisposeRecord(Proposed);
>  >  				Proposed = -1;
>  > +
>  > +				int newVersion = (HasVersion > 
> (DataRowVersion.Current)) ?
> Current : Original;					
>  > +				foreach(Index index in Table.Indexes)
>  > +					
> index.Update(this,newVersion);					
> =======================================================
> why need to update indexes in CancelEdit. anyway, all values 
> of the row are removed?.
> =======================================================

No, not all the values are removed, but the Proposed record is removed
and the old (Current or Original) is coming instead.

> 
>  > @@ -1299,7 +1303,13 @@
>  >  				}
>  >  				CheckChildRows(DataRowAction.Rollback);
>  >
>  > -				Current = Original;
>  > +				if (Current != Original) {
>  > +					foreach(Index index in 
> Table.Indexes) {
>  > +						index.Delete (this);
>  > +						> 
> index.Update(this,Original);
>  > +					}
>  > +					Current = Original;
>  > +				}
> 
> =======================================================
> that means, wherever there is a pattern like this (even 
> Original = Current), we need to update indexes.

I'm not sure we should act the same in all the cases. Maybe we should
review the code after the patches and, if we'll succeed to find a test
cases, fix the code.

>  > There is two additional problems :
>  > - Not that overall number of the rows returned differs 
> from .Net. It is  > a separate bug (it happens before the 
> update also).
> 
> is there a bug report?

No, but I'll create one.

> regressions are not acceptable in favor of performance 
> benefits. your3 patch introduces 1 regression in default 
> profile and 2 regressions in net_2_0 profile. But, the bug is 
> somewhere with handling of the indexes.

I've worked a bit more on the patch, so it does not causes a regressions
in default.
I have problems working with 2_0 here, so can you please recheck my
patches after I send them?

> I think you can split your fix into
> 
> 1. Equals implementations for expressions 2. Filter.Equals 
> inside Key.Equals 3. Select method optimization.

Ok, I'll send separate patched later.

Thanks,

--
Boris Kirzner
Mono R&D team, Mainsoft Corporation.
Blogging at http://boriskirzner.blogspot.com/