Skip to content

[SPARK-10905][SparkR]: Export freqItems() for DataFrameStatFunctions #8962

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

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

[SPARK-10905][SparkR]: Export freqItems() for DataFrameStatFunctions #8962

wants to merge 4 commits into from

Conversation

@rerngvit
Copy link
Contributor

@rerngvit rerngvit commented Oct 2, 2015

[SPARK-10905][SparkR]: Export freqItems() for DataFrameStatFunctions

  • Add function (together with roxygen2 doc) to DataFrame.R and generics.R
  • Expose the function in NAMESPACE
  • Add unit test for the function

@shivaram
Copy link
Contributor

shivaram commented Oct 2, 2015

Jenkins, ok to test

@shivaram
Copy link
Contributor

shivaram commented Oct 2, 2015

cc @sun-rui

@SparkQA
Copy link

SparkQA commented Oct 2, 2015

Test build #43181 has finished for PR 8962 at commit f86b6c2.

  • This patch fails R style tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • abstract class UnaryMathExpression(val f: Double => Double, name: String)
    • case class Ceil(child: Expression) extends UnaryMathExpression(math.ceil, "CEIL")
    • case class Floor(child: Expression) extends UnaryMathExpression(math.floor, "FLOOR")

@SparkQA
Copy link

SparkQA commented Oct 3, 2015

Test build #43211 has finished for PR 8962 at commit e15645e.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class HyperLogLogPlusPlus(child: Expression, relativeSD: Double = 0.05)
    • case class JsonTuple(children: Seq[Expression])
    • abstract class UnaryMathExpression(val f: Double => Double, name: String)
    • case class Ceil(child: Expression) extends UnaryMathExpression(math.ceil, "CEIL")
    • case class Floor(child: Expression) extends UnaryMathExpression(math.floor, "FLOOR")

sun-rui
sun-rui reviewed Oct 3, 2015
View reviewed changes
R/pkg/R/generics.R Outdated
Copy link
Contributor

@sun-rui sun-rui Oct 3, 2015

Choose a reason for hiding this comment

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

I think using vector for cols is more R stylish.
setGeneric("freqItems", function(x, cols, support = 0.01) { standardGeneric("freqItems") })

@sun-rui
Copy link
Contributor

sun-rui commented Oct 3, 2015

@shivaram, could you merge my PR for SPARK-10752 first? that PR creates a new R file for stat functions. Then @rerngvit can rebase this PR to it.

@rerngvit
Copy link
Contributor Author

rerngvit commented Oct 4, 2015

@sun-rui I revised according to your comments. Please have a look.

@SparkQA
Copy link

SparkQA commented Oct 4, 2015

Test build #43225 has finished for PR 8962 at commit 57cc150.

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

felixcheung
felixcheung reviewed Oct 6, 2015
View reviewed changes
R/pkg/R/DataFrame.R Outdated
Copy link
Member

@felixcheung felixcheung Oct 6, 2015

Choose a reason for hiding this comment

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

I think sqlCtx should be sqlContext

Copy link
Member

@felixcheung felixcheung Oct 9, 2015

Choose a reason for hiding this comment

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

any update?

@sun-rui
Copy link
Contributor

sun-rui commented Oct 8, 2015

@rerngvit, could you rebase it to the latest master? As I moved stats functions to a new R file stats.R in #8869

@rerngvit rerngvit force-pushed the SPARK-10905 branch 5 times, most recently from b8d71fe to f44f5ea Compare October 8, 2015 17:40
@rerngvit
[SPARK-10905] Revised according to comments
1339e24 
    - Move code to stats.R
    - Revised @Rdname document
    - Document the x dataframe
    - Add additional testcase
    - Convert input columns to a vector instead of R ellipsis
@SparkQA
Copy link

SparkQA commented Oct 8, 2015

Test build #43406 has finished for PR 8962 at commit ae55b24.

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

@SparkQA
Copy link

SparkQA commented Oct 8, 2015

Test build #43410 has finished for PR 8962 at commit 1339e24.

  • This patch fails MiMa tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@shivaram
Copy link
Contributor

shivaram commented Oct 8, 2015

Jenkins, retest this please

@rerngvit
Copy link
Contributor Author

rerngvit commented Oct 8, 2015

The errors seem not related to the PR. There might be an issue with Jenkins.
"ERROR: Timeout after 15 minutes
ERROR: Error fetching remote repo 'origin'"

@shivaram
Copy link
Contributor

shivaram commented Oct 8, 2015

Jenkins, retest this please

@SparkQA
Copy link

SparkQA commented Oct 8, 2015

Test build #43431 has finished for PR 8962 at commit 1339e24.

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

sun-rui
sun-rui reviewed Oct 9, 2015
View reviewed changes
R/pkg/inst/tests/test_sparkSQL.R Outdated
Copy link
Contributor

@sun-rui sun-rui Oct 9, 2015

Choose a reason for hiding this comment

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

Don't need to add a new test_that. Just move this test case into the above one.

sun-rui
sun-rui reviewed Oct 9, 2015
View reviewed changes
R/pkg/R/stats.R Outdated
Copy link
Contributor

@sun-rui sun-rui Oct 9, 2015

Choose a reason for hiding this comment

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

support = 0.01

Copy link
Member

@felixcheung felixcheung Oct 9, 2015

Choose a reason for hiding this comment

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

+1

@rerngvit
Copy link
Contributor Author

rerngvit commented Oct 9, 2015

@sun-rui @felixcheung Thanks for the review. I revised according to your comments. Please have a look.

@SparkQA
Copy link

SparkQA commented Oct 9, 2015

Test build #43463 has finished for PR 8962 at commit 404c8b2.

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

@sun-rui
Copy link
Contributor

sun-rui commented Oct 9, 2015

LGTM

@shivaram
Copy link
Contributor

shivaram commented Oct 9, 2015

Thanks @rerngvit and @sun-rui - LGTM. Merging this

@asfgit asfgit closed this in 70f44ad Oct 9, 2015
@rerngvit rerngvit deleted the SPARK-10905 branch June 7, 2016 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@rerngvit @shivaram @SparkQA @sun-rui @felixcheung