Skip to content

[SPARK-17480][SQL][FOLLOWUP] Fix more instances which calls List.length/size which is O(n) #15093

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
HyukjinKwon wants to merge 6 commits into from
+31 −33
Closed

[SPARK-17480][SQL][FOLLOWUP] Fix more instances which calls List.length/size which is O(n) #15093

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fbc3419
Fix more same instances
HyukjinKwon Sep 14, 2016
ecd3dde
Last instance
HyukjinKwon Sep 14, 2016
996e292
Revert unrelated changes and use funtional transformation for a case
HyukjinKwon Sep 14, 2016
cb536a4
Address comment
HyukjinKwon Sep 15, 2016
f5e8131
Fix comments
HyukjinKwon Sep 17, 2016
8a3d293
Correct the comments more
HyukjinKwon Sep 17, 2016
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
Revert unrelated changes and use funtional transformation for a case
  • Loading branch information
@HyukjinKwon
HyukjinKwon committed Sep 14, 2016
commit 996e2922aae07675524afd2e3d6f02fc2310dedd
8 changes: 4 additions & 4 deletions mllib/src/main/scala/org/apache/spark/mllib/linalg/distributed/RowMatrix.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -586,7 +586,7 @@ class RowMatrix @Since("1.0.0") (
colMags: Array[Double],
gamma: Double): CoordinateMatrix = {
require(gamma > 1.0, s"Oversampling should be greater than 1: $gamma")
require(colMags.length == this.numCols(), "Number of magnitudes didn't match column dimension")
require(colMags.size == this.numCols(), "Number of magnitudes didn't match column dimension")
val sg = math.sqrt(gamma) // sqrt(gamma) used many times

// Don't divide by zero for those columns with zero magnitude
Expand All @@ -601,11 +601,11 @@ class RowMatrix @Since("1.0.0") (
val q = qBV.value

val rand = new XORShiftRandom(indx)
val scaled = new Array[Double](p.length)
val scaled = new Array[Double](p.size)
iter.flatMap { row =>
row match {
case SparseVector(size, indices, values) =>
val nnz = indices.length
val nnz = indices.size
var k = 0
while (k < nnz) {
scaled(k) = values(k) / q(indices(k))
Expand All @@ -630,7 +630,7 @@ class RowMatrix @Since("1.0.0") (
buf
}.flatten
case DenseVector(values) =>
val n = values.length
val n = values.size
var i = 0
while (i < n) {
scaled(i) = values(i) / q(i)
Expand Down
8 changes: 1 addition & 7 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1658,20 +1658,14 @@ class Analyzer(
// Third, for every Window Spec, we add a Window operator and set currentChild as the
// child of it.
var currentChild = child
var i = 0
val size = groupedWindowExpressions.size
while (i < size) {
val ((partitionSpec, orderSpec), windowExpressions) = groupedWindowExpressions(i)
groupedWindowExpressions.foreach { case ((partitionSpec, orderSpec), windowExpressions) =>
Copy link
Member Author

@HyukjinKwon HyukjinKwon Sep 15, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @srowen , It seems groupedWindowExpressions is Seq[((Seq[Expression], Seq[SortOrder]), ArrayBuffer[NamedExpression])] but the desired output is LogicalPlan. So, it seems I can't directly use fold or reduce simply. So, could this just use foreach rather than others? Otherwise, please teach me for a better expression.

Copy link
Member

@srowen srowen Sep 15, 2016

Choose a reason for hiding this comment

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

Well currentChild is a Window, and that's the only thing whose computation is changed here. How about:

      val currentChild =
        groupedWindowExpressions.foldLeft(child) {
          case (last, ((partitionSpec, orderSpec), windowExpressions)) =>
            Window(windowExpressions, partitionSpec, orderSpec, last)
        }

Copy link
Member Author

@HyukjinKwon HyukjinKwon Sep 15, 2016

Choose a reason for hiding this comment

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

Ah, that's clear and better. I will try. Thanks!

// Set currentChild to the newly created Window operator.
currentChild =
Window(
windowExpressions,
partitionSpec,
orderSpec,
currentChild)

// Move to next Window Spec.
i += 1
}

// Finally, we create a Project to output currentChild's output
Expand Down