-
Notifications
You must be signed in to change notification settings - Fork 29k
[DOCS][MINOR] Scaladoc fixes (aka typo hunting) #18074
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
Conversation
|
|
||
| /** | ||
| * Return all metadata that describes more details of this SparkPlan. | ||
| * Returns all metadata that describes more details of this SparkPlan. |
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.
You can even turn this into @return but no big deal.
| * | ||
| * The main method is the agg function, which has multiple variants. This class also contains | ||
| * convenience some first order statistics such as mean, sum for convenience. | ||
| * The main method is the [[agg]] function, which has multiple variants. This class also contains |
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.
These look fine but have you had a chance to run the doc generation, to make sure scaladoc is OK with it?
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.
No scaladoc. I was focused on javadoc and seems I missed it. Fixed.
|
Test build #77255 has finished for PR 18074 at commit
|
|
Test build #77347 has finished for PR 18074 at commit
|
|
Test build #77348 has finished for PR 18074 at commit
|
| // Add a function and its function to the map for a given frame. | ||
| def collect(tpe: String, fr: SpecifiedWindowFrame, e: Expression, fn: Expression): Unit = { | ||
| def collect( | ||
| tpe: String, fr: SpecifiedWindowFrame, e: WindowExpression, fn: Expression): Unit = |
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.
This changed a type, which could be fine as it's an internal method, but why should it be the more specific type? this isn't a doc change
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'll revert it to not "pollute" the PR. It was much easier to read the code when the internals were less generic (that does nothing at all).
|
Test build #77352 has finished for PR 18074 at commit
|
|
Test build #77471 has finished for PR 18074 at commit
|
|
Test build #3766 has finished for PR 18074 at commit
|
|
Test build #77491 has finished for PR 18074 at commit
|
|
Hey @srowen could you review the changes again and accept possibly? Thanks! |
|
Test build #77500 has finished for PR 18074 at commit
|
| private[this] lazy val windowFrameExpressionFactoryPairs = { | ||
| type FrameKey = (String, FrameType, Option[Int], Option[Int]) | ||
| type ExpressionBuffer = mutable.Buffer[Expression] | ||
| type WindowExpressionBuffer = mutable.Buffer[WindowExpression] |
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.
@jaceklaskowski this is still not reverted. Please, back out any non-doc changes. I can't spend 4 rounds of review on things like this, it's not fair to anyone else waiting
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.
Sorry. Reverted the change manually and looks like it slipped through the cracks. Apologies.
|
Test build #77543 has finished for PR 18074 at commit
|
|
Merged to master |
What changes were proposed in this pull request?
Minor changes to scaladoc
How was this patch tested?
Local build