Cudf: [BUG] Make mutable_column_view conversion to column_view explicit

[jeanp413]

Describe the bug
Could we make the mutable_column_view conversion to column_view explicit ?
from:
https://github.com/rapidsai/cudf/blob/51f7f4aa619c43eed829208c1a0ee70b86b13479/cpp/include/cudf/column/column_view.hpp#L499
to:
explicit operator column_view() const;

Steps/Code to reproduce bug

1. Call
   ```
   auto column = cudf::make_numeric_column(...);
   cudf::experimental::fill(column->mutable_view(), 0, column->size(), scalar);
   ````

Expected behavior
I intended to call
https://github.com/rapidsai/cudf/blob/51f7f4aa619c43eed829208c1a0ee70b86b13479/cpp/include/cudf/filling.hpp#L53-L54
and expected a compile error by compiling the above code but due to the implicit casting the above code compiles just fine and ends up calling https://github.com/rapidsai/cudf/blob/51f7f4aa619c43eed829208c1a0ee70b86b13479/cpp/include/cudf/filling.hpp#L79-L82

[harrism]

@jrhemstad any non-obvious impacts of the suggested change?

[harrism]

BTW @jeanp413 feel free to submit a PR. :)

[jrhemstad]

I have mixed feelings, but right now I'm a bit resistant to making it explicit.

All libcudf APIs operate on column_view objects such that if you have a column or mutable_column_view object, you can transparently pass that object into any libcudf API and it "just works".

column c{...}
mutable_column_view mcv{...};

libcudf_api(c);
libcudf_api(mcv); // either of these should work transparently

The intention is that converting from a mutable view to an immutable view is always a safe operation. This is similar to how it's always safe to pass a non-const object into an API that accepts a const& without any user work, but the reverse is not true---you can't pass a const object into an API that accepts a non-const ref (this would require an explicit const_cast).

If we make it explicit, then it makes using libcudf APIs more awkward whenever a mutable_column_view is used.

column c{...}
mutable_column_view mcv{...};

libcudf_api(c);
libcudf_api(column_view{mcv}); // explicit and kind of awkward

From @jeanp413's example,

cudf::experimental::fill(column->mutable_view(), 0, column->size(), scalar);

as was stated, this is an error because it's passing in a r-value into an API that expects its argument by non-const ref. It would definitely be nice if in this situation the compiler could have told you that this was an error.

The argument could be made that mutable_column_views are used so infrequently, it wouldn't be common to have to do the explicit conversion, and so my above concerns about the awkwardness wouldn't be _that bad_, but I'm still conflicted.

[jlowe]

Regardless of how this is resolved, I think it would be clearer if the in-place fill were named as such to avoid any possibility of confusion on whether it will create a copy or not. IMO the semantics are different enough that it warrants a new name rather than subtle overloading on the same name.

[jeanp413]

The intention is that converting from a mutable view to an immutable view is always a safe operation. This is similar to how it's always safe to pass a non-const object into an API that accepts a const& without any user work, but the reverse is not true---you can't pass a const object into an API that accepts a non-const ref (this would require an explicit const_cast).

That makes sense then I think renaming the in-place fill is the way to go as jlowe suggested.
Feel free to close my PR then.

[harrism]

I agree. @jeanp413 can you just change your PR into a PR that renames in-place fill to fill_in_place? (or open a new one if you prefer).

[jeanp413]

Sure

[jeanp413]

I just found in-place copy_range which has the same problem
https://github.com/rapidsai/cudf/blob/11a0e42cd304cb71f6e5e129cb1375388c153ce1/cpp/include/cudf/copying.hpp#L261-L264
I'm gonna create another PR to rename it to copy_range_in_place
@harrism @jrhemstad