Skip to content

Conversation

@kiszk
Copy link
Member

@kiszk kiszk commented Aug 8, 2018

What changes were proposed in this pull request?

This PR simplified code generation for ArrayDistinct. #21966 enabled code generation only if the type can be specialized by the hash set. This PR follows this strategy.

Optimization of null handling will be implemented in #21912.

How was this patch tested?

Existing UTs

@SparkQA
Copy link

SparkQA commented Aug 8, 2018

Test build #94450 has finished for PR 22044 at commit 9d0cb10.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@kiszk
Copy link
Member Author

kiszk commented Aug 9, 2018

retest this please

@SparkQA
Copy link

SparkQA commented Aug 9, 2018

Test build #94461 has finished for PR 22044 at commit 9d0cb10.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@kiszk
Copy link
Member Author

kiszk commented Aug 9, 2018

retest this please

@SparkQA
Copy link

SparkQA commented Aug 9, 2018

Test build #94471 has finished for PR 22044 at commit 9d0cb10.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@kiszk
Copy link
Member Author

kiszk commented Aug 9, 2018

retest this please

}

@transient protected lazy val (hsPostFix, hsTypeName) = {
val ptName = CodeGenerator.primitiveTypeName (elementType)
Copy link
Member

Choose a reason for hiding this comment

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

nit: extra space after primitiveTypeName.

}

private def setNotNullValue(isPrimitive: Boolean,
private def setNotNullValue(
Copy link
Member

Choose a reason for hiding this comment

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

We can inline this?

case _ => false
}

@transient protected lazy val canUseSpecializedHashSet = elementType match {
Copy link
Member

Choose a reason for hiding this comment

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

Can we extract those common methods?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can do so. To minimize the changes due to remaining time for cutting, I would like to do this in another PR #21912.

s"$distinctArray.set$primitiveValueTypeName($pos, $getValue1)"
}

private def setValueForFastEval(
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should rename this.

@SparkQA
Copy link

SparkQA commented Aug 9, 2018

Test build #94487 has finished for PR 22044 at commit 89d0664.

  • This patch fails MiMa tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

val openHashSet = classOf[OpenHashSet[_]].getName
val hs = ctx.freshName("hs")
val classTag = s"scala.reflect.ClassTag$$.MODULE$$.$hsTypeName()"
val getValue1 = CodeGenerator.getValue(array, elementType, i)
Copy link
Member

Choose a reason for hiding this comment

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

nit: val getValue?

@SparkQA
Copy link

SparkQA commented Aug 9, 2018

Test build #94475 has finished for PR 22044 at commit 9d0cb10.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@ueshin
Copy link
Member

ueshin commented Aug 9, 2018

LGTM pending Jenkins.

@SparkQA
Copy link

SparkQA commented Aug 9, 2018

Test build #94488 has finished for PR 22044 at commit ec262ab.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@ueshin
Copy link
Member

ueshin commented Aug 10, 2018

Thanks! merging to master.

@asfgit asfgit closed this in ab1029f Aug 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants