Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Fix scala style.
  • Loading branch information
viirya committed Jun 10, 2015
commit 21c3bfdec9f30efee6d679f87d6040cc5d9d9f13
2 changes: 1 addition & 1 deletion python/pyspark/sql/functions.py
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ def _():
'polar coordinates (r, theta).',
'hypot': 'Computes `sqrt(a^2^ + b^2^)` without intermediate overflow or underflow.',
'pow': 'Returns the value of the first argument raised to the power of the second argument.',
'logarithm': 'Returns the first argument-based logarithm of the second argument',
'log': 'Returns the first argument-based logarithm of the second argument',
Copy link
Contributor

Choose a reason for hiding this comment

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

can we define this function directly instead of using this magic?

}

_window_functions = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ case class Pow(left: Expression, right: Expression)
}

object Logarithm {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need this? We should assume people will use the ln instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because we want to support the usage of df.selectExpr("log(a)", "log(2.0, a)", "log(b)").

Copy link
Contributor

Choose a reason for hiding this comment

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

take a look at #6806

Copy link
Member Author

Choose a reason for hiding this comment

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

ok. it solves this problem.

def apply(exprs: Seq[Expression]) = {
def apply(exprs: Seq[Expression]): Expression = {
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't need this one do we? just need an apply with one argument, along with the original case class' constructor.

if (exprs.length == 1) {
new Log(exprs(0))
} else if (exprs.length == 2) {
Expand All @@ -213,7 +213,7 @@ object Logarithm {
sys.error(s"Log only accepts two input expressions")
}
}
def apply(child: Expression) = new Log(child)
def apply(child: Expression): Expression = new Log(child)
}

case class Logarithm(left: Expression, right: Expression)
Copy link
Contributor

Choose a reason for hiding this comment

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

We seem to be doing this throughout the file, but it seems pretty confusing to me to be using left and right instead of something like value and base. I'm not sure this is worth whatever code reuse we are getting.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh - one thing is that left/right is coming from BinaryExpression

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that is what I meant saying it wasn't worth whatever code reuse we are getting. The other option would be to name the arguments and have def left = base, etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Inheriting from BinaryMathExpression seems like a bad idea to me. It is forcing the arguments to have weird names and is resulting in dead code.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ class DataFrameFunctionsSuite extends QueryTest {
checkAnswer(
df.select(org.apache.spark.sql.functions.log("a"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: When wrapping, wrap all arguments. Also the indent below is wrong.

org.apache.spark.sql.functions.log(2.0, "a"),
org.apache.spark.sql.functions.log("b")), Row(math.log(123), math.log(123) / math.log(2), null))
org.apache.spark.sql.functions.log("b")),
Row(math.log(123), math.log(123) / math.log(2), null))
}
}