Skip to content

Conversation

@cloud-fan
Copy link
Contributor

in MLlib sometimes we need to set metadata for the new column, thus we will alias the new column with metadata before call withColumn and in withColumn we alias this clolumn again. Here I overloaded withColumn to allow user set metadata, just like what we did for Column.as.

@cloud-fan
Copy link
Contributor Author

cc @marmbrus @rxin , this blocks #7957

@SparkQA
Copy link

SparkQA commented Aug 13, 2015

Test build #40732 has finished for PR 8159 at commit 4698d05.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

Copy link
Member

Choose a reason for hiding this comment

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

typo: metadata

@jkbradley
Copy link
Member

The ML changes look good. (Thanks for adding this.)

The new method looks fine, unless you want to reduce code duplication.

@rxin
Copy link
Contributor

rxin commented Aug 13, 2015

Does the new function needs to be public? Can it be private[spark] ?

@jkbradley
Copy link
Member

ML does not need it to be public.

@rxin
Copy link
Contributor

rxin commented Aug 14, 2015

OK @cloud-fan let's make this private[spark] for now.

@SparkQA
Copy link

SparkQA commented Aug 14, 2015

Test build #40841 has finished for PR 8159 at commit 11c5575.

  • This patch fails to build.
  • This patch merges cleanly.
  • This patch adds no public classes.

@rxin
Copy link
Contributor

rxin commented Aug 14, 2015

The API change LGTM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should we make Column.as(alias: String, metadata: Metadata) also private? As it expose the Metadata too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't think we can change that since it's been public since 1.3.

@SparkQA
Copy link

SparkQA commented Aug 14, 2015

Test build #40843 has finished for PR 8159 at commit 39ce9c7.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@jkbradley
Copy link
Member

LGTM

@rxin
Copy link
Contributor

rxin commented Aug 14, 2015

Merging this in master (not branch-1.5).

@asfgit asfgit closed this in 34d610b Aug 14, 2015
CodingCat pushed a commit to CodingCat/spark that referenced this pull request Aug 17, 2015
in MLlib sometimes we need to set metadata for the new column, thus we will alias the new column with metadata before call `withColumn` and in `withColumn` we alias this clolumn again. Here I overloaded `withColumn` to allow user set metadata, just like what we did  for `Column.as`.

Author: Wenchen Fan <cloud0fan@outlook.com>

Closes apache#8159 from cloud-fan/withColumn.
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.

4 participants