[GitHub] spark pull request #20787: Documenting months_between direction
Github user HyukjinKwon commented on a diff in the pull request: https://github.com/apache/spark/pull/20787#discussion_r175985253 --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala --- @@ -1115,13 +1115,17 @@ case class AddMonths(startDate: Expression, numMonths: Expression) override def prettyName: String = "add_months" } - /** - * Returns number of months between dates date1 and date2. - */ + * Returns number of months between dates `timestamp1` and `timestamp2`. + * If `timestamp` is later than `timestamp2`, then the result is positive. + * If `timestamp1` and `timestamp2` are on the same day of month, or both + * are the last day of month, returns an integer (time of day will be ignored). + * Otherwise, the difference is calculated based on 31 days per month, and + * rounded to 8 digits. +*/ // scalastyle:off line.size.limit @ExpressionDescription( - usage = "_FUNC_(timestamp1, timestamp2) - Returns number of months between `timestamp1` and `timestamp2`.", + usage = "_FUNC_(timestamp1, timestamp2) - Returns number of months between `timestamp1` and `timestamp2`. Positive if `timestamp1` is later than `timestamp2`", --- End diff -- You could do either ```scala @ExpressionDescription( usage = """ _FUNC_(timestamp1, timestamp2) - blablabla blabla blabla """, ... ``` Let's add the description here too. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20787: Documenting months_between direction
Github user HyukjinKwon commented on a diff in the pull request: https://github.com/apache/spark/pull/20787#discussion_r175983804 --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala --- @@ -881,10 +881,10 @@ object DateTimeUtils { * Returns number of months between time1 and time2. time1 and time2 are expressed in * microseconds since 1.1.1970. * - * If time1 and time2 having the same day of month, or both are the last day of month, - * it returns an integer (time under a day will be ignored). + * If time1 and time2 are on the same day of month, or both are the last day of month, + * returns an integer (time under a day will be ignored). --- End diff -- It seems a bit awkward because it actually returns a double. Shall we fix this like .. `returns an integer (time under a day will be ignored)` -> `time under a day will be ignored.`? --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20787: Documenting months_between direction
Github user HyukjinKwon commented on a diff in the pull request: https://github.com/apache/spark/pull/20787#discussion_r175983334 --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala --- @@ -1115,13 +1115,17 @@ case class AddMonths(startDate: Expression, numMonths: Expression) override def prettyName: String = "add_months" } - --- End diff -- Let's revert this change back. Seems unrelated. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20787: Documenting months_between direction
Github user HyukjinKwon commented on a diff in the pull request: https://github.com/apache/spark/pull/20787#discussion_r175982564 --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala --- @@ -1115,13 +1115,17 @@ case class AddMonths(startDate: Expression, numMonths: Expression) override def prettyName: String = "add_months" } - /** - * Returns number of months between dates date1 and date2. - */ + * Returns number of months between dates `timestamp1` and `timestamp2`. --- End diff -- Hm, this should have been caught by Scala linter because we follow Java style comment. See "Code documentation style" in http://spark.apache.org/contributing.html --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20787: Documenting months_between direction
Github user HyukjinKwon commented on a diff in the pull request: https://github.com/apache/spark/pull/20787#discussion_r175668391 --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala --- @@ -1115,13 +1115,18 @@ case class AddMonths(startDate: Expression, numMonths: Expression) override def prettyName: String = "add_months" } - /** - * Returns number of months between dates date1 and date2. - */ + * Returns number of months between dates `date1` and `date2`. + * If `date1` is later than `date2`, then the result is positive. --- End diff -- `date` -> `timestamp` --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20787: Documenting months_between direction
Github user HyukjinKwon commented on a diff in the pull request: https://github.com/apache/spark/pull/20787#discussion_r173619271 --- Diff: sql/core/src/main/scala/org/apache/spark/sql/functions.scala --- @@ -2708,7 +2708,8 @@ object functions { def minute(e: Column): Column = withExpr { Minute(e.expr) } /** - * Returns number of months between dates `date1` and `date2`. + * Returns number of months between dates `date1` and `date2`. If `date1` is later than `date2`, + * then the result is positive. --- End diff -- Can we resemble this: https://github.com/apache/spark/blob/6e36d8d56279a2c5c92c8df8e89ee99b514817e7/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala#L884-L888 ? --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20787: Documenting months_between direction
GitHub user aditkumar opened a pull request: https://github.com/apache/spark/pull/20787 Documenting months_between direction ## What changes were proposed in this pull request? It's useful to know what relationship between date1 and date2 result in a positive number. You can merge this pull request into a Git repository by running: $ git pull https://github.com/aditkumar/spark master Alternatively you can review and apply these changes as the patch at: https://github.com/apache/spark/pull/20787.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 #20787 commit de207321717c607eccb280aa0b77b3393ef4a04b Author: aditkumar Date: 2018-03-09T16:08:38Z Date direction in months_between pyspark commit 5f8a23fea197d60f870177ed3272154eab60bca9 Author: aditkumar Date: 2018-03-09T16:11:15Z datediff direction in months_between commit 1f5c543cf8e03b32691bbe477b09c4bce02e6f4d Author: aditkumar Date: 2018-03-09T16:16:01Z date direction in months_between --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org