Exposed Column.Apply() API #323

Niharikadutta · 2019-10-30T20:16:26Z

We are excited to review your PR.

So we can do the best job, please check:

There's a descriptive title that will make sense to other developers some time from now.
There's associated issues. All PR's should have issue(s) associated - unless a trivial self-evident change such as fixing a typo. You can use the format Fixes #nnnn in your description to cause GitHub to automatically close the issue(s) when your PR is merged.
Your change description explains what the change does, why you chose your approach, and anything else that reviewers should know.
You have included any necessary tests in the same PR.

This PR addresses feature request #314 and fixes issue #201

suhsteve · 2019-10-30T20:58:49Z

src/csharp/Microsoft.Spark/Sql/Column.cs

+        /// </summary>
+        /// <param name="colObject">Column object to apply</param>
+        /// <returns>Column object</returns>
+        public Column Apply(object colObject)


Is this only going to be exposed for Column type or anything like how scala has it defined

/** * Extracts a value or values from a complex type. * The following types of extraction are supported: * <ul> * <li>Given an Array, an integer ordinal can be used to retrieve a single value.</li> * <li>Given a Map, a key of the correct type can be used to retrieve an individual value.</li> * <li>Given a Struct, a string fieldName can be used to extract that field.</li> * <li>Given an Array of Structs, a string fieldName can be used to extract filed * of every struct in that array, and return an Array of fields.</li> * </ul> * @group expr_ops * @since 1.4.0 */ def apply(extraction: Any): Column = withExpr { UnresolvedExtractValue(expr, lit(extraction).expr) }

Any which is why it is an object, I will change the parameter name to avoid confusion

The summary description should be updated to be somewhat similar to scala's.

suhsteve · 2019-10-30T20:59:03Z

src/csharp/Microsoft.Spark/Sql/Column.cs

+        /// <returns>Column object</returns>
+        public Column Apply(object colObject)
+        {
+            return Apply("apply", colObject);


Can you add a unit test

AFFogarty · 2019-10-30T23:54:15Z

src/csharp/Microsoft.Spark/Sql/Column.cs

+        /// </summary>
+        /// <param name="colObject">Column object to apply</param>
+        /// <returns>Column object</returns>
+        public Column Apply(object colObject)


If we're revealing this public method Apply() then I think we should rename the private method Apply() which has totally different semantics.

Any suggestions on what to call the private Apply function?

How about ApplyMethod

suhsteve · 2019-10-31T16:24:50Z

@imback82 it looks like python's column.getItem() eventually calls the "apply" method. I'm wondering if we should mimic that behavior with C#'s Column.GetItem()

imback82 · 2019-10-31T16:26:44Z

Actually, this is a bug in Spark. I am fixing it: https://issues.apache.org/jira/browse/SPARK-29664

…d unit test for Apply() Column API

…ta/spark into nidutta/ExposeColumnApplyAPI

suhsteve · 2019-11-01T05:02:30Z

src/csharp/Microsoft.Spark/Sql/Column.cs

+
+        /// <summary>
+        /// Extracts a value or values from a complex type.
+        ///The following types of extraction are supported:


Suggested change

///The following types of extraction are supported:

/// The following types of extraction are supported:

Same for the next 5 lines

suhsteve · 2019-11-01T05:04:39Z

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


-            col = col1.Name("alias");
+            //col.Explain(true); -> Do I want to do this on col1 or col2? Or remove this line.


Suggested change

//col.Explain(true); -> Do I want to do this on col1 or col2? Or remove this line.

col1.Explain(true);

suhsteve · 2019-11-01T05:08:14Z

src/csharp/Microsoft.Spark/Sql/Column.cs

+        /// </summary>
+        /// <param name="Obj">object to apply</param>
+        /// <returns>Column object</returns>
+        public Column Apply(object Obj)


Is there anything more meaningful than Obj ? Same comment regarding the param description

I'll change the paramname and description to something more meaningful

suhsteve · 2019-11-01T19:27:17Z

src/csharp/Microsoft.Spark/Sql/Column.cs

+        /// </summary>
+        /// <param name="mappingObject">Object to use to map the values on returning Column object </param>
+        /// <returns>Column object</returns>
+        public Column Apply(object mappingObject)


let's use extraction. Update description as well.

suhsteve · 2019-11-01T19:27:54Z

src/csharp/Microsoft.Spark/Sql/Column.cs

+        /// of every struct in that array, and return an Array of fields.
+        ///
+        /// </summary>
+        /// <param name="mappingObject">Object to use to map the values on returning Column object </param>


Suggested change

/// <param name="mappingObject">Object to use to map the values on returning Column object </param>

/// <param name="mappingObject">Object to use to map the values on returning Column object</param>

suhsteve

LGTM

imback82

LGTM, merging to master.

imback82 · 2019-11-03T17:12:48Z

Fixes #201

imback82 · 2019-11-03T17:15:48Z

Spark side change is also merged: apache/spark#26351

Niharikadutta added 2 commits October 30, 2019 13:08

Exposed Apply() API for Column object

229e98c

Added summary for Column.Apply() API

e692970

suhsteve reviewed Oct 30, 2019

View reviewed changes

AFFogarty requested changes Oct 30, 2019

View reviewed changes

rapoth and others added 3 commits October 31, 2019 19:45

Merge branch 'master' into nidutta/ExposeColumnApplyAPI

727d629

PR review changes - Changed private Apply() to ApplyMethod() and adde…

b36978f

…d unit test for Apply() Column API

Merge branch 'nidutta/ExposeColumnApplyAPI' of github.com:Niharikadut…

e7ba3a7

…ta/spark into nidutta/ExposeColumnApplyAPI

suhsteve reviewed Nov 1, 2019

View reviewed changes

Changed param name and description for Apply()

6cd168c

suhsteve reviewed Nov 1, 2019

View reviewed changes

Niharikadutta and others added 2 commits November 1, 2019 16:19

Changed the parameter name to extraction

632a2d6

Merge branch 'master' into nidutta/ExposeColumnApplyAPI

829c90f

suhsteve approved these changes Nov 3, 2019

View reviewed changes

imback82 approved these changes Nov 3, 2019

View reviewed changes

imback82 merged commit 30d6c37 into dotnet:master Nov 3, 2019

imback82 mentioned this pull request Nov 3, 2019

[BUG]: Cannot use column as input to mapping column #201

Closed

Niharikadutta deleted the nidutta/ExposeColumnApplyAPI branch November 4, 2019 22:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exposed Column.Apply() API #323

Exposed Column.Apply() API #323

Niharikadutta commented Oct 30, 2019

suhsteve Oct 30, 2019

Niharikadutta Oct 30, 2019 •

edited

Loading

suhsteve Oct 30, 2019

suhsteve Oct 30, 2019

AFFogarty Oct 30, 2019

Niharikadutta Oct 31, 2019

suhsteve Oct 31, 2019

suhsteve commented Oct 31, 2019

imback82 commented Oct 31, 2019

suhsteve Nov 1, 2019 •

edited

Loading

suhsteve Nov 1, 2019

suhsteve Nov 1, 2019

Niharikadutta Nov 1, 2019

suhsteve Nov 1, 2019

suhsteve Nov 1, 2019

suhsteve left a comment

imback82 left a comment

imback82 commented Nov 3, 2019

imback82 commented Nov 3, 2019

	///The following types of extraction are supported:
	/// The following types of extraction are supported:


		col = col1.Name("alias");
		//col.Explain(true); -> Do I want to do this on col1 or col2? Or remove this line.

	//col.Explain(true); -> Do I want to do this on col1 or col2? Or remove this line.
	col1.Explain(true);

	/// <param name="mappingObject">Object to use to map the values on returning Column object </param>
	/// <param name="mappingObject">Object to use to map the values on returning Column object</param>

Exposed Column.Apply() API #323

Exposed Column.Apply() API #323

Conversation

Niharikadutta commented Oct 30, 2019

Choose a reason for hiding this comment

Niharikadutta Oct 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suhsteve commented Oct 31, 2019

imback82 commented Oct 31, 2019

suhsteve Nov 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suhsteve left a comment

Choose a reason for hiding this comment

imback82 left a comment

Choose a reason for hiding this comment

imback82 commented Nov 3, 2019

imback82 commented Nov 3, 2019

Niharikadutta Oct 30, 2019 •

edited

Loading

suhsteve Nov 1, 2019 •

edited

Loading