-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Changing the database cursor to return default for DBNull #4070
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
Conversation
|
src/Microsoft.ML.Experimental/DataLoadSave/Database/DatabaseLoaderCursor.cs
Outdated
Show resolved
Hide resolved
@@ -236,79 +236,79 @@ private Delegate CreateGetterDelegate<TValue>(int col) | |||
private ValueGetter<bool> CreateBooleanGetterDelegate(ColInfo colInfo) | |||
{ | |||
int columnIndex = GetColumnIndex(colInfo); | |||
return (ref bool value) => value = DataReader.GetBoolean(columnIndex); | |||
return (ref bool value) => value = DataReader.IsDBNull(columnIndex) ? default : DataReader.GetBoolean(columnIndex); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think blindly turning null
into default
is really the correct thing to do here.
I wonder if we should have optional behaviors that the user can opt into. Potentially something like:
- (default behavior) throw on
null
s so the user knows they have to make some decision. - Turn
null
intodefault
. - Convert nullable integer columns into
float
/double
, and useNaN
to designatenull
values. They can then use the Replace N/A transforms available to them in the rest of the pipeline. - The user can always change their schema (either inserting the data into a different table and replacing
null
as appropriate, creating a special stored proc or SELECT statement to do thenull
conversion) as an option to get around the exception as well.
See @TomFinley's comments at #673 (comment) for more thoughts here.
@codemzs @ebarsoumMS - any thoughts on the appropriate behavior here?
This updates the
DatabaseLoaderCursor
to support nullable columns by treating them asdefault
.