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

Follow up on UDF that takes in and returns Row #406

Merged
merged 4 commits into from
Jan 29, 2020

Conversation

elvaliuliuliu
Copy link
Contributor

@elvaliuliuliu elvaliuliuliu commented Jan 28, 2020

This is a follow up of #376 and #214 to remove the materialization inside construct(). And change it on the worker side. This would make the behavior consistent for UDF that takes in Row and that returns Row.

Copy link
Contributor

@imback82 imback82 left a comment

Choose a reason for hiding this comment

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

Few minor comments, but generally looks good to me.

Assert.Equal(expected, actual);
}

// Row is a part of top-level column.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is a bit misleading. We can say UDF that takes multiple Rows, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I have updated to Multiple Rows.

@@ -65,15 +65,6 @@ public object construct(object[] args)
s_schemaCache = new Dictionary<string, StructType>();
}

// When a row is ready to be materialized, then construct() is called
Copy link
Contributor

Choose a reason for hiding this comment

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

This is actually a breaking change, but @suhsteve is updating the worker version in his PR: https://github.com/dotnet/spark/pull/387/files

Copy link
Contributor

@imback82 imback82 left a comment

Choose a reason for hiding this comment

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

LGTM

@imback82 imback82 merged commit a8db985 into dotnet:master Jan 29, 2020
@elvaliuliuliu elvaliuliuliu deleted the elva/udfTest branch February 1, 2020 00:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants