Skip to content

[MINOR][DOC] Fix comments of ConvertToLocalRelation rule #23273

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.

Sign up for GitHub

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

Closed
seancxmao wants to merge 2 commits into from
Closed

[MINOR][DOC] Fix comments of ConvertToLocalRelation rule #23273

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dfd0f71
[SPARK-25212][SQL][FOLLOWUP][DOC] Fix comments of ConvertToLocalRelat…
seancxmao Dec 10, 2018
1cca847
fix according to comments
seancxmao Dec 12, 2018
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
Next Next commit
[SPARK-25212][SQL][FOLLOWUP][DOC] Fix comments of ConvertToLocalRelat… 
…ion rule
  • Loading branch information
@seancxmao
seancxmao committed Dec 10, 2018
commit dfd0f71afb8d95253ea4f64d00cea53c306b6e1c
14 changes: 7 additions & 7 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,11 +131,11 @@ abstract class Optimizer(sessionCatalog: SessionCatalog)
// since the other rules might make two separate Unions operators adjacent.
Batch("Union", Once,
CombineUnions) ::
// run this once earlier. this might simplify the plan and reduce cost of optimizer.
// for example, a query such as Filter(LocalRelation) would go through all the heavy
// Run this once earlier. This might simplify the plan and reduce cost of optimizer.
// For example, a query such as Filter(LocalRelation) would go through all the heavy
// optimizer rules that are triggered when there is a filter
// (e.g. InferFiltersFromConstraints). if we run this batch earlier, the query becomes just
// LocalRelation and does not trigger many rules
// (e.g. InferFiltersFromConstraints). If we run this batch earlier, the query becomes just
// LocalRelation and does not trigger many rules.
Batch("LocalRelation early", fixedPoint,
ConvertToLocalRelation,
PropagateEmptyRelation) ::
Expand Down Expand Up @@ -1370,10 +1370,10 @@ object DecimalAggregates extends Rule[LogicalPlan] {
}

/**
* Converts local operations (i.e. ones that don't require data exchange) on LocalRelation to
* another LocalRelation.
* Converts local operations (i.e. ones that don't require data exchange) on [[LocalRelation]] to
* another [[LocalRelation]].
*
* This is relatively simple as it currently handles only 2 single case: Project and Limit.
* This rule currently handles 3 cases: [[Project]], [[Limit]] and [[Filter]].
Copy link
Member

@srowen srowen Dec 11, 2018

Choose a reason for hiding this comment

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

Maybe we should just not specify this in the docs. Are you sure the [[...]] links work when you generate scaladoc? the test builder doesn't check. I've seen many errors/warnings generated by some types of links.
If so again maybe easier to restrict this to merely removing this line.

Copy link
Contributor Author

@seancxmao seancxmao Dec 12, 2018

Choose a reason for hiding this comment

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

@srowen Sorry, I found that the links just do not work for scaladoc, though they works in IDE like Intellij IDEA. I should have generated the docs to see if there's any problem.

I have changed [[...]] to backticks in a new commit.

Also, I have some findings:

  • Because org/apache/spark/sql/catalyst is excluded for doc generation, I include catalyst in SparkBuild.scala and run build/sbt unidoc, there are many errors/warnings, should we fix these problems? Seems simple but many places.
  • As for [[...]], fully qualified class name should be used if a link points to a class in another package. If the link points to a class in the same package, package prefix could be omitted.

Copy link
Member

@srowen srowen Dec 12, 2018

Choose a reason for hiding this comment

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

catalyst isn't meant to be a public API (for our convenience, it's not all marked private in the source). That's why it's not in the docs and yeah that's why writing references to it won't work, as I recall.

For this comment I think it's simplest to remove it, or leave it but don't add references, or move it to an inline comment.

Copy link
Contributor Author

@seancxmao seancxmao Dec 12, 2018

Choose a reason for hiding this comment

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

Thanks a lot @srowen for your explanation. Since it's not a public API and the code is clear, it makes sense to remove this line, this can also eliminate the needs to modify this line when new cases are added. I have removed this line in the new commit.

*/
object ConvertToLocalRelation extends Rule[LogicalPlan] {
def apply(plan: LogicalPlan): LogicalPlan = plan transform {
Expand Down