-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Replace DvInt* with .NET standard data types. WIP #683
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
Changes from 13 commits
9ba4dab
baf0696
f7a0fff
1fdfe0d
734540f
10d753c
46299b8
a99831b
a0ceabb
f30dd7e
a6e5c75
c4bae95
9aea10b
db2da0b
5d14c0a
9051ef9
789182a
4e70eb4
ef54c60
83dec91
b3ba06d
fb9f792
25d1a52
1853471
66fa9be
f17b7b0
0c8fb8c
f16b0a9
9c128a3
9c4eebf
d7ba332
63e45e7
39f150b
11be480
681b3ef
1a38dfe
7580513
3c5ce8e
8f8e544
3fb1cc2
c844b4e
53b4e57
f1d1da6
f05e2f1
33b7150
d028702
9a4f192
a7cd3d8
40c01fc
ad37fb5
b9edc92
078cd8c
be3d177
cd7de9f
7e80e50
9d4f4d6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -292,42 +292,22 @@ private Action<TRow> GenerateSetter(IRow input, int index, InternalSchemaDefinit | |
else if (fieldType.GetElementType() == typeof(int)) | ||
{ | ||
Ch.Assert(colType.ItemType == NumberType.I4); | ||
return CreateConvertingVBufferSetter<DvInt4, int>(input, index, poke, peek, x => (int)x); | ||
} | ||
else if (fieldType.GetElementType() == typeof(int?)) | ||
{ | ||
Ch.Assert(colType.ItemType == NumberType.I4); | ||
return CreateConvertingVBufferSetter<DvInt4, int?>(input, index, poke, peek, x => (int?)x); | ||
return CreateConvertingVBufferSetter<int, int>(input, index, poke, peek, x => (int)x); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
These were previously necessary when the types are different. Why are they necessary now? For example I definitely do not see things for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. agreed, this was a miss. No need for this conversion. In reply to: 212031778 [](ancestors = 212031778) |
||
} | ||
else if (fieldType.GetElementType() == typeof(short)) | ||
{ | ||
Ch.Assert(colType.ItemType == NumberType.I2); | ||
return CreateConvertingVBufferSetter<DvInt2, short>(input, index, poke, peek, x => (short)x); | ||
} | ||
else if (fieldType.GetElementType() == typeof(short?)) | ||
{ | ||
Ch.Assert(colType.ItemType == NumberType.I2); | ||
return CreateConvertingVBufferSetter<DvInt2, short?>(input, index, poke, peek, x => (short?)x); | ||
return CreateConvertingVBufferSetter<short, short>(input, index, poke, peek, x => x); | ||
} | ||
else if (fieldType.GetElementType() == typeof(long)) | ||
{ | ||
Ch.Assert(colType.ItemType == NumberType.I8); | ||
return CreateConvertingVBufferSetter<DvInt8, long>(input, index, poke, peek, x => (long)x); | ||
} | ||
else if (fieldType.GetElementType() == typeof(long?)) | ||
{ | ||
Ch.Assert(colType.ItemType == NumberType.I8); | ||
return CreateConvertingVBufferSetter<DvInt8, long?>(input, index, poke, peek, x => (long?)x); | ||
return CreateConvertingVBufferSetter<long, long>(input, index, poke, peek, x => x); | ||
} | ||
else if (fieldType.GetElementType() == typeof(sbyte)) | ||
{ | ||
Ch.Assert(colType.ItemType == NumberType.I1); | ||
return CreateConvertingVBufferSetter<DvInt1, sbyte>(input, index, poke, peek, x => (sbyte)x); | ||
} | ||
else if (fieldType.GetElementType() == typeof(sbyte?)) | ||
{ | ||
Ch.Assert(colType.ItemType == NumberType.I1); | ||
return CreateConvertingVBufferSetter<DvInt1, sbyte?>(input, index, poke, peek, x => (sbyte?)x); | ||
return CreateConvertingVBufferSetter<sbyte, sbyte>(input, index, poke, peek, x => x); | ||
} | ||
|
||
// VBuffer<T> -> T[] | ||
|
@@ -373,49 +353,25 @@ private Action<TRow> GenerateSetter(IRow input, int index, InternalSchemaDefinit | |
{ | ||
Ch.Assert(colType == NumberType.I4); | ||
Ch.Assert(peek == null); | ||
return CreateConvertingActionSetter<DvInt4, int>(input, index, poke, x => (int)x); | ||
} | ||
else if (fieldType == typeof(int?)) | ||
{ | ||
Ch.Assert(colType == NumberType.I4); | ||
Ch.Assert(peek == null); | ||
return CreateConvertingActionSetter<DvInt4, int?>(input, index, poke, x => (int?)x); | ||
return CreateConvertingActionSetter<int, int>(input, index, poke, x => x); | ||
} | ||
else if (fieldType == typeof(short)) | ||
{ | ||
Ch.Assert(colType == NumberType.I2); | ||
Ch.Assert(peek == null); | ||
return CreateConvertingActionSetter<DvInt2, short>(input, index, poke, x => (short)x); | ||
} | ||
else if (fieldType == typeof(short?)) | ||
{ | ||
Ch.Assert(colType == NumberType.I2); | ||
Ch.Assert(peek == null); | ||
return CreateConvertingActionSetter<DvInt2, short?>(input, index, poke, x => (short?)x); | ||
return CreateConvertingActionSetter<short, short>(input, index, poke, x => x); | ||
} | ||
else if (fieldType == typeof(long)) | ||
{ | ||
Ch.Assert(colType == NumberType.I8); | ||
Ch.Assert(peek == null); | ||
return CreateConvertingActionSetter<DvInt8, long>(input, index, poke, x => (long)x); | ||
} | ||
else if (fieldType == typeof(long?)) | ||
{ | ||
Ch.Assert(colType == NumberType.I8); | ||
Ch.Assert(peek == null); | ||
return CreateConvertingActionSetter<DvInt8, long?>(input, index, poke, x => (long?)x); | ||
return CreateConvertingActionSetter<long, long>(input, index, poke, x => x); | ||
} | ||
else if (fieldType == typeof(sbyte)) | ||
{ | ||
Ch.Assert(colType == NumberType.I1); | ||
Ch.Assert(peek == null); | ||
return CreateConvertingActionSetter<DvInt1, sbyte>(input, index, poke, x => (sbyte)x); | ||
} | ||
else if (fieldType == typeof(sbyte?)) | ||
{ | ||
Ch.Assert(colType == NumberType.I1); | ||
Ch.Assert(peek == null); | ||
return CreateConvertingActionSetter<DvInt1, sbyte?>(input, index, poke, x => (sbyte?)x); | ||
return CreateConvertingActionSetter<sbyte, sbyte>(input, index, poke, x => x); | ||
} | ||
// T -> T | ||
if (fieldType.IsGenericType && fieldType.GetGenericTypeDefinition() == typeof(Nullable<>)) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -567,14 +567,18 @@ public static BoolType Instance | |
get | ||
{ | ||
if (_instance == null) | ||
Interlocked.CompareExchange(ref _instance, new BoolType(), null); | ||
Interlocked.CompareExchange(ref _instance, new BoolType(DataKind.BL, "Bool"), null); | ||
return _instance; | ||
} | ||
} | ||
|
||
private BoolType() | ||
: base(typeof(DvBool), DataKind.BL) | ||
private readonly string _name; | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmmm. What's this for? Is it still relevant? #Closed |
||
private BoolType(DataKind kind, string name) | ||
: base(kind.ToType(), kind) | ||
{ | ||
Contracts.AssertNonEmpty(name); | ||
_name = name; | ||
} | ||
|
||
public override bool Equals(ColumnType other) | ||
|
@@ -587,7 +591,7 @@ public override bool Equals(ColumnType other) | |
|
||
public override string ToString() | ||
{ | ||
return "Bool"; | ||
return _name; | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -55,7 +55,7 @@ public enum DataKind : byte | |
public static class DataKindExtensions | ||
{ | ||
public const DataKind KindMin = DataKind.I1; | ||
public const DataKind KindLim = DataKind.UG + 1; | ||
public const DataKind KindLim = DataKind.U16 + 1; | ||
public const int KindCount = KindLim - KindMin; | ||
|
||
/// <summary> | ||
|
@@ -141,19 +141,19 @@ public static Type ToType(this DataKind kind) | |
switch (kind) | ||
{ | ||
case DataKind.I1: | ||
return typeof(DvInt1); | ||
return typeof(sbyte); | ||
case DataKind.U1: | ||
return typeof(byte); | ||
case DataKind.I2: | ||
return typeof(DvInt2); | ||
return typeof(short); | ||
case DataKind.U2: | ||
return typeof(ushort); | ||
case DataKind.I4: | ||
return typeof(DvInt4); | ||
return typeof(int); | ||
case DataKind.U4: | ||
return typeof(uint); | ||
case DataKind.I8: | ||
return typeof(DvInt8); | ||
return typeof(long); | ||
case DataKind.U8: | ||
return typeof(ulong); | ||
case DataKind.R4: | ||
|
@@ -185,25 +185,25 @@ public static bool TryGetDataKind(this Type type, out DataKind kind) | |
Contracts.CheckValueOrNull(type); | ||
|
||
// REVIEW: Make this more efficient. Should we have a global dictionary? | ||
if (type == typeof(DvInt1) || type == typeof(sbyte) || type == typeof(sbyte?)) | ||
if (type == typeof(sbyte)) | ||
kind = DataKind.I1; | ||
else if (type == typeof(byte) || type == typeof(byte?)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is "byte?" mapped to a U1? I thought U1 just mapped to a byte and did not support nullables ....this needs to go away anyways In reply to: 210963225 [](ancestors = 210963225) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Someone made a mistake somewhere, didn't realize this was supposed to be inverse to above function. Who knows who, but we ought to correct now. In reply to: 210965477 [](ancestors = 210965477,210963225) |
||
kind = DataKind.U1; | ||
else if (type == typeof(DvInt2)|| type== typeof(short) || type == typeof(short?)) | ||
else if (type == typeof(short)) | ||
kind = DataKind.I2; | ||
else if (type == typeof(ushort)|| type == typeof(ushort?)) | ||
else if (type == typeof(ushort) || type == typeof(ushort?)) | ||
kind = DataKind.U2; | ||
else if (type == typeof(DvInt4) || type == typeof(int)|| type == typeof(int?)) | ||
else if (type == typeof(int)) | ||
kind = DataKind.I4; | ||
else if (type == typeof(uint)|| type == typeof(uint?)) | ||
else if (type == typeof(uint) || type == typeof(uint?)) | ||
kind = DataKind.U4; | ||
else if (type == typeof(DvInt8) || type==typeof(long)|| type == typeof(long?)) | ||
else if (type == typeof(long)) | ||
kind = DataKind.I8; | ||
else if (type == typeof(ulong)|| type == typeof(ulong?)) | ||
else if (type == typeof(ulong) || type == typeof(ulong?)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The use of nulltable types for the unsigned types seems wrong. #Closed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually this entire function is wrong. This is meant to be an inverse to the function above. Didn't we already talk about this in a prior PR months ago? In reply to: 211039836 [](ancestors = 211039836) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So: please remove all mentions of any types (whether nullable or not) that do not appear in the earlier function. However probably there's some code somewhere that relied on that change, try to figure out what it was, and fix it to use its own utility function. In reply to: 211040069 [](ancestors = 211040069,211039836) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @TomFinley, I believe this was added by @Ivanidzo4ka in #555 and you also approved that PR. I agree nullables here is wrong and I will remove them all. #Resolved There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. https://getyarn.io/yarn-clip/fc2be4a1-ca56-450b-8cbb-f771207b3df1 Anyway good. Not just the nullables but all but the single correct type. 😄 #Closed |
||
kind = DataKind.U8; | ||
else if (type == typeof(Single)|| type == typeof(Single?)) | ||
else if (type == typeof(Single) || type == typeof(Single?)) | ||
kind = DataKind.R4; | ||
else if (type == typeof(Double)|| type == typeof(Double?)) | ||
else if (type == typeof(Double) || type == typeof(Double?)) | ||
kind = DataKind.R8; | ||
else if (type == typeof(DvText)) | ||
kind = DataKind.TX; | ||
|
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.
Why do we need a "converting" delegate here to return the same value? #Resolved