[GitHub] spark pull request #22331: [SPARK-25331][SS] Make FileStreamSink ignore part...

2018-12-07 Thread misutoth
Github user misutoth closed the pull request at:

https://github.com/apache/spark/pull/22331


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark issue #22331: [SPARK-25331][SS] Make FileStreamSink ignore partitions ...

2018-12-07 Thread misutoth
Github user misutoth commented on the issue:

https://github.com/apache/spark/pull/22331
  
So I am considering this as the recommended way to read a file sink's 
output. If there is a need to include the protocol in this PR as an alternative 
we can still reopen it.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark issue #22532: [SPARK-20845][SQL] Support specification of column names...

2018-10-08 Thread misutoth
Github user misutoth commented on the issue:

https://github.com/apache/spark/pull/22532
  
Many thanks for the feedback. I will list the test scenarios that I had in 
mind and collected while I implemented this item.

And sorry about the failure, seems like I did not rerun all the tests in my 
last step... For example when the same field is queried multiple times it is 
not handled properly. I will fix them also ...


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark issue #22331: [SPARK-25331][SS] Make FileStreamSink ignore partitions ...

2018-09-28 Thread misutoth
Github user misutoth commented on the issue:

https://github.com/apache/spark/pull/22331
  
@lw-lin , @marmbrus , in the meantime I found that you have been discussing 
about having deterministic file names in a 
[PR](https://github.com/apache/spark/pull/16987#issuecomment-282094165). Could 
you please tell those cases?

I was just thinking if it is a reasonable expectation from a sink's point 
of view to receive the same data partitioned the same way if it is actually the 
same batch?

@gaborgsomogyi, you may also be interested in this change.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark issue #22532: [SPARK-20845][SQL] Support specification of column names...

2018-09-26 Thread misutoth
Github user misutoth commented on the issue:

https://github.com/apache/spark/pull/22532
  
@janewangfb, @gatorsmile could you please possibly review this change?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark issue #22331: [SPARK-25331][SS] Make FileStreamSink ignore partitions ...

2018-09-25 Thread misutoth
Github user misutoth commented on the issue:

https://github.com/apache/spark/pull/22331
  
@rxin could you please look into this change?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #22532: [SPARK-20845][SQL] Support specification of colum...

2018-09-23 Thread misutoth
GitHub user misutoth opened a pull request:

https://github.com/apache/spark/pull/22532

[SPARK-20845][SQL] Support specification of column names in INSERT INTO 
command.

## What changes were proposed in this pull request?

One can specify a list of columns for an INSERT INTO command. The columns 
shall be listed in parenthesis just following the table name. Query columns are 
then matched to this very same order.

```
scala> sql("CREATE TABLE t (s string, i int)")
scala> sql("INSERT INTO t values ('first', 1)")
scala> sql("INSERT INTO t (i, s) values (2, 'second')")
scala> sql("SELECT * FROM t").show
+--+---+
| s|  i|
+--+---+
| first|  1|
|second|  2|
+--+---+


scala>
```

In the above example the _second_ insertion utilizes the new functionality. 
The number and its associated string is given in reverse order `(2, 'second')` 
according to the column list specified for the table `(i, s)`. The result can 
be seen at the end of the command list. Intermediate output of the commands are 
omitted for the sake of brevity.

## How was this patch tested?

InsertSuite (both in source and in hive sub-packages) were extended with 
tests exercising specification of column names listing in INSERT INTO commands.

Also ran the above sample, and ran tests in `sql`.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/misutoth/spark insert-into-columns

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/spark/pull/22532.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #22532


commit 1dda672d336b906ecc133f468435b4cf38859e2d
Author: Mihaly Toth 
Date:   2018-03-20T06:13:01Z

[SPARK-20845][SQL] Support specification of column names in INSERT INTO 
command.




---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #22331: [SPARK-25331][SS] Make FileStreamSink ignore part...

2018-09-21 Thread misutoth
Github user misutoth commented on a diff in the pull request:

https://github.com/apache/spark/pull/22331#discussion_r219521421
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/StagingFileCommitProtocol.scala
 ---
@@ -0,0 +1,141 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.execution.streaming
+
+import org.apache.hadoop.fs.{FileAlreadyExistsException, FileContext, Path}
+import org.apache.hadoop.mapreduce.{JobContext, TaskAttemptContext}
+
+import org.apache.spark.internal.Logging
+import org.apache.spark.internal.io.FileCommitProtocol
+import org.apache.spark.internal.io.FileCommitProtocol.TaskCommitMessage
+
+class StagingFileCommitProtocol(jobId: String, path: String)
+  extends FileCommitProtocol with Serializable with Logging
+  with ManifestCommitProtocol {
+  private var stagingDir: Option[Path] = None
--- End diff --

Yes, I will do that.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #22331: Tests for idempotency of FileStreamSink - Work in...

2018-09-04 Thread misutoth
GitHub user misutoth opened a pull request:

https://github.com/apache/spark/pull/22331

Tests for idempotency of FileStreamSink - Work in Progress

## What changes were proposed in this pull request?

Reproduce File Sink duplication in driver failure scenario to help 
understanding the situation.

## How was this patch tested?
This is a test addition only that was run and the last 2 tests failed 
showing there is a problem.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/misutoth/spark file-sink-dupe

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/spark/pull/22331.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #22331


commit 0a5c6c45a4b90fc2ea8bd2647b6d3d3dfd8bd1a4
Author: Mihaly Toth 
Date:   2018-09-03T11:47:52Z

Tests for idempotency of FileStreamSink




---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #21591: [SQL][WIP] Added column name listing option to IN...

2018-08-24 Thread misutoth
Github user misutoth closed the pull request at:

https://github.com/apache/spark/pull/21591


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #21591: [SQL][WIP] Added column name listing option to IN...

2018-06-19 Thread misutoth
GitHub user misutoth opened a pull request:

https://github.com/apache/spark/pull/21591

[SQL][WIP] Added column name listing option to INSERT INTO

## What changes were proposed in this pull request?

Added possibility to specify column list to INSERT INTO. Source column list 
matched against that explicit list of columns. Also works if column requires 
casting.

Did not work out all the cases. I am interested in if the direction and 
basic concepts are good. Then I can create a "real" PR.

## How was this patch tested?

Ran unit tests, created new ones in InsertInto.


You can merge this pull request into a Git repository by running:

$ git pull https://github.com/misutoth/spark master-SPARK-20845

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/spark/pull/21591.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #21591


commit 830859c05d07f9db2780d377a12925e056bcebcd
Author: Mihaly Toth 
Date:   2018-03-20T06:13:01Z

[SQL][WIP] Added column name listing option to INSERT INTO




---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark issue #20674: [SPARK-23465][SQL] Introduce new function to rename colu...

2018-04-05 Thread misutoth
Github user misutoth commented on the issue:

https://github.com/apache/spark/pull/20674
  
Seems like this functionality is not missed very painfully, so closing the 
PR.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20674: [SPARK-23465][SQL] Introduce new function to rena...

2018-04-05 Thread misutoth
Github user misutoth closed the pull request at:

https://github.com/apache/spark/pull/20674


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark issue #20853: [SPARK-23729][CORE] Respect URI fragment when resolving ...

2018-03-20 Thread misutoth
Github user misutoth commented on the issue:

https://github.com/apache/spark/pull/20853
  
retest this please


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20853: [SPARK-23729][CORE] Respect URI fragment when res...

2018-03-20 Thread misutoth
Github user misutoth commented on a diff in the pull request:

https://github.com/apache/spark/pull/20853#discussion_r175664716
  
--- Diff: core/src/main/scala/org/apache/spark/deploy/DependencyUtils.scala 
---
@@ -137,16 +138,32 @@ private[deploy] object DependencyUtils {
   def resolveGlobPaths(paths: String, hadoopConf: Configuration): String = 
{
 require(paths != null, "paths cannot be null.")
 Utils.stringToSeq(paths).flatMap { path =>
-  val uri = Utils.resolveURI(path)
-  uri.getScheme match {
-case "local" | "http" | "https" | "ftp" => Array(path)
-case _ =>
-  val fs = FileSystem.get(uri, hadoopConf)
-  Option(fs.globStatus(new Path(uri))).map { status =>
-status.filter(_.isFile).map(_.getPath.toUri.toString)
-  }.getOrElse(Array(path))
+  val (base, fragment) = splitOnFragment(path)
+  (resolveGlobPath(base, hadoopConf), fragment) match {
+case (resolved, Some(_)) if resolved.length > 1 => throw new 
SparkException(
+s"${base.toString} resolves ambiguously to multiple files: 
${resolved.mkString(",")}")
--- End diff --

There was no space used here before. Actually there should not be any space 
in the resulting list. Tests also rely on this.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20853: [SPARK-23729][CORE] Respect URI fragment when res...

2018-03-19 Thread misutoth
Github user misutoth commented on a diff in the pull request:

https://github.com/apache/spark/pull/20853#discussion_r175576249
  
--- Diff: core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala ---
@@ -245,6 +245,19 @@ object SparkSubmit extends CommandLineUtils with 
Logging {
   args: SparkSubmitArguments,
   conf: Option[HadoopConfiguration] = None)
   : (Seq[String], Seq[String], SparkConf, String) = {
+try {
+  doPrepareSubmitEnvironment(args, conf)
+} catch {
+  case e: SparkException =>
+printErrorAndExit(e.getMessage)
+throw new RuntimeException("Unreachable production code")
--- End diff --

Actually the directory deletion is hooked into JVM shutdown. So I will let 
this to do the housekeeping for us and will avoid a new field either.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20853: [SPARK-23729][CORE] Respect URI fragment when res...

2018-03-19 Thread misutoth
Github user misutoth commented on a diff in the pull request:

https://github.com/apache/spark/pull/20853#discussion_r175575718
  
--- Diff: core/src/main/scala/org/apache/spark/deploy/DependencyUtils.scala 
---
@@ -137,16 +137,29 @@ private[deploy] object DependencyUtils {
   def resolveGlobPaths(paths: String, hadoopConf: Configuration): String = 
{
 require(paths != null, "paths cannot be null.")
 Utils.stringToSeq(paths).flatMap { path =>
-  val uri = Utils.resolveURI(path)
-  uri.getScheme match {
-case "local" | "http" | "https" | "ftp" => Array(path)
-case _ =>
-  val fs = FileSystem.get(uri, hadoopConf)
-  Option(fs.globStatus(new Path(uri))).map { status =>
-status.filter(_.isFile).map(_.getPath.toUri.toString)
-  }.getOrElse(Array(path))
+  val spath = path.split('#')
--- End diff --

You are right. It took some time to clone a URI without the fragment part 
though but next version will include that.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark issue #20853: [SPARK-23729][CORE] Respect URI fragment when resolving ...

2018-03-19 Thread misutoth
Github user misutoth commented on the issue:

https://github.com/apache/spark/pull/20853
  
> Maybe just let the exception propagate? That's what a lot of this code 
does... then you don't need to change this file at all.

@vanzin I want to present an error on the CLI. This is what the 
`printErrorAndExit` does as I understood.

On the other hand I simplified to just rethrowing the Exception `e`


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20853: [SPARK-23729][CORE] Respect URI fragment when res...

2018-03-19 Thread misutoth
Github user misutoth commented on a diff in the pull request:

https://github.com/apache/spark/pull/20853#discussion_r175546041
  
--- Diff: 
core/src/test/scala/org/apache/spark/deploy/SparkSubmitSuite.scala ---
@@ -105,11 +105,17 @@ class SparkSubmitSuite
 
   // Necessary to make ScalaTest 3.x interrupt a thread on the JVM like 
ScalaTest 2.2.x
   implicit val defaultSignaler: Signaler = ThreadSignaler
+  var dir: File = null
--- End diff --

I wanted to make sure the directory is deleted even if the test fails


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20853: [SPARK-23729][CORE] Respect URI fragment when res...

2018-03-19 Thread misutoth
Github user misutoth commented on a diff in the pull request:

https://github.com/apache/spark/pull/20853#discussion_r175543984
  
--- Diff: core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala ---
@@ -245,6 +245,19 @@ object SparkSubmit extends CommandLineUtils with 
Logging {
   args: SparkSubmitArguments,
   conf: Option[HadoopConfiguration] = None)
   : (Seq[String], Seq[String], SparkConf, String) = {
+try {
+  doPrepareSubmitEnvironment(args, conf)
+} catch {
+  case e: SparkException =>
+printErrorAndExit(e.getMessage)
+throw new RuntimeException("Unreachable production code")
--- End diff --

Otherwise execution just continues in the test itself where `exitFn` does 
not stop it.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20853: [SPARK-23729][CORE] Respect URI fragment when res...

2018-03-19 Thread misutoth
Github user misutoth commented on a diff in the pull request:

https://github.com/apache/spark/pull/20853#discussion_r175541331
  
--- Diff: core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala ---
@@ -245,6 +245,19 @@ object SparkSubmit extends CommandLineUtils with 
Logging {
   args: SparkSubmitArguments,
   conf: Option[HadoopConfiguration] = None)
   : (Seq[String], Seq[String], SparkConf, String) = {
+try {
+  doPrepareSubmitEnvironment(args, conf)
+} catch {
+  case e: SparkException =>
+printErrorAndExit(e.getMessage)
+throw new RuntimeException("Unreachable production code")
--- End diff --

Which part do you mean and overkill?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20853: [SPARK-23729][CORE] Respect URI fragment when res...

2018-03-19 Thread misutoth
Github user misutoth commented on a diff in the pull request:

https://github.com/apache/spark/pull/20853#discussion_r175540130
  
--- Diff: 
core/src/test/scala/org/apache/spark/deploy/SparkSubmitSuite.scala ---
@@ -606,9 +612,12 @@ class SparkSubmitSuite
   }
 
   test("resolves command line argument paths correctly") {
+val archive = Paths.get(dir.toPath.toString, "single.zip")
+Files.createFile(archive)
 val jars = "/jar1,/jar2" // --jars
 val files = "local:/file1,file2"  // --files
-val archives = "file:/archive1,archive2" // --archives
+val archives = 
s"file:/archive1,${dir.toPath.toAbsolutePath.toString}/*.zip#archive3"
+ // --archives
 val pyFiles = "py-file1,py-file2"// --py-files
--- End diff --

According to the 
[doc](https://spark.apache.org/docs/latest/running-on-yarn.html) only `--files` 
and `--archives` support it. 


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20853: [SPARK-23729][CORE] Respect URI fragment when res...

2018-03-19 Thread misutoth
Github user misutoth commented on a diff in the pull request:

https://github.com/apache/spark/pull/20853#discussion_r175539256
  
--- Diff: 
core/src/test/scala/org/apache/spark/deploy/SparkSubmitSuite.scala ---
@@ -105,11 +105,17 @@ class SparkSubmitSuite
 
   // Necessary to make ScalaTest 3.x interrupt a thread on the JVM like 
ScalaTest 2.2.x
   implicit val defaultSignaler: Signaler = ThreadSignaler
+  var dir: File = null
--- End diff --

I was thinking about this too. I wanted to avoid making it an Option or 
doing a not very nice null check. I can do that later though...


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20853: [SPARK-23729][SS] Glob resolution is done without...

2018-03-18 Thread misutoth
GitHub user misutoth opened a pull request:

https://github.com/apache/spark/pull/20853

[SPARK-23729][SS] Glob resolution is done without the fragment part which 
is meant to be the remote name

## What changes were proposed in this pull request?

Firstly, glob resolution will not result in swallowing the remote name part 
(that is preceded by the `#` sign) in case of `--files` or `--archives` options

Moreover in the special case of multiple resolutions when the remote naming 
does not make sense and error is returned.
## How was this patch tested?

Enhanced current test and wrote additional test for the error case

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/misutoth/spark glob-with-remote-name

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/spark/pull/20853.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #20853


commit 50b5ad1289fabffe4f661d0b44fcc9ef3ecd2367
Author: Mihaly Toth <misutoth@...>
Date:   2018-03-18T06:04:15Z

[SPARK-23729][SS] Glob resolution is done without the fragment part which 
is meant to be the remote name
In case glob resolution results multiple items for a file with a remote 
name, an error is displayed.




---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark issue #20674: [SPARK-23465][SQL] Introduce new function to rename colu...

2018-03-05 Thread misutoth
Github user misutoth commented on the issue:

https://github.com/apache/spark/pull/20674
  
@rxin @marmbrus @gatorsmile do you think this might be useful? I would 
appreciate your comments.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20618: [SPARK-23329][SQL] Fix documentation of trigonome...

2018-03-02 Thread misutoth
Github user misutoth commented on a diff in the pull request:

https://github.com/apache/spark/pull/20618#discussion_r171992166
  
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/functions.scala ---
@@ -1313,131 +1313,165 @@ object functions {
   
//
 
   /**
-   * Computes the cosine inverse of the given value; the returned angle is 
in the range
-   * 0.0 through pi.
+   * @return inverse cosine of `e` in radians, as if computed by 
[[java.lang.Math#acos]]
*
* @group math_funcs
* @since 1.4.0
*/
   def acos(e: Column): Column = withExpr { Acos(e.expr) }
 
   /**
-   * Computes the cosine inverse of the given column; the returned angle 
is in the range
-   * 0.0 through pi.
+   * @return inverse cosine of `columnName`, as if computed by 
[[java.lang.Math#acos]]
*
* @group math_funcs
* @since 1.4.0
*/
   def acos(columnName: String): Column = acos(Column(columnName))
 
   /**
-   * Computes the sine inverse of the given value; the returned angle is 
in the range
-   * -pi/2 through pi/2.
+   * @return inverse sine of `e` in radians, as if computed by 
[[java.lang.Math#asin]]
*
* @group math_funcs
* @since 1.4.0
*/
   def asin(e: Column): Column = withExpr { Asin(e.expr) }
 
   /**
-   * Computes the sine inverse of the given column; the returned angle is 
in the range
-   * -pi/2 through pi/2.
+   * @return inverse sine of `columnName`, as if computed by 
[[java.lang.Math#asin]]
*
* @group math_funcs
* @since 1.4.0
*/
   def asin(columnName: String): Column = asin(Column(columnName))
 
   /**
-   * Computes the tangent inverse of the given column; the returned angle 
is in the range
-   * -pi/2 through pi/2
+   * @return inverse tangent of `e`, as if computed by 
[[java.lang.Math#atan]]
*
* @group math_funcs
* @since 1.4.0
*/
   def atan(e: Column): Column = withExpr { Atan(e.expr) }
 
   /**
-   * Computes the tangent inverse of the given column; the returned angle 
is in the range
-   * -pi/2 through pi/2
+   * @return inverse tangent of `columnName`, as if computed by 
[[java.lang.Math#atan]]
*
* @group math_funcs
* @since 1.4.0
*/
   def atan(columnName: String): Column = atan(Column(columnName))
 
   /**
-   * Returns the angle theta from the conversion of rectangular 
coordinates (x, y) to
-   * polar coordinates (r, theta). Units in radians.
+   * @param y coordinate on y-axis
+   * @param x coordinate on x-axis
+   * @return the theta component of the point
+   * (r,theta)
+   * in polar coordinates that corresponds to the point
+   * (x,y) in Cartesian coordinates,
+   * as if computed by [[java.lang.Math#atan2]]
*
* @group math_funcs
* @since 1.4.0
*/
-  def atan2(l: Column, r: Column): Column = withExpr { Atan2(l.expr, 
r.expr) }
+  def atan2(y: Column, x: Column): Column = withExpr { Atan2(y.expr, 
x.expr) }
 
   /**
-   * Returns the angle theta from the conversion of rectangular 
coordinates (x, y) to
-   * polar coordinates (r, theta).
+   * @param y coordinate on y-axis
+   * @param xName coordinate on x-axis
+   * @return the theta component of the point
+   * (r,theta)
+   * in polar coordinates that corresponds to the point
+   * (x,y) in Cartesian coordinates,
+   * as if computed by [[java.lang.Math#atan2]]
*
* @group math_funcs
* @since 1.4.0
*/
-  def atan2(l: Column, rightName: String): Column = atan2(l, 
Column(rightName))
+  def atan2(y: Column, xName: String): Column = atan2(y, Column(xName))
 
   /**
-   * Returns the angle theta from the conversion of rectangular 
coordinates (x, y) to
-   * polar coordinates (r, theta).
+   * @param yName coordinate on y-axis
+   * @param x coordinate on x-axis
+   * @return the theta component of the point
+   * (r,theta)
+   * in polar coordinates that corresponds to the point
+   * (x,y) in Cartesian coordinates,
+   * as if computed by [[java.lang.Math#atan2]]
*
* @group math_funcs
* @since 1.4.0
*/
-  def atan2(leftName: String, r: Column): Column = atan2(Column(leftName), 
r)
+  def atan2(yName: String, x: Column): Column = atan2(Column(yName), x)
 
   /**
-   * Returns the angle theta from the conversion of rectangular 
coordinates (x, y) to
-   * polar coordinates (r, theta).
+   * @param yName coordinate on y-axis
+   * @param

[GitHub] spark pull request #20618: [SPARK-23329][SQL] Fix documentation of trigonome...

2018-03-02 Thread misutoth
Github user misutoth commented on a diff in the pull request:

https://github.com/apache/spark/pull/20618#discussion_r171991788
  
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/functions.scala ---
@@ -1500,31 +1534,35 @@ object functions {
   }
 
   /**
-   * Computes the cosine of the given value. Units in radians.
+   * @param e angle in radians
+   * @return cosine of the angle, as if computed by [[java.lang.Math#cos]]
--- End diff --

Replaced all of them as suggested.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20618: [SPARK-23329][SQL] Fix documentation of trigonome...

2018-03-02 Thread misutoth
Github user misutoth commented on a diff in the pull request:

https://github.com/apache/spark/pull/20618#discussion_r171991691
  
--- Diff: python/pyspark/sql/functions.py ---
@@ -173,16 +172,26 @@ def _():
 
 _functions_2_1 = {
 # unary math functions
-'degrees': 'Converts an angle measured in radians to an approximately 
equivalent angle ' +
-   'measured in degrees.',
-'radians': 'Converts an angle measured in degrees to an approximately 
equivalent angle ' +
-   'measured in radians.',
+'degrees': """Converts an angle measured in radians to an 
approximately equivalent angle
+   measured in degrees.
--- End diff --

added


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20618: [SPARK-23329][SQL] Fix documentation of trigonome...

2018-03-02 Thread misutoth
Github user misutoth commented on a diff in the pull request:

https://github.com/apache/spark/pull/20618#discussion_r171991733
  
--- Diff: python/pyspark/sql/functions.py ---
@@ -173,16 +172,26 @@ def _():
 
 _functions_2_1 = {
 # unary math functions
-'degrees': 'Converts an angle measured in radians to an approximately 
equivalent angle ' +
-   'measured in degrees.',
-'radians': 'Converts an angle measured in degrees to an approximately 
equivalent angle ' +
-   'measured in radians.',
+'degrees': """Converts an angle measured in radians to an 
approximately equivalent angle
+   measured in degrees.
+   :param col: angle in radians
+   :return: angle in degrees, as if computed by 
`java.lang.Math.toDegrees()`""",
+'radians': """Converts an angle measured in degrees to an 
approximately equivalent angle measured in radians.
--- End diff --

Yes. I wrapped the text.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20618: [SPARK-23329][SQL] Fix documentation of trigonome...

2018-03-02 Thread misutoth
Github user misutoth commented on a diff in the pull request:

https://github.com/apache/spark/pull/20618#discussion_r171991647
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala
 ---
@@ -512,7 +529,11 @@ case class Rint(child: Expression) extends 
UnaryMathExpression(math.rint, "ROUND
 case class Signum(child: Expression) extends 
UnaryMathExpression(math.signum, "SIGNUM")
 
 @ExpressionDescription(
-  usage = "_FUNC_(expr) - Returns the sine of `expr`.",
+  usage = "_FUNC_(expr) - Returns the sine of `expr`, as if computed by 
`java.lang.Math._FUNC_`.",
--- End diff --

As far as the trigonometric functions concerned they should be now in sync 
I think.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20674: [SPARK-23465][SQL] Introduce new function to rena...

2018-02-28 Thread misutoth
Github user misutoth commented on a diff in the pull request:

https://github.com/apache/spark/pull/20674#discussion_r171386636
  
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/Dataset.scala ---
@@ -2239,6 +2239,34 @@ class Dataset[T] private[sql](
 }
   }
 
+   /**
+* Returns a new Dataset with altered column names.
+* This is a no-op if schema doesn't contain existingName.
+*
+* @param convert conversion function from the old to the new name
+*
+* @group untypedrel
+* @since 2.0.0
+*/
+  def withAllColumnsRenamed(convert: String => String): DataFrame = {
+val output = queryExecution.analyzed.output
+var containsRename = false
--- End diff --

Thats correct. Removed that optimization and the code became much smaller.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark issue #20618: [SPARK-23329][SQL] Fix documentation of trigonometric fu...

2018-02-28 Thread misutoth
Github user misutoth commented on the issue:

https://github.com/apache/spark/pull/20618
  
I included java.lang.Math references in functions.R


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark issue #20618: [SPARK-23329][SQL] Fix documentation of trigonometric fu...

2018-02-27 Thread misutoth
Github user misutoth commented on the issue:

https://github.com/apache/spark/pull/20618
  
As discussed in email R documentation is reorganized and math functions are 
grouped as part of SPARK-20889. Because of this grouping I dont think this 
change is really applicable on R. @srowen, @HyukjinKwon, @felixcheung  what do 
you think about it?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark issue #20618: [SPARK-23329][SQL] Fix documentation of trigonometric fu...

2018-02-27 Thread misutoth
Github user misutoth commented on the issue:

https://github.com/apache/spark/pull/20618
  
@felixcheung, I have started a mail thread on d...@spark.apache.org with 
title _Help needed in R documentation generation_ because I did not feel it is 
directly related to this PR. Thanks for your reply on this thread already.



---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark issue #20618: [SPARK-23329][SQL] Fix documentation of trigonometric fu...

2018-02-25 Thread misutoth
Github user misutoth commented on the issue:

https://github.com/apache/spark/pull/20618
  
Sorry, I missed these comments. As I understood we fix all of them here. I 
am just struggling with the R documentation: it seems the generated doc is 
incorrect even if I just take the latest commit as it is. I thought about 
writing to the dev list to see if others have experienced this.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20674: [SPARK-23465][SQL] Introduce new function to rena...

2018-02-25 Thread misutoth
GitHub user misutoth opened a pull request:

https://github.com/apache/spark/pull/20674

[SPARK-23465][SQL] Introduce new function to rename columns using an 
algoritm

## What changes were proposed in this pull request?

Add an additional convenient method to rename multiple of columns by 
specifying a mapping between the old and the new column name.

## How was this patch tested?

Wrote additional unit test cases, ran scalastyle.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/misutoth/spark column-rename

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/spark/pull/20674.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #20674


commit f03884c3582cdb3bc2eaa034cbc0bcdcfd1c8250
Author: Mihaly Toth <misutoth@...>
Date:   2018-02-24T20:19:19Z

[SPARK-23465][SQL] Introduce new function to rename columns using an 
algorithm




---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark issue #20618: [SPARK-23329][SQL] Fix documentation of trigonometric fu...

2018-02-20 Thread misutoth
Github user misutoth commented on the issue:

https://github.com/apache/spark/pull/20618
  
Sure, lets do that, no problem.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark issue #20618: [SPARK-23329][SQL] Fix documentation of trigonometric fu...

2018-02-20 Thread misutoth
Github user misutoth commented on the issue:

https://github.com/apache/spark/pull/20618
  
Looking back I guess we can expect a couple of comments on R and Python 
side too, though I will target a lower number of them. :) So I am a little bit 
favoring moving functions.* update into a separate ticket.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20618: [SPARK-23329][SQL] Fix documentation of trigonome...

2018-02-20 Thread misutoth
Github user misutoth commented on a diff in the pull request:

https://github.com/apache/spark/pull/20618#discussion_r169313329
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala
 ---
@@ -548,7 +579,14 @@ case class Sqrt(child: Expression) extends 
UnaryMathExpression(math.sqrt, "SQRT"
 case class Tan(child: Expression) extends UnaryMathExpression(math.tan, 
"TAN")
 
 @ExpressionDescription(
-  usage = "_FUNC_(expr) - Returns the cotangent of `expr`.",
+  usage = """
+_FUNC_(expr) - Returns the cotangent of `expr`, as if computed by
+  `1/java.lang.Math._FUNC_`.
--- End diff --

yes


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20618: [SPARK-23329][SQL] Fix documentation of trigonome...

2018-02-20 Thread misutoth
Github user misutoth commented on a diff in the pull request:

https://github.com/apache/spark/pull/20618#discussion_r169313008
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala
 ---
@@ -262,6 +272,10 @@ case class Cos(child: Expression) extends 
UnaryMathExpression(math.cos, "COS")
 
 @ExpressionDescription(
   usage = "_FUNC_(expr) - Returns the hyperbolic cosine of `expr`.",
--- End diff --

Yes, we should


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20618: [SPARK-23329][SQL] Fix documentation of trigonome...

2018-02-20 Thread misutoth
Github user misutoth commented on a diff in the pull request:

https://github.com/apache/spark/pull/20618#discussion_r169312315
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala
 ---
@@ -262,6 +272,10 @@ case class Cos(child: Expression) extends 
UnaryMathExpression(math.cos, "COS")
 
 @ExpressionDescription(
   usage = "_FUNC_(expr) - Returns the hyperbolic cosine of `expr`.",
+  arguments = """
+Arguments:
+  * expr - hyperbolic angle.
--- End diff --

ok


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20618: [SPARK-23329][SQL] Fix documentation of trigonome...

2018-02-20 Thread misutoth
Github user misutoth commented on a diff in the pull request:

https://github.com/apache/spark/pull/20618#discussion_r169311993
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala
 ---
@@ -252,7 +255,14 @@ case class Ceil(child: Expression) extends 
UnaryMathExpression(math.ceil, "CEIL"
 }
 
 @ExpressionDescription(
-  usage = "_FUNC_(expr) - Returns the cosine of `expr`.",
+  usage = """
+_FUNC_(expr) - Returns the cosine of `expr`, as if computed by
+  `java.lang.Math._FUNC_`.
+  """,
+  arguments = """
+Arguments:
+  * expr - angle in radians
+""",
--- End diff --

yes, indeed that is used elsewhere


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20618: [SPARK-23329][SQL] Fix documentation of trigonome...

2018-02-20 Thread misutoth
Github user misutoth commented on a diff in the pull request:

https://github.com/apache/spark/pull/20618#discussion_r169311232
  
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/functions.scala ---
@@ -2005,71 +1967,63 @@ object functions {
   def signum(columnName: String): Column = signum(Column(columnName))
 
   /**
-   * @param e angle in radians
-   * @return sine of the angle, as if computed by [[java.lang.Math#sin]]
+   * Computes the sine of the given value. Units in radians.
*
* @group math_funcs
* @since 1.4.0
*/
   def sin(e: Column): Column = withExpr { Sin(e.expr) }
 
   /**
-   * @param columnName angle in radians
-   * @return sine of the angle, as if computed by [[java.lang.Math#sin]]
+   * Computes the sine of the given column.
*
* @group math_funcs
* @since 1.4.0
*/
   def sin(columnName: String): Column = sin(Column(columnName))
 
   /**
-   * @param e hyperbolic angle
-   * @return hyperbolic sine of the given value, as if computed by 
[[java.lang.Math#sinh]]
+   * Computes the hyperbolic sine of the given value.
*
* @group math_funcs
* @since 1.4.0
*/
   def sinh(e: Column): Column = withExpr { Sinh(e.expr) }
 
   /**
-   * @param columnName hyperbolic angle
-   * @return hyperbolic sine of the given value, as if computed by 
[[java.lang.Math#sinh]]
+   * Computes the hyperbolic sine of the given column.
*
* @group math_funcs
* @since 1.4.0
*/
   def sinh(columnName: String): Column = sinh(Column(columnName))
 
   /**
-   * @param e angle in radians
-   * @return tangent of the given value, as if computed by 
[[java.lang.Math#tan]]
+   * Computes the tangent of the given value. Units in radians.
*
* @group math_funcs
* @since 1.4.0
*/
   def tan(e: Column): Column = withExpr { Tan(e.expr) }
 
   /**
-   * @param columnName angle in radians
-   * @return tangent of the given value, as if computed by 
[[java.lang.Math#tan]]
+   * Computes the tangent of the given column.
*
* @group math_funcs
* @since 1.4.0
*/
   def tan(columnName: String): Column = tan(Column(columnName))
 
   /**
-   * @param e hyperbolic angle
-   * @return hyperbolic tangent of the given value, as if computed by 
[[java.lang.Math#tanh]]
--- End diff --

This way the description and the return value text would be redundant. For 
this reason it was proposed [during the discussion of the 
issue](https://issues.apache.org/jira/browse/SPARK-23329?focusedCommentId=16354178=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16354178)
 to keep only the `@return` part.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20618: [SPARK-23329][SQL] Fix documentation of trigonome...

2018-02-20 Thread misutoth
Github user misutoth commented on a diff in the pull request:

https://github.com/apache/spark/pull/20618#discussion_r169291756
  
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/functions.scala ---
@@ -1313,131 +1313,165 @@ object functions {
   
//
 
   /**
-   * Computes the cosine inverse of the given value; the returned angle is 
in the range
-   * 0.0 through pi.
--- End diff --

Seems like a valid point and I tend to agree with the conclusion to create 
a separate Jira for this. So far the discussion was about {{functions.scala}} 
but other languages should offer similar descriptions for the same thing. 
@srowen, can you please comment on this? I will take {{functions.scala}} out 
for now.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20618: [SPARK-23329][SQL] Fix documentation of trigonome...

2018-02-20 Thread misutoth
Github user misutoth commented on a diff in the pull request:

https://github.com/apache/spark/pull/20618#discussion_r169287876
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala
 ---
@@ -521,7 +542,15 @@ case class Signum(child: Expression) extends 
UnaryMathExpression(math.signum, "S
 case class Sin(child: Expression) extends UnaryMathExpression(math.sin, 
"SIN")
 
 @ExpressionDescription(
-  usage = "_FUNC_(expr) - Returns the hyperbolic sine of `expr`.",
+  usage = """
+  _FUNC_(expr) - Returns hyperbolic sine of `expr`, as if computed by
--- End diff --

correct


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20618: [SPARK-23329][SQL] Fix documentation of trigonome...

2018-02-20 Thread misutoth
Github user misutoth commented on a diff in the pull request:

https://github.com/apache/spark/pull/20618#discussion_r169287270
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala
 ---
@@ -262,6 +273,11 @@ case class Cos(child: Expression) extends 
UnaryMathExpression(math.cos, "COS")
 
 @ExpressionDescription(
   usage = "_FUNC_(expr) - Returns the hyperbolic cosine of `expr`.",
+  arguments =
+"""
--- End diff --

You are right, I will fix this.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20618: [SPARK-23329][SQL] Fix documentation of trigonome...

2018-02-19 Thread misutoth
Github user misutoth commented on a diff in the pull request:

https://github.com/apache/spark/pull/20618#discussion_r169187041
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala
 ---
@@ -804,7 +858,6 @@ case class Pow(left: Expression, right: Expression)
   }
 }
 
-
--- End diff --

Sure


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20618: [SPARK-23329][SQL] Fix documentation of trigonome...

2018-02-19 Thread misutoth
Github user misutoth commented on a diff in the pull request:

https://github.com/apache/spark/pull/20618#discussion_r169186706
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala
 ---
@@ -538,8 +559,14 @@ case class Sinh(child: Expression) extends 
UnaryMathExpression(math.sinh, "SINH"
   """)
 case class Sqrt(child: Expression) extends UnaryMathExpression(math.sqrt, 
"SQRT")
 
+// scalastyle:off line.size.limit
--- End diff --

Turned it on for all trigonometric functions.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20618: [SPARK-23329][SQL] Fix documentation of trigonome...

2018-02-19 Thread misutoth
Github user misutoth commented on a diff in the pull request:

https://github.com/apache/spark/pull/20618#discussion_r169186651
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala
 ---
@@ -512,16 +522,27 @@ case class Rint(child: Expression) extends 
UnaryMathExpression(math.rint, "ROUND
 case class Signum(child: Expression) extends 
UnaryMathExpression(math.signum, "SIGNUM")
 
 @ExpressionDescription(
-  usage = "_FUNC_(expr) - Returns the sine of `expr`.",
+  usage = "_FUNC_(expr) - Returns the sine of `expr`, as if computed by 
`java.lang.Math._FUNC_`.",
+  arguments =
+"""
--- End diff --

Yes, I applied to all places in scope.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark issue #20618: [SPARK-23329][SQL] Fix documentation of trigonometric fu...

2018-02-19 Thread misutoth
Github user misutoth commented on the issue:

https://github.com/apache/spark/pull/20618
  
Thanks @srowen and @HyukjinKwon for your comments so far ...


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20618: [SPARK-23329][SQL] Fix documentation of trigonome...

2018-02-19 Thread misutoth
Github user misutoth commented on a diff in the pull request:

https://github.com/apache/spark/pull/20618#discussion_r169100931
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala
 ---
@@ -770,7 +837,14 @@ case class Unhex(child: Expression) extends 
UnaryExpression with ImplicitCastInp
 
 // scalastyle:off line.size.limit
 @ExpressionDescription(
-  usage = "_FUNC_(expr1, expr2) - Returns the angle in radians between the 
positive x-axis of a plane and the point given by the coordinates (`expr1`, 
`expr2`).",
--- End diff --

Now I got it. Sorry, earlier I just misunderstood it. Are you proposing to 
move this case class inside the file to be next to the other trigonometric 
function? Based on the comments it seems the functions are group based on the 
number of arguments. This function has 2 arguments that is why it is under `// 
Binary math functions` section. Even though this does not seem to create big 
cohesion inside such a group I guess we do not want to reorganize the file now?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20618: [SPARK-23329][SQL] Fix documentation of trigonome...

2018-02-15 Thread misutoth
Github user misutoth commented on a diff in the pull request:

https://github.com/apache/spark/pull/20618#discussion_r168578156
  
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/functions.scala ---
@@ -1313,131 +1313,178 @@ object functions {
   
//
 
   /**
-   * Computes the cosine inverse of the given value; the returned angle is 
in the range
-   * 0.0 through pi.
+   * @param e the value whose arc cosine is to be returned
+   * @return  cosine inverse of the given value in the range of 0.0 
through pi,
--- End diff --

I am not sure what you mean on _above_. Do you mean reverting this part of 
the change?

How about simply `@return the angle whose cosine is 'e'` and refer to 
java.lang.Math for further details?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20618: [SPARK-23329][SQL] Fix documentation of trigonome...

2018-02-15 Thread misutoth
Github user misutoth commented on a diff in the pull request:

https://github.com/apache/spark/pull/20618#discussion_r168531352
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala
 ---
@@ -770,7 +837,14 @@ case class Unhex(child: Expression) extends 
UnaryExpression with ImplicitCastInp
 
 // scalastyle:off line.size.limit
 @ExpressionDescription(
-  usage = "_FUNC_(expr1, expr2) - Returns the angle in radians between the 
positive x-axis of a plane and the point given by the coordinates (`expr1`, 
`expr2`).",
+  usage = "_FUNC_(exprY, exprX) - Returns the angle in radians between the 
positive x-axis of a plane and the point given by the coordinates (`exprX`, 
`exprY`), " +
+"as if computed by `java.lang.Math._FUNC_`.",
+  arguments =
+"""
+Arguments:
+  * exprY - the ordinate coordinate
+  * exprX - the abscissa coordinate
--- End diff --

Sure, I will fix that. I borrowed that from java.lang.Math ... which 
sometimes is more complicated than it should be.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20618: [SPARK-23329][SQL] Fix documentation of trigonome...

2018-02-15 Thread misutoth
Github user misutoth commented on a diff in the pull request:

https://github.com/apache/spark/pull/20618#discussion_r168526385
  
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/functions.scala ---
@@ -1313,131 +1313,178 @@ object functions {
   
//
 
   /**
-   * Computes the cosine inverse of the given value; the returned angle is 
in the range
-   * 0.0 through pi.
+   * @param e the value whose arc cosine is to be returned
+   * @return  cosine inverse of the given value in the range of 0.0 
through pi,
+   *  as if computed by [[java.lang.Math#acos]]
*
* @group math_funcs
* @since 1.4.0
*/
   def acos(e: Column): Column = withExpr { Acos(e.expr) }
 
   /**
-   * Computes the cosine inverse of the given column; the returned angle 
is in the range
-   * 0.0 through pi.
+   * @param colName the value whose arc cosine is to be returned
+   * @returncosine inverse of the given value in the range of 0.0 
through pi,
+   *as if computed by [[java.lang.Math#acos]]
*
* @group math_funcs
* @since 1.4.0
*/
-  def acos(columnName: String): Column = acos(Column(columnName))
+  def acos(colName: String): Column = acos(Column(colName))
--- End diff --

columnName was too long and it ran into the description in the generated 
doc.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20618: [SPARK-23329][SQL] Fix documentation of trigonome...

2018-02-15 Thread misutoth
Github user misutoth commented on a diff in the pull request:

https://github.com/apache/spark/pull/20618#discussion_r168525504
  
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/functions.scala ---
@@ -1313,131 +1313,178 @@ object functions {
   
//
 
   /**
-   * Computes the cosine inverse of the given value; the returned angle is 
in the range
-   * 0.0 through pi.
+   * @param e the value whose arc cosine is to be returned
+   * @return  cosine inverse of the given value in the range of 0.0 
through pi,
--- End diff --

Ok.

Then I will cut the part about the domains and ranges.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20618: [SPARK-23329][SQL] Fix documentation of trigonome...

2018-02-15 Thread misutoth
Github user misutoth commented on a diff in the pull request:

https://github.com/apache/spark/pull/20618#discussion_r168521316
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala
 ---
@@ -262,6 +285,11 @@ case class Cos(child: Expression) extends 
UnaryMathExpression(math.cos, "COS")
 
 @ExpressionDescription(
   usage = "_FUNC_(expr) - Returns the hyperbolic cosine of `expr`.",
+  arguments =
+"""
+Arguments:
+  * expr - number whose hyperbolic consine is to be returned.
--- End diff --

I was not very familiar with hyperbolic functions so I followed the way 
this is described in java.lang.Math. But hyperbolic angle is really what this 
parameter actually is.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #20618: [SPARK-23329][SQL] Fix documentation of trigonome...

2018-02-15 Thread misutoth
GitHub user misutoth opened a pull request:

https://github.com/apache/spark/pull/20618

[SPARK-23329][SQL] Fix documentation of trigonometric functions

## What changes were proposed in this pull request?

Provide more details in trigonometric function documentations. Referenced 
`java.lang.Math` for further details in the descriptions. 
## How was this patch tested?

Ran full build, checked generated documentation manually

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/misutoth/spark trigonometric-doc

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/spark/pull/20618.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #20618


commit 25c329b4f93b407f53d87e8199444e83b6a1be15
Author: Mihaly Toth <mtoth@...>
Date:   2018-02-13T14:34:26Z

[SPARK-23329][SQL] Fix documentation of trigonometric functions




---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] lucene-solr pull request #311: SOLR-11873: Use a time based expiration cache...

2018-01-23 Thread misutoth
GitHub user misutoth opened a pull request:

https://github.com/apache/lucene-solr/pull/311

SOLR-11873: Use a time based expiration cache for one off HDFS FileSy…

…stem instances in all functions.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/misutoth/lucene-solr master-hdfsfix

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/lucene-solr/pull/311.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #311


commit 53ba8bd68f025aa786c40e407b29f354bd12e7f5
Author: Mihaly Toth <misutoth@...>
Date:   2018-01-23T18:58:11Z

SOLR-11873: Use a time based expiration cache for one off HDFS FileSystem 
instances in all functions.




---

-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org