Cleanup of IRowCursor and related interfaces. IRowCursor/IRow are now classes. #1814
Conversation
TomFinley
requested review from
codemzs,
Ivanidzo4ka,
eerhardt,
yaeldekel,
artidoro and
sfilipi
| { | ||
| /// <summary> | ||
| /// Returns the state of the cursor. Before the first call to <see cref="MoveNext"/> or | ||
| /// <see cref="MoveMany(long)"/> this should be <see cref="CursorState.NotStarted"/>. After | ||
| /// any call those move functions that returns <see langword="true"/>, this should return | ||
| /// <see cref="CursorState.Good"/>, | ||
| /// </summary> | ||
| CursorState State { get; } | ||
| public abstract CursorState State { get; } |
There was a problem hiding this comment.
If you, gentle reader, took my advice and are reviewing commit by commit, you might in reading this be tempted to note that these have no documentation. Note that the lack of documentation on these in this commit is a temporary situation I knew I would change in a subsequent commit, since when IRow itself became a class in a later commit, I then immediately turn around and delete these abstract members.
|
|
||
| namespace Microsoft.ML.Benchmarks | ||
| { | ||
| public class CacheDataViewBench |
There was a problem hiding this comment.
This benchmark was necessary because, once I made IRowCursor and IRowSeeker abstract classes, not interfaces, the internal implementation code of the CacheDataView could no longer exploit the multiple inheritance you get from interfaces, to have both have common code; I instead solved that by composition, which involves wrapping objects. (A common pattern in my changes.) I wanted to check whether this resulted in a perf degradation when I did the change in a subsequent commit. (It doesn't seem like it did.)
* Members of ICounted moved to IRow. * Members of ICursor moved to IRowCursor. * Remove ICounted and ICursor. * Adjust implementing code to the absence of these interfaces.
…rent sorts of cursors.
TomFinley
force-pushed
the
tfinley/Cursor
branch
from
f03934f to
d995553
Compare
| Ch.CheckParam(col == 0, nameof(col)); | ||
| return Enumerable.Empty<KeyValuePair<string, ColumnType>>(); | ||
| } | ||
| public override long Batch => throw new NotImplementedException(); |
There was a problem hiding this comment.
throw new NotImplementedException(); [](start = 42, length = 36)
flagging this one too.
There was a problem hiding this comment.
Thanks @sfilipi , I believe I fixed that in my last commit (I fixed both this and the other one that you had already commented on at the same time), maybe you are looking at the older commit?
ghost
locked as resolved and limited conversation to collaborators
Partial fix for #1532. I say partial because part of what we also wanted to do was clean up some of the things in the interface (now class) that have not proven useful, but, this work is already large to the point where it would be probably unwise to try to make the PR larger.
I have structured the commits so they are easy to understand, if viewed one by one. The commits with "Rename" in the description are precisely that, that is, me going to one or more types as described, hitting F2, typing something new, and that becomes my commit. Consider reviewing the other ones -- there is probably not much use in review of the rename ones. (Except perhaps to suggest a better name than
Row. 😛 )