Skip to content

Fix Apply in PrimitiveColumnContainer so it does not change source column #6642

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
May 5, 2023
Merged

Fix Apply in PrimitiveColumnContainer so it does not change source column #6642

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 6 additions & 14 deletions src/Microsoft.Data.Analysis/PrimitiveColumnContainer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,31 +257,23 @@ public void Apply<TResult>(Func<T?, TResult?> func, PrimitiveColumnContainer<TRe
{
for (int b = 0; b < Buffers.Count; b++)
{
ReadOnlyDataFrameBuffer<T> buffer = Buffers[b];
long prevLength = checked(Buffers[0].Length * b);
DataFrameBuffer<T> mutableBuffer = DataFrameBuffer<T>.GetMutableBuffer(buffer);
Buffers[b] = mutableBuffer;
Span<T> span = mutableBuffer.Span;
DataFrameBuffer<byte> mutableNullBitMapBuffer = DataFrameBuffer<byte>.GetMutableBuffer(NullBitMapBuffers[b]);
NullBitMapBuffers[b] = mutableNullBitMapBuffer;
Span<byte> nullBitMapSpan = mutableNullBitMapBuffer.Span;
ReadOnlyDataFrameBuffer<T> sourceBuffer = Buffers[b];
Copy link
Contributor

@JakeRadMSFT JakeRadMSFT May 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@agocke @jaredpar

Just for my own learning purposes. Is this something that would be improved with the span changes in .NET? Would we no longer need to do all the GetMutableBuffer stuff?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it doesn't because it actually does a copy?

https://github.com/dotnet/machinelearning/blob/d974a212538884bbffa0d95c82bf85de44a55782/src/Microsoft.Data.Analysis/DataFrameBuffer.cs#LL43C13-L44C36

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Specifically I'm wondering if there would be a better way to refactor out the code like this:

DataFrameBuffer<TResult> resultMutableBuffer = DataFrameBuffer<TResult>.GetMutableBuffer(resultBuffer);
                resultContainer.Buffers[b] = resultMutableBuffer;
                Span<TResult> resultSpan = resultMutableBuffer.Span;
                DataFrameBuffer<byte> resultMutableNullBitMapBuffer = DataFrameBuffer<byte>.GetMutableBuffer(resultContainer.NullBitMapBuffers[b]);
                resultContainer.NullBitMapBuffers[b] = resultMutableNullBitMapBuffer;
                Span<byte> resultNullBitMapSpan = resultMutableNullBitMapBuffer.Span;

It's all over this file in some form.
Clone, Apply, ApplyElementWise, CloneAs

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a lot of very questionable stuff going on here. The code is going to great lengths to copy the original buffer to get a mutable version... and then passing the span to IsValid or calling the indexer, which both accept ReadOnlySpan.

And then there's the ReadOnlyDataFrameBuffer<T> type which takes a T : struct, but should be T : unmanaged.

This code could likely be simplified by extracting some helper methods, but I think the above issues indicate that someone should review the actual intent and ensure that the code reflects it.

Copy link
Contributor

@JakeRadMSFT JakeRadMSFT May 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's sort of what it felt like but I appreciate having some expert eyes on it! Thanks @agocke!

I might take this up as a pet project in the future :)

Copy link
Contributor

@JakeRadMSFT JakeRadMSFT May 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@agocke - I'm not sure it's 100% of the way there but I did a pass and opened a different PR.

https://github.com/dotnet/machinelearning/pull/6656/files

Copy link
Contributor

@asmirnov82 asmirnov82 May 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that PR already closed, but I have some comments regarding this GetMutableBuffer staff. It looks it's used for working with readonly memory, when column is created from the Apache Arrow RecordBatch in

public static DataFrame FromArrowRecordBatch(RecordBatch recordBatch)
{
    DataFrame ret = new DataFrame();
    Apache.Arrow.Schema arrowSchema = recordBatch.Schema;
    int fieldIndex = 0;
    IEnumerable<IArrowArray> arrowArrays = recordBatch.Arrays;
    foreach (IArrowArray arrowArray in arrowArrays)
    {
        Field field = arrowSchema.GetFieldByIndex(fieldIndex);
        AppendDataFrameColumnFromArrowArray(field, arrowArray, ret);
        fieldIndex++;
    }
    return ret;
}

However, this implementation looks very suspicious.

RecordBatch is a disposable object. Apache Arrow by default uses NativeMemoryAllocator to allocate unmanaged memory (for example, this default allocator is used in Spark.Net to create RecorBatch and pass it to DataFrame.FromArrowRecordBatch factory method).
So it's up to a DataFrame to hold the link to the RecordBatch and correctly Dispose it. Or we have to copy the unmanaged readonly memory from the RecordBatch into managed buffers (that exactly what is happening in GetMutableBuffer on attempt to edit data), but in this case we can avoid using ReadOnlyBuffers at all.

ReadOnlySpan<byte> sourceNullBitMap = NullBitMapBuffers[b].ReadOnlySpan;

ReadOnlyDataFrameBuffer<TResult> resultBuffer = resultContainer.Buffers[b];
long resultPrevLength = checked(resultContainer.Buffers[0].Length * b);
DataFrameBuffer<TResult> resultMutableBuffer = DataFrameBuffer<TResult>.GetMutableBuffer(resultBuffer);
resultContainer.Buffers[b] = resultMutableBuffer;
Span<TResult> resultSpan = resultMutableBuffer.Span;
DataFrameBuffer<byte> resultMutableNullBitMapBuffer = DataFrameBuffer<byte>.GetMutableBuffer(resultContainer.NullBitMapBuffers[b]);
resultContainer.NullBitMapBuffers[b] = resultMutableNullBitMapBuffer;
Span<byte> resultNullBitMapSpan = resultMutableNullBitMapBuffer.Span;

for (int i = 0; i < span.Length; i++)
for (int i = 0; i < Buffers[b].Length; i++)
{
long curIndex = i + prevLength;
bool isValid = IsValid(nullBitMapSpan, i);
TResult? value = func(isValid ? span[i] : default(T?));
bool isValid = IsValid(sourceNullBitMap, i);
TResult? value = func(isValid ? sourceBuffer[i] : default(T?));
resultSpan[i] = value.GetValueOrDefault();
SetValidityBit(resultNullBitMapSpan, i, value != null);
resultContainer.SetValidityBit(resultNullBitMapSpan, i, value != null);
}
}
}
Expand Down