-
Notifications
You must be signed in to change notification settings - Fork 28.3k
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.
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
[SPARK-8218][SQL] Binary log math function update. #6871
Conversation
Some minor updates based on after merging apache#6725.
cc @davies for review. |
Test build #35097 has finished for PR 6871 at commit
|
Test build #35155 has finished for PR 6871 at commit
|
Jenkins, retest this please. |
Test build #35175 has finished for PR 6871 at commit
|
|
||
/** | ||
* Natural log, i.e. using e as the base. | ||
*/ | ||
def this(child: Expression) = { | ||
this(EulerNumber(), child) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to remove this single parameter constructor, because this would make both ln
and log(c)
express the same thing in a SQL expression.
And also complex the binary log here to check its first argument branch in both eval
& genCode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's a good idea. Can you fix it in your pr? I'm gonig to merge this one first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, already fix this in my pr. :)
I like that idea. |
Some minor updates based on after merging apache#6725. Author: Reynold Xin <rxin@databricks.com> Closes apache#6871 from rxin/log and squashes the following commits: ab51542 [Reynold Xin] Use JVM log 76fc8de [Reynold Xin] Fixed arg. a7c1522 [Reynold Xin] [SPARK-8218][SQL] Binary log math function update.
Some minor updates based on after merging #6725.