Shorten names. Implement string columns and computations #2660

pgovind · 2019-04-23T21:28:18Z

More work towards #26854 to add a DataFrame type to .NET Core

This patch:
Improves binary operations on primitive columns
Creates a StringColumn to support columns of strings. It also implements some meaningful binary ops on string columns
Implements basic computations on primitive columns

I added another patch that implements "DataFrame.Sort" too. The changes are mostly new and incremental. Apologies for putting it in this patch and making the review harder. I'm on app building for the rest of the week, so I wanted to get this out soon

eerhardt · 2019-04-25T14:37:59Z

using System;

need a copyright header on every code file.

Refers to: src/Microsoft.Data/StringColumn.cs:1 in 63bd555. [](commit_id = 63bd555, deletion_comment = False)

src/Microsoft.Data/StringColumn.cs

eerhardt · 2019-04-25T14:44:55Z

src/Microsoft.Data/StringColumn.BinaryOperations.cs

+            // TODO: Using indexing is VERY inefficient here. Each indexer call will find the "right" buffer and then return the value
+            if (Length != column.Length)
+            {
+                throw new ArgumentException($"Column lengths are mismatched", nameof(column));


Should we be adding the exception messages (and any other localizable strings) to a .resx file from the start? It will be easier to add them now, then go through all the strings and try to do it after the fact.

Oh good idea. I'll do that later this week, once I'm done with app building

src/Microsoft.Data/StringColumn.BinaryOperations.cs

eerhardt · 2019-04-25T14:47:17Z

src/Microsoft.Data/StringColumn.BinaryOperations.cs

+            {
+                IList<string> buffer = stringBuffers[i];
+                int bufferLen = buffer.Count;
+                for (int j = 0; j < bufferLen; j++)


Why does the Equals method above use a single for loop over Length and the index, but here we are doing 2 for loops over the stringBuffers?

I wrote that initially thinking the 2 for loop version may be more optimized since I'm avoiding indexing. I changed it now to use the single for loop though. When I fix #2659 though, the single loop version should be super efficient.

src/Microsoft.Data/StringColumn.cs

eerhardt · 2019-04-25T14:49:45Z

tests/Microsoft.Data.DataFrame.Tests/DataFrameTests.cs

+        }
+
+        [Fact]
+        public void TestIndexer()


We need to write some more tests. I don't think these tests cover the amount of code that is in the library.

eerhardt · 2019-04-25T14:51:20Z

src/Microsoft.Data/PrimitiveColumnComputations.cs

+    internal static class PrimitiveColumnComputation<T>
+        where T : struct
+    {
+        public static IPrimitiveColumnComputation<T> Instance => PrimitiveColumnComputation.GetComputation<T>();


This will new up the Computation object every time it is called. That seems wasteful. Instead, we should cache it in a field.

Really? Because this is static, shouldn't this new up a computation object once per T and reuse that for next time?

shouldn't this new up a computation object once per T and reuse that for next time?

I would think that is the behavior we want, but the code you've written doesn't do that.

public static IPrimitiveColumnComputation<T> Instance => PrimitiveColumnComputation.GetComputation<T>();

That is short-hand for:

public static IPrimitiveColumnComputation<T> Instance { get { return PrimitiveColumnComputation.GetComputation<T>(); } }

Which means every time the property is invoked/called it will call GetComputation<T>(), which in turn returns a new object every time it is invoked.

=> means execute the following code each time. = means assignment. You need the last one to initialize the member only once when the static constructor is called.

Suggested change

public static IPrimitiveColumnComputation<T> Instance => PrimitiveColumnComputation.GetComputation<T>();

public static readonly IPrimitiveColumnComputation<T> Instance = PrimitiveColumnComputation.GetComputation<T>();

However, the suggested change is turning Instance from a property into a field. And as written above, allows the field to be publicly settable.

In general, we shouldn't be exposing fields publicly unless there is a strong reason for it. We should keep it a read-only property. But it should reuse the same object each time it is invoked, so it doesn't create unnecessary allocations.

Yep, fixed. Thanks!

Should this rule be applied to internal classes?

I missed that it was an internal class. That does change the "property or field" decision.

It is probably personal preference, but I generally tend to keep all fields (static or instance) private by default and exposed through properties. And only expose them as fields if there are specific reasons for it.

src/Microsoft.Data/PrimitiveColumnComputations.cs

eerhardt · 2019-04-25T14:58:15Z

src/Microsoft.Data/PrimitiveColumn.cs

+namespace Microsoft.Data
+{
+    /// <summary>
+    /// A column to hold primitive values such as int, float etc. Other value types are not really supported


What does Other value types are not really supported mean?

I wanted to say that only primitive types are really supported. A user defined struct for example will throw if some binary op is called. I removed the last sentence and changed it to now say "A column to hold primitive types such as int, float etc"

src/Microsoft.Data/PrimitiveColumn.cs

eerhardt · 2019-04-25T15:00:32Z

src/Microsoft.Data/PrimitiveColumn.cs

+            }
+        }
+
+        // This method involves boxing


Do we want to have T based indexers as well? That way we can avoid boxing in certain situations.

First thing I tried. I don't think generic indexers are supported in C# yet. There's an issue for it here:

dotnet/csharplang#2054

PrimitiveColumn<T> is already generic. So you can return T here. Just like we do in List<T>.

https://github.com/dotnet/corefx/blob/a26fd9bb1b17510d51a9b5a2ce7aadc1a5b631ee/src/Common/src/CoreLib/System/Collections/Generic/List.cs#L145-L167

The csharplang feature you referred to is for when you want to have a generic indexer that isn't in a generic class (or the indexer returns a different type than what the class is generic for).

Right, but I'm overriding a virtual method from BaseColumn though? Unless I'm not seeing something, I would have to make BaseColumn's indexer generic and then deal with type conversions in the derived class? I thought boxing to return a single value seemed acceptable in this instance

What I am thinking is adding a new indexer on the derived class that is typed as T.

At first I was thinking:

public class BaseColumn { public virtual object this[long rowIndex] { get => throw new NotImplementedException(); set => throw new NotImplementedException(); } } public class PrimitiveColumn<T> : BaseColumn where T : struct { public override object this[long rowIndex] { get => base[rowIndex]; set => base[rowIndex] = value; } public new T this[long rowIndex] { get => default; set => base[rowIndex] = value; } }

But that doesn't compile:

CS0111 Type 'PrimitiveColumn' already defines a member called 'this' with the same parameter types

So instead, we could do:

public class BaseColumn { public object this[long rowIndex] { get => GetValue(rowIndex); set => SetValue(rowIndex, value); } protected virtual object GetValue(long rowIndex) => throw new NotImplementedException(); protected virtual void SetValue(long rowIndex, object value) => throw new NotImplementedException(); } public class PrimitiveColumn<T> : BaseColumn where T : struct { protected override object GetValue(long rowIndex) { return base.GetValue(rowIndex); } protected override void SetValue(long rowIndex, object value) { base.SetValue(rowIndex, value); } public new T this[long rowIndex] { get => default; set => base[rowIndex] = value; } }

This allows us to not box/unbox when the column is casted to a PrimitiveColumn<T>.

eerhardt · 2019-04-25T15:01:09Z

<#@ template debug="false" hostspecific="false" language="C#" #>

What's the difference between this file and PrimitiveColumnComputations.tt?

Refers to: src/Microsoft.Data/PrimitiveColumn.Computations.tt:1 in 63bd555. [](commit_id = 63bd555, deletion_comment = False)

eerhardt · 2019-04-25T15:02:47Z

    #region Binary Operations

Do we need this region? Everything in this file is a binary operation.

Refers to: src/Microsoft.Data/PrimitiveColumn.BinaryOperations.cs:16 in 63bd555. [](commit_id = 63bd555, deletion_comment = False)

eerhardt · 2019-04-25T15:05:20Z

            case Type boolType when boolType == typeof(bool):

You can just switch on column and use case PrimitiveColumn<bool> boolColumn, can't you?

If not, you should be able to use pattern matching by simply saying if (column is PrimitiveColumn<bool> boolColumn)

Refers to: src/Microsoft.Data/PrimitiveColumn.BinaryOperations.cs:21 in 63bd555. [](commit_id = 63bd555, deletion_comment = False)

eerhardt · 2019-04-25T15:13:12Z

        switch (typeof(T))

Another possible pattern that could be used here is the same approach that ML.NET uses for conversions.

https://github.com/dotnet/machinelearning/blob/master/src/Microsoft.ML.Data/Data/Conversion.cs#L65-L67

Where you have a map of (Type, Type) tuples that map to Delegates that perform the function. So here we would have a delegate that would take in 2 ints and divide them. Then another that would take an int and a short and divide them. Two doubles. A double and a float, etc. etc.

Refers to: src/Microsoft.Data/PrimitiveColumn.BinaryOperations.cs:985 in 63bd555. [](commit_id = 63bd555, deletion_comment = False)

eerhardt · 2019-04-25T15:15:15Z

src/Microsoft.Data/DataFrameTable.cs

        }

-        private DataFrameTable(IList<BaseDataFrameColumn> columns)
+        public DataFrameTable(IList<BaseColumn> columns)


~~Why is this now public?~~

Nevermind, the whole class is internal

What criteria do we use to choose between public and internal visibility on members, on an internal class? Assuming the member is not impelmenting an interface.

Good question: In this case it doesn't matter if this constructor public or internal. I just chose public, there was no particular reason to do so.

src/Microsoft.Data/DataFrame.cs

eerhardt · 2019-04-25T15:19:23Z

Is the ColumnArithmeticTemplate.cs file necessary? It is empty.

src/Microsoft.Data/BaseColumn.cs

src/Microsoft.Data/StringColumn.cs

src/Microsoft.Data/StringColumn.BinaryOperations.cs

eerhardt · 2019-05-10T22:22:27Z

src/Microsoft.Data/StringColumn.BinaryOperations.cs

+{
+    public partial class StringColumn : BaseColumn
+    {
+        public override BaseColumn Add(BaseColumn column)


Do we want strongly typed versions of these methods? That way if I have a StringColumn, and I call one of these methods, I should get back a StringColumn, not?

Hmmm, so at the moment, I've taken the less strongly typed approach and since one of the main use cases is notebooks, it pairs well with returning a copy everytime(since I don't know the return type until runtime). In the case of a StringColumn, Add will always returns a StringColumn, but for intColumn + 5.0, the result will be a PrimitiveColumn<double> for example. To enable the strong typed versions, I would have to define APIs for all possible combinations of all columns. How about I consider that in a separate issue? Since these methods are general, it's easy enough to build the strong APIs on top of this?

Yes, a separate issue for this sounds good to me.

As for every combination of type, I don't think I'd go that far. But for the case where I have a strongly-typed string or Int32 column, if I add a string or Int32 to that column, I know I get back a string or Int32.

The analogy here is this code:

string a = "foo"; string b = "bar"; ??? c = a + b;

It's pretty obvious that c should be a string, not object.

src/Microsoft.Data/DecimalConverter.cs

src/Microsoft.Data/Microsoft.Data.DataFrame.csproj

eerhardt · 2019-05-10T22:29:16Z

src/Microsoft.Data/PrimitiveColumnArithmetic.tt

        where T : struct
    {
-        public static IPrimitiveDataFrameColumnArithmetic<T> Instance => PrimitiveDataFrameColumnArithmetic.GetArithmetic<T>();
+        public static IPrimitiveColumnArithmetic<T> Instance = PrimitiveColumnArithmetic.GetArithmetic<T>();


How come this one is a field, but PrimitiveColumnComputations is a property? It's nice to be consistent in these kinds of things.

ahsonkhan · 2019-05-14T02:57:41Z

FYI, this build is failing. GitHub just isn't showing the results properly from the CI legs: https://dev.azure.com/dnceng/public/_build/results?buildId=183617

ahsonkhan · 2019-05-14T20:15:22Z

/azp run

azure-pipelines · 2019-05-14T20:15:36Z

Azure Pipelines successfully started running 1 pipeline(s).

… BinaryOperationsExtensions

pgovind · 2019-05-15T20:30:11Z

src/Microsoft.Data/BaseColumn.cs

+
+        public object this[long startIndex, int length]
+        {
+            get => GetValue(startIndex, length);


Just as a note: On a notebook, most calls such as the following will still box at the moment:
var intColumn = df.Column('intColumn'); //intColumn is a PrimitiveColumn<int>, but returned as a BaseColumn var thisWillBox = intColumn[2];

src/Microsoft.Data/Microsoft.Data.DataFrame.csproj

pgovind · 2019-05-16T00:27:12Z

src/Microsoft.Data/PrimitiveColumn.Sort.cs

+
+        internal override BaseColumn GetAscendingSortIndices()
+        {
+            // Is Comparer<T>.Default guaranteed to sort in ascending order?


This is guaranteed for ALL IComparer implementations.

See https://docs.microsoft.com/en-us/dotnet/api/system.collections.generic.icomparer-1.compare?view=netframework-4.8

Returns: A signed integer that indicates the relative values of x and y, as shown in the following table.

Value Meaning

Less than zero x is less than y.

Zero x equals y.

Greater than zero x is greater than y.

You can remove this comment (in the next PR if you want)

In reply to: 284780397 [](ancestors = 284780397)

pgovind · 2019-05-16T07:45:48Z

src/Microsoft.Data/PrimitiveColumn.cs

+
+        public new T this[long startIndex, int length]
+        {
+            get => (T)GetValue(startIndex, length);


This will still box. Need to replace this with the code inside GetValue itself

Yes - that is the point of these "new" strongly typed members. No boxing should be necessary.

eerhardt · 2019-05-16T23:15:14Z

src/Microsoft.Data/DataFrame.cs

+        public DataFrame Sort(string columnName, bool ascending = true)
+        {
+            BaseColumn column = this[columnName];
+            BaseColumn sortIndices = column.GetAscendingSortIndices();


we should only call public BaseColumn methods so this works with external derived classes. As it exists today - this will throw an exception because there is no way for an external column to override an internal method.

eerhardt · 2019-05-16T23:19:55Z

src/Microsoft.Data/Microsoft.Data.DataFrame.csproj

    </Compile>
  </ItemGroup>

+  <ItemGroup>
+    <EmbeddedResource Update="Strings.resx">


I think the casing needs to match the actual file casing - is it possible to rename the file?

See https://stackoverflow.com/questions/1793735/change-case-of-a-file-on-windows for how to do this on Windows.

eerhardt · 2019-05-16T23:22:41Z

src/Microsoft.Data/StringColumn.cs

+            }
+            else
+            {
+                throw new ArgumentException(Strings.MismatchedValueType + " string", nameof(value));


When throwing exceptions with a message that comes from a resource, you shouldn't concat strings. Instead use formatted strings - {0}. The reason is because when translating to other languages, the hard-coded type name may need to move in the sentence to make sense in the language.

eerhardt · 2019-05-16T23:23:16Z

src/Microsoft.Data/StringColumn.cs

+
+        public new string this[long rowIndex]
+        {
+            get => (string)GetValue(rowIndex);


Is it possible to implement this without the cast?

eerhardt

This is looking good. Keep the good work coming!

feel free to merge now, and address my additional comments in a separate PR.

* Shorten names, implement sort, binary op extensions and computations

pgovind requested review from eerhardt and ericstj April 23, 2019 21:28

pgovind added the area-Microsoft.Data label Apr 23, 2019