Issue 4047, improve error message give expected type #5189
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5189 +/- ##
==========================================
- Coverage 73.33% 69.07% -4.26%
==========================================
Files 1007 762 -245
Lines 188388 144527 -43861
Branches 20286 18374 -1912
==========================================
- Hits 138145 99833 -38312
+ Misses 44723 39510 -5213
+ Partials 5520 5184 -336
|
| throw Ch.Except($"Invalid TValue: '{typeof(TValue)}', " + | ||
| $"expected type: '{_getter.GetType().GetGenericArguments().First()}'."); |
There was a problem hiding this comment.
Nit: shouldn't this be _getter.GetType().GetGenericArguments().First().GetGenericArguments().First() just as you did in Transposer.cs since this expects a VBuffer? #Resolved
There was a problem hiding this comment.
|
|
||
| var originGetter = _getters[_colToActivesIndex[column.Index]]; | ||
| ValueGetter<TValue> getter = originGetter as ValueGetter<TValue>; | ||
| if (getter == null) | ||
| throw Ch.Except("Invalid TValue: '{0}'", typeof(TValue)); | ||
| throw Ch.Except($"Invalid TValue: '{typeof(TValue)}', " + | ||
| $"expected type: '{originGetter.GetType().GetGenericArguments().First()}'."); |
There was a problem hiding this comment.
(pinned to code to allow for threaded conversation)
In general, I think that the approach you used to fix this issue is fine, and it's actually a very good addition to make our exception messages more clear.
The only potential problem I see with it is that maybe this will end up throwing another exception: for example, the _getters in this method is an array of Delegates (i.e. it's not an array of ValueGetter<>'s) so the cast as ValueGetter<TValue> could potentially fail not only if the the generic parameter is mistaken, but also in the case that _getters[i] isn't a ValueGetter<> to begin with. If it isn't a ValueGetter<>, and if it's not a delegate with generic types, then the .First() call in here will throw an exception because the .GetGenericArguments() method would have thrown an empty array.
I haven't checked the other places were you've made changes, but some of them do have an original _getter which is already a ValueGetter<> (so this wouldn't be a problem in those cases) but in other cases the original _getter is a Delegate instead of ValueGetter<> (so potentially they might not have GenericArguments, and then throw another exception).
I'm not sure if I'm explaining myself.
In any case, I also don't know if we should worry about this problem... that other exception would only be thrown in some very fringe case (which I'm not sure if it's actually possible to reach that case), and in any way we were already throwing an exception. So maybe this is unimportant. #Resolved
There was a problem hiding this comment.
I understand your concern, please check out the implementation of this _getters, mainly method CreateGetterDelegate in same class and you will find below implementation, so it is guaranteed you will get ValueGetter for this Delegate but the real problem is you are using wrong TValue type.
private ValueGetter<TValue> CreateGetterDelegate<TValue>(ShufflePipe pipe)
{
Ch.AssertValue(pipe);
Ch.Assert(pipe is ShufflePipe<TValue>);
var pipe2 = (ShufflePipe<TValue>)pipe;
ValueGetter<TValue> getter =
(ref TValue value) =>
{
Ch.Assert(_pipeIndex == _pipeIndices[_circularIndex]);
pipe2.Fetch(_pipeIndex, ref value);
};
return getter;
}
In reply to: 433925403 [](ancestors = 433925403)
| ValueGetter<VBuffer<TValue>> getter = _getter as ValueGetter<VBuffer<TValue>>; | ||
| if (getter == null) | ||
| throw Ch.Except("Invalid TValue: '{0}'", typeof(TValue)); | ||
| throw Ch.Except($"Invalid TValue: '{typeof(TValue)}', " + | ||
| $"expected type: '{_getter.GetType().GetGenericArguments().First().GetGenericArguments().First()}'."); |
There was a problem hiding this comment.
In here we expect a VBuffer, and that's why you're getting .GetGenericArguments() twice, which is fine to get a descriptive exception.
But what happens if _getter is, let's say, of type ValueGetter<int> whereas we expect one of type ValueGetter<VBuffer<float>> ? since the getter we're getting isn't a VBuffer<> then another exception will be thrown when calling the second .First() because the second .GetGenericArguments() would return an empty array. This other exception would be more confusing than the original one, so please add the necessary logic to handle this case correctly in the places where we expect a VBuffer. 😄 #Resolved
There was a problem hiding this comment.
check out the definition of _getter: private readonly ValueGetter<VBuffer> _getter;
In reply to: 433952241 [](ancestors = 433952241)
|
You can include the issue number in your PR, but please give a more meaningful title to your PR. #Resolved |
frank-dong-ms-zz
changed the title
|
type for ValueGetter is determined by the type definition so it is safe to give exception In reply to: 422781310 [](ancestors = 422781310) |
|
Thanks, I changed the title, issue number is already put at PR comments In reply to: 637723803 [](ancestors = 637723803) |
ghost
locked as resolved and limited conversation to collaborators
address #4047