Skip to content

IDataView Cleanup: Clear cutting the IRowCursor interface jungle #1532

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
TomFinley opened this issue Nov 5, 2018 · 1 comment
Closed

IDataView Cleanup: Clear cutting the IRowCursor interface jungle #1532

TomFinley opened this issue Nov 5, 2018 · 1 comment
Assignees

Comments

@TomFinley
Copy link
Contributor

TomFinley commented Nov 5, 2018

Right now we have a fairly elaborate set of interfaces surrounding IRowCursor. These include ISchematized, ICounted, IRow.

While IRowCursor and IRow are quite useful, it is not clear if separating out ICursor and ICounted as separate interfaces is actually helpful, and we think we ought to get rid of ISchematized anyway (see #1502). The goal would be to refactor this jungle so that there are, relating to row cursors, two types, both abstract classes, corresponding to IRow and IRowCursor, but of course not named that since they won't be interfaces. (What would be a good name is up for debate. RowCursor seems probably fine, but just Row does not seem like a good name.)

It may be that we have the name Row for now until we imagine a better name, and treat that renaming as a separate issue from this issue which mostly treats on the proper refactoring of the code.

ICursor does have some implementations that are not also IRowCursor, but it is unclear whether this has to be this way, or if it is just that way just because it was there.

The collapse of ICounted into these hypothetical Row and RowCursor classes would be mostly into Row.

While we are at it...

While refactoring this code it would be advantageous to at the same time deal with the refactorings of other things methods in cursors, as we will be restructuring them anyway.

  • The utility of State on cursors has proven to not be terribly useful. People create and use cursors more or less like they do enumerators -- they create them, move-next over them, then when they stop the thing is disposed. It is valuable for the sake of assert checking, but otherwise I am not aware of any use. Certainly IEnumerator gets along just fine without this...

  • The MoveMany method seems attractive. In many cases, you can in fact move faster than just an exhaustive MoveNext to get to where you want to go. The trouble is the utility for such a mechanism has proven to be limited -- sure, a Skip filter could be more performant, but beyond that usages are few. We may as well remove it.

  • GetRootCursor's performs an essential job (bypass nested move-next so that nested cursors, which is to say most cursors, have a fast MoveNext). By moving this to an abstract class, we can see if this "bypass" mechanism can be achieved in a less obnoxious (possibly internal) way to cursors.

  • Possibly not to be done here, it would be nice if instead of IRowCursor (or rather, its abstract class descendant) being disposable, that the IRow is disposable. This is desirable but may require additional considerations, and might be best achieved in a separate issue.

  • The utility of the IsActiveColumn is not immediately clear. Can we get away with simply not providing it? It may be though that we decide to keep it.

/cc @Zruty0 @shauheen @terrajobst

@TomFinley
Copy link
Contributor Author

  • The utility of the IsActiveColumn is not immediately clear. Can we get away with simply not providing it? It may be though that we decide to keep it.

As I started removing it its utility became more clear. Some nested verification relies on it in way that are hard to duplicate otherwise. Further, some test code relied on it; I could not otherwise reasonably tease apart "failed because of bug," and "failed because it is supposed to fail, because we did not activate the cursor."

And, while we are used to thinking about clearcut end-to-end scenarios involving Row and RowCursor, some other scenarios like in testing that struck me as potentially useful became clear. So for example, imagine writing, say, a debug proxy for a Row or RowCursor. Can you really do it reasonably without something like IsActiveColumn?

There were lots of little things like this that kept adding up till the point where it became clear that its removal introduced more problems than it solved. (In contrast to the other removals.)

@ghost ghost locked as resolved and limited conversation to collaborators Mar 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant