Skip to content
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

Updating DatabaseLoader to support getting column info from a given .NET type. #4091

Merged
merged 4 commits into from Aug 12, 2019

Conversation

@tannergooding
Copy link
Member

commented Aug 7, 2019

This adds limited support for determining the column info from a .NET type in order to match what TextLoader provides.

return DbType.Boolean;
}

case InternalDataKind.DT:

This comment has been minimized.

Copy link
@eerhardt

eerhardt Aug 8, 2019

Member

What about DateTimeOffset (InternalDataKind.DZ) and TimeSpan (InternalDataKind.TS) ?

This comment has been minimized.

Copy link
@tannergooding

tannergooding Aug 8, 2019

Author Member

There is no equivalent for DbKind

This comment has been minimized.

Copy link
@eerhardt

This comment has been minimized.

Copy link
@tannergooding

tannergooding Aug 8, 2019

Author Member

The latter is the SQL time type, not an equivalent to the System.TimeSpan type; I'm not sure what the appropriate conversion is. I missed DateTimeOffset.

This comment has been minimized.

Copy link
@eerhardt

eerhardt Aug 8, 2019

Member

The latter is the SQL time type, not an equivalent to the System.TimeSpan type;

According to https://docs.microsoft.com/en-us/dotnet/framework/data/adonet/sql-server-data-type-mappings, it is the appropriate mapping:

SQL Server Database Engine type .NET Framework type SqlDbType enumeration SqlDataReader SqlTypes typed accessor DbType enumeration SqlDataReader DbType typed accessor
time(SQL Server 2008 and later) TimeSpan Time none Time GetDateTime

This comment has been minimized.

Copy link
@tannergooding

tannergooding Aug 8, 2019

Author Member

Is that also the appropriate mapping for all database types, not just SQL?

Also, it looks like it isn't even the SQL time type, it is the SQL datetime type and there is a separate value in a separate enum that should be used for SQL time:

| Time | 17 | A type representing a SQL Server DateTime value. If you want to use a SQL Server time value, use Time. |

This comment has been minimized.

Copy link
@tannergooding

tannergooding Aug 8, 2019

Author Member

ah, right.
These weren't done because DbDataReader doesn't expose Get methods for them

@tannergooding tannergooding force-pushed the tannergooding:DatabaseLoader branch from dcf2790 to d785db4 Aug 12, 2019

public int? Max;

/// <summary>
/// Whether this range extends to the end of the line, but should be a fixed number of items.

This comment has been minimized.

Copy link
@eerhardt

eerhardt Aug 12, 2019

Member

What does "to the end of the line" mean to a database loader?

I think we should remove these options until we decide we have data that says they are required. That way they don't "sneak" in to the product, and don't do anything.

This comment has been minimized.

Copy link
@tannergooding

tannergooding Aug 12, 2019

Author Member

In this case it would mean to the end of the table. I'm pretty sure it is used for vector support.

@tannergooding

This comment has been minimized.

Copy link
Member Author

commented Aug 12, 2019

@eerhardt, any other feedback here or can it be merged and I can start on getting the vector support up?

@eerhardt

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

@eerhardt, any other feedback here or can it be merged

I don't believe that this feedback has been addressed:

I think we should remove these options until we decide we have data that says they are required.

We shouldn't be adding public options that aren't being used. You can easily add these new options if and when they get supported.

@tannergooding

This comment has been minimized.

Copy link
Member Author

commented Aug 12, 2019

We shouldn't be adding public options that aren't being used. You can easily add these new options if and when they get supported.

They are going to be used in the next PR which adds the vector support. Is it really worthwhile to pull it out and retrigger CI just for it to be used and re-added in the following PR?

@eerhardt eerhardt merged commit e34f2e1 into dotnet:master Aug 12, 2019

17 checks passed

MachineLearning-CI Build #20190812.2 succeeded
Details
MachineLearning-CI (Centos_x64_NetCoreApp30 Debug_Build) Centos_x64_NetCoreApp30 Debug_Build succeeded
Details
MachineLearning-CI (Centos_x64_NetCoreApp30 Release_Build) Centos_x64_NetCoreApp30 Release_Build succeeded
Details
MachineLearning-CI (MacOS_x64_NetCoreApp21 Debug_Build) MacOS_x64_NetCoreApp21 Debug_Build succeeded
Details
MachineLearning-CI (MacOS_x64_NetCoreApp21 Release_Build) MacOS_x64_NetCoreApp21 Release_Build succeeded
Details
MachineLearning-CI (Ubuntu_x64_NetCoreApp21 Debug_Build) Ubuntu_x64_NetCoreApp21 Debug_Build succeeded
Details
MachineLearning-CI (Ubuntu_x64_NetCoreApp21 Release_Build) Ubuntu_x64_NetCoreApp21 Release_Build succeeded
Details
MachineLearning-CI (Windows_x64_NetCoreApp21 Debug_Build) Windows_x64_NetCoreApp21 Debug_Build succeeded
Details
MachineLearning-CI (Windows_x64_NetCoreApp21 Release_Build) Windows_x64_NetCoreApp21 Release_Build succeeded
Details
MachineLearning-CI (Windows_x64_NetCoreApp30 Debug_Build) Windows_x64_NetCoreApp30 Debug_Build succeeded
Details
MachineLearning-CI (Windows_x64_NetCoreApp30 Release_Build) Windows_x64_NetCoreApp30 Release_Build succeeded
Details
MachineLearning-CI (Windows_x64_NetFx461 Debug_Build) Windows_x64_NetFx461 Debug_Build succeeded
Details
MachineLearning-CI (Windows_x64_NetFx461 Release_Build) Windows_x64_NetFx461 Release_Build succeeded
Details
MachineLearning-CI (Windows_x86_NetCoreApp21 Debug_Build) Windows_x86_NetCoreApp21 Debug_Build succeeded
Details
MachineLearning-CI (Windows_x86_NetCoreApp21 Release_Build) Windows_x86_NetCoreApp21 Release_Build succeeded
Details
WIP Ready for review
Details
license/cla All CLA requirements met.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.