Changes to support Arrow 2.0 and Spark 3.0 #711

pgovind · 2020-10-05T20:53:32Z

Fix for #654

Apache arrow changes are here: apache/arrow#8348

Alright, this is ready to review now!

cc @eerhardt @suhsteve

eerhardt · 2020-10-12T15:42:54Z

we need a minor change in MDA

Does this mean we will need to ship a new version of MDA? Is it possible to get that minor change in now?

src/csharp/Microsoft.Spark.E2ETest/IpcTests/Sql/DataFrameTests.cs

src/csharp/Microsoft.Spark.Worker/Command/SqlCommandExecutor.cs

src/csharp/Microsoft.Spark.Worker.UnitTest/CommandExecutorTests.cs

src/csharp/Microsoft.Spark.Worker/Command/SqlCommandExecutor.cs

src/csharp/Microsoft.Spark.E2ETest/IpcTests/Sql/DataFrameTests.cs

src/csharp/Microsoft.Spark.Worker/Command/SqlCommandExecutor.cs

pgovind · 2020-10-21T17:04:17Z

Done, I believe this is ready to review now. We're handling the spark version internally in the Spark worker, so no UDFs have to change.

src/csharp/Microsoft.Spark.Worker/Command/SqlCommandExecutor.cs

src/csharp/Microsoft.Spark/Microsoft.Spark.csproj

…rraystructInternal

eerhardt

Changes look good to me. Thanks for the great work here, @pgovind.

You may want to change the PR from "Draft" now.

pgovind · 2020-10-21T19:04:00Z

You may want to change the PR from "Draft" now.

I'm not sure I can. I think only the repo owners can do that. @rapoth @suhsteve @imback82

suhsteve

LGTM. Just a few minor comments. Thanks @pgovind !

src/csharp/Microsoft.Spark.Worker.UnitTest/CommandExecutorTests.cs

src/csharp/Microsoft.Spark.Worker/Command/SqlCommandExecutor.cs

suhsteve · 2020-10-21T19:39:15Z

@pgovind would it be possible to add a E2E test that uses chained Arrow/FxDataFrame UDFs ?

pgovind · 2020-10-22T17:13:54Z

would it be possible to add a E2E test that uses chained Arrow/FxDataFrame UDFs ?

I think they exist already right? I see chained UDFs being tested in E2ETests/IpcTests/Sql/DataFrameTests.cs?

suhsteve · 2020-10-22T17:24:01Z

would it be possible to add a E2E test that uses chained Arrow/FxDataFrame UDFs ?

I think they exist already right? I see chained UDFs being tested in E2ETests/IpcTests/Sql/DataFrameTests.cs?

Great, I see them now.

suhsteve

some minor nits.

src/csharp/Microsoft.Spark.Worker/Command/SqlCommandExecutor.cs

suhsteve · 2020-10-22T20:19:26Z

src/csharp/Microsoft.Spark.Worker/Command/SqlCommandExecutor.cs


+        private RecordBatch WrapColumnsInStructIfApplicable(RecordBatch batch)
+        {
+            if (_version >= new Version(Versions.V3_0_0))


maybe this instead ?

Suggested change

if (_version >= new Version(Versions.V3_0_0))

if (_version.Major >= 3)

Ha, that's actually what I had first, until I saw this:

spark/src/csharp/Microsoft.Spark.E2ETest/Utils/VersionBasedFacts.cs

Line 14 in 1bcd37b

if (SparkSettings.Version < new Version(version))

.

Are you worried about the new ? I don't mind changing it!!

just a very minor micro optimization. We can keep, I don't mind either. 👍

pgovind · 2020-10-23T18:03:40Z

Just so you know, I don't have merge permissions here :) So one of you will need to merge this PR

imback82

LGTM, thanks for the great work @pgovind!

src/csharp/Microsoft.Spark.Worker/Command/SqlCommandExecutor.cs

Prashanth Govindarajan added 2 commits October 5, 2020 13:49

Turn on GroupedMap unit test

895b2dc

Local changes to test the arrow repo

15e6288

pgovind changed the title ~~Turn on GroupedMap unit test~~ WIP: Turn on GroupedMap unit test Oct 5, 2020

Prashanth Govindarajan added 4 commits October 9, 2020 13:03

Test CI

f5ceed4

Test DataFrame UDF

77c37c5

CHange gitignore

6d23a66

Temp changes

a8e478e

pgovind force-pushed the arraystruct branch from 6d23a66 to 0e9910a Compare October 9, 2020 21:49

DataFrame based unit test

aeaaf0f

pgovind force-pushed the arraystruct branch from 0e9910a to aeaaf0f Compare October 9, 2020 23:09

rapoth marked this pull request as draft October 9, 2020 23:47

Arrow related changes. MDA still needs a new nupkg

317e136

eerhardt reviewed Oct 20, 2020

View reviewed changes

src/csharp/Microsoft.Spark.E2ETest/IpcTests/Sql/DataFrameTests.cs Outdated Show resolved Hide resolved

Wrap columns in a Struct

e7a6293

pgovind commented Oct 20, 2020

View reviewed changes

src/csharp/Microsoft.Spark.E2ETest/IpcTests/Sql/DataFrameTests.cs Outdated Show resolved Hide resolved

pgovind commented Oct 20, 2020

View reviewed changes

src/csharp/Microsoft.Spark.Worker/Command/SqlCommandExecutor.cs Outdated Show resolved Hide resolved

Fix version and arrow based UDFs

13ae385

pgovind changed the title ~~WIP: Turn on GroupedMap unit test~~ Changes to support Arrow 2.0 and Spark 3.0 Oct 20, 2020

sq

329c2d7

eerhardt reviewed Oct 21, 2020

View reviewed changes

src/csharp/Microsoft.Spark.Worker.UnitTest/CommandExecutorTests.cs Outdated Show resolved Hide resolved

eerhardt reviewed Oct 21, 2020

View reviewed changes

src/csharp/Microsoft.Spark.Worker/Command/SqlCommandExecutor.cs Outdated Show resolved Hide resolved

pgovind commented Oct 21, 2020

View reviewed changes

src/csharp/Microsoft.Spark.E2ETest/IpcTests/Sql/DataFrameTests.cs Outdated Show resolved Hide resolved

eerhardt reviewed Oct 21, 2020

View reviewed changes

src/csharp/Microsoft.Spark.Worker/Command/SqlCommandExecutor.cs Outdated Show resolved Hide resolved

eerhardt reviewed Oct 21, 2020

View reviewed changes

src/csharp/Microsoft.Spark.Worker/Command/SqlCommandExecutor.cs Outdated Show resolved Hide resolved

eerhardt reviewed Oct 21, 2020

View reviewed changes

src/csharp/Microsoft.Spark.Worker/Command/SqlCommandExecutor.cs Outdated Show resolved Hide resolved

Prashanth Govindarajan added 4 commits October 20, 2020 17:41

Some cleanup

9013fe9

Comment out a unit test

cae8f01

Comment out another unit test

28ea3f0

Use _version

e2dcd03

sq

bc451fa

Merge branch 'master' into arraystruct

b3be37a

eerhardt reviewed Oct 21, 2020

View reviewed changes

src/csharp/Microsoft.Spark.Worker/Command/SqlCommandExecutor.cs Outdated Show resolved Hide resolved

eerhardt reviewed Oct 21, 2020

View reviewed changes

src/csharp/Microsoft.Spark.Worker/Command/SqlCommandExecutor.cs Show resolved Hide resolved

eerhardt reviewed Oct 21, 2020

View reviewed changes

src/csharp/Microsoft.Spark/Microsoft.Spark.csproj Show resolved Hide resolved

Prashanth Govindarajan added 2 commits October 21, 2020 11:56

Nits

5021df4

Merge branch 'arraystruct' of https://github.com/pgovind/spark into a…

f5da9cf

…rraystructInternal

eerhardt previously approved these changes Oct 21, 2020

View reviewed changes

suhsteve marked this pull request as ready for review October 21, 2020 19:05

suhsteve previously approved these changes Oct 21, 2020

View reviewed changes

Address comments

8a67ece

pgovind dismissed stale reviews from suhsteve and eerhardt via 8a67ece October 22, 2020 17:18

suhsteve reviewed Oct 22, 2020

View reviewed changes

src/csharp/Microsoft.Spark.Worker/Command/SqlCommandExecutor.cs Show resolved Hide resolved

src/csharp/Microsoft.Spark.Worker/Command/SqlCommandExecutor.cs Show resolved Hide resolved

src/csharp/Microsoft.Spark.Worker/Command/SqlCommandExecutor.cs Outdated Show resolved Hide resolved

Prashanth Govindarajan added 2 commits October 22, 2020 12:36

Nits

4e46ff4

Rename

016d6d0

suhsteve reviewed Oct 22, 2020

View reviewed changes

suhsteve previously approved these changes Oct 22, 2020

View reviewed changes

imback82 previously approved these changes Oct 23, 2020

View reviewed changes

src/csharp/Microsoft.Spark.Worker/Command/SqlCommandExecutor.cs Outdated Show resolved Hide resolved

Update src/csharp/Microsoft.Spark.Worker/Command/SqlCommandExecutor.cs

e2cc263

imback82 dismissed stale reviews from suhsteve and themself via e2cc263 October 23, 2020 18:42

imback82 merged commit d6f0b09 into dotnet:master Oct 23, 2020

	if (_version >= new Version(Versions.V3_0_0))
	if (_version.Major >= 3)

Changes to support Arrow 2.0 and Spark 3.0 #711

Changes to support Arrow 2.0 and Spark 3.0 #711

Uh oh!

Conversation

pgovind commented Oct 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eerhardt commented Oct 12, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pgovind commented Oct 21, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

pgovind commented Oct 21, 2020

Uh oh!

suhsteve left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

suhsteve commented Oct 21, 2020

Uh oh!

pgovind commented Oct 22, 2020

Uh oh!

suhsteve commented Oct 22, 2020

Uh oh!

suhsteve left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

suhsteve Oct 22, 2020

Choose a reason for hiding this comment

Uh oh!

pgovind Oct 22, 2020

Choose a reason for hiding this comment

Uh oh!

suhsteve Oct 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgovind commented Oct 23, 2020

Uh oh!

imback82 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pgovind commented Oct 5, 2020 •

edited

Loading

suhsteve Oct 22, 2020 •

edited

Loading