Skip to content

Remove IRowCursorConsolidator #1867

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 Dec 12, 2018 · 1 comment
Closed

Remove IRowCursorConsolidator #1867

TomFinley opened this issue Dec 12, 2018 · 1 comment
Labels
API Issues pertaining the friendly API

Comments

@TomFinley
Copy link
Contributor

Consider this method:

RowCursor[] GetRowCursorSet(out IRowCursorConsolidator consolidator,
Func<int, bool> needCol, int n, Random rand = null);

This returns an instance of the mysterious IRowCursorConsolidator interface. Why an interface? In retrospect I'm not quite sure.

Having an interface allows for different implementations, but we've never actually really exploited that capability. Nor, even if we were of such a mind to do so, would it be clear how we could. What would they even do differently? The semantics around Batch and whatnot are sufficiently clear and simple as to make only one implementation obvious, and even if we did have different implementations since the resulting cursors result most often from transformers (that is, components downstream from the set creation), they couldn't really do anything radically different anyway, since to do anything implementation specific on any cursor would be to break the composability at the core of what makes IDataView work at all.

So: get rid of this interface, and replace all usage of it with a simple utility method somewhere that can be called to do the reconciliation. It needn't even be a public utility method, but there may be reasons to do so.

@TomFinley TomFinley added the API Issues pertaining the friendly API label Dec 12, 2018
@TomFinley
Copy link
Contributor Author

TomFinley commented Dec 20, 2018

Now that I've started working on this, I am no longer convinced at all that having an "official" utility method somewhere is useful. The most common user by far of this is other IDataView implementations, and the sheer number of ways in which this thing is consumed make quite clear that among all the possible things you could do with a cursor set, there are many things that can and are useful to do with the set, and only one of which the reconciler could do. But the user would never even need to call the reconciler themselves anyway: if they are just using IDataView serially through the serial cursor, then the splitting and reconcilation is happening for them anyway. And if they genuinely did want a row cursor set, it is a vanishingly small probability -- in fact I can't really think of any valid situation -- that the "right" thing to do with this thing would be to collapse it down into one cursor. Indeed, in retrospect, I now recall multiple situations where having this very conspicuous IRowCursorConsolidator mislead people rather entirely -- they thought quite naturally, surely this is being provided for some reason, but in reality in nearly all cases the right thing to do was to ignore it and pretend it did not exist. Only in a few isolated cases (again, relating to IDataView transformer implementations) was consolidation correct.

So, I will take out the consolidator, and not (at least at first) replace it with anything. I will instead update the descriptions of row cursor sets so it is more clear how to consume them (and what the role of Row.Batch is), and leave them to their own devices on how precisely they want to exploit this sharding of the data -- which of course, has been our practical position all this time for years.

This is outlined by the fact that throughout even our own codebase, of all the things that were done with cursor sets, actual consolidation only happened in one or two places.

@TomFinley TomFinley changed the title Remove IRowCursorConsolidator, replace with utility method somewhere Remove IRowCursorConsolidator Dec 20, 2018
@ghost ghost locked as resolved and limited conversation to collaborators Mar 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
API Issues pertaining the friendly API
Projects
None yet
Development

No branches or pull requests

1 participant