Skip to content

Conversation

@viirya
Copy link
Member

@viirya viirya commented Dec 13, 2015

JIRA: https://issues.apache.org/jira/browse/SPARK-12293

Make LocalTableScan support UnsafeRow.

@SparkQA
Copy link

SparkQA commented Dec 13, 2015

Test build #47628 has finished for PR 10283 at commit ca4326a.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):\n * case class LocalRelation(output: Seq[Attribute], data: Seq[UnsafeRow] = Nil)\n

@SparkQA
Copy link

SparkQA commented Dec 14, 2015

Test build #47634 has finished for PR 10283 at commit a0a991a.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):\n * case class LocalRelation(output: Seq[Attribute], data: Seq[UnsafeRow] = Nil)\n

Copy link
Contributor

Choose a reason for hiding this comment

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

The return type of UnsafeProjection.apply is UnsafeRow already, looks like we don't need the asInstanceOf here?

@viirya
Copy link
Member Author

viirya commented Dec 14, 2015

I think the commits to fix these test errors are worth having their PR. I will create them later.

Copy link
Contributor

Choose a reason for hiding this comment

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

Normally Decimal should only be used inside spark SQL as the internal representation of decimal type, and we don't need to catch it here. Do we break it in tests?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. constructorFor will call dataTypeFor to determine if a type is ObjectType or not. If there is not case for Decimal, it will be recognized as ObjectType and causes bug.

@SparkQA
Copy link

SparkQA commented Dec 14, 2015

Test build #47651 has finished for PR 10283 at commit f0e6ac0.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):\n * case class LocalRelation(output: Seq[Attribute], data: Seq[UnsafeRow] = Nil)\n

Copy link
Contributor

Choose a reason for hiding this comment

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

If the unwrapped is null, the Invoke will return null, see https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects.scala#L172-L175

I think we don't need this extra If, did I miss something here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think you are right. I add this to make sure it is not possibly creating wrong values when I debug. I will remove these If.

@SparkQA
Copy link

SparkQA commented Dec 16, 2015

Test build #47829 has finished for PR 10283 at commit 97a81c3.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):\n * case class LocalRelation(output: Seq[Attribute], data: Seq[UnsafeRow] = Nil)\n

@SparkQA
Copy link

SparkQA commented Dec 17, 2015

Test build #47888 has finished for PR 10283 at commit 97d390f.

  • This patch fails from timeout after a configured wait of 250m.
  • This patch merges cleanly.
  • This patch adds no public classes.

@viirya
Copy link
Member Author

viirya commented Dec 17, 2015

retest this please.

@SparkQA
Copy link

SparkQA commented Dec 17, 2015

Test build #47911 has finished for PR 10283 at commit 97d390f.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):\n * case class LocalRelation(output: Seq[Attribute], data: Seq[UnsafeRow] = Nil)\n

@viirya
Copy link
Member Author

viirya commented Dec 17, 2015

retest this please.

@SparkQA
Copy link

SparkQA commented Dec 17, 2015

Test build #47920 has finished for PR 10283 at commit 97d390f.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):\n * case class LocalRelation(output: Seq[Attribute], data: Seq[UnsafeRow] = Nil)\n

Copy link
Contributor

Choose a reason for hiding this comment

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

we can use encoderFor[T] here

@viirya
Copy link
Member Author

viirya commented Dec 19, 2015

SQLUserDefinedType support for encoder is proposed in another pr #10390.

@viirya
Copy link
Member Author

viirya commented Dec 21, 2015

@cloud-fan I think I have addressed all your comments. These bugs found in implementing UnsafeRow support in LocalTableScan are submitted as other PRs with their tests, so you can review them better.

@SparkQA
Copy link

SparkQA commented Dec 21, 2015

Test build #48094 has finished for PR 10283 at commit 2500de3.

  • This patch fails from timeout after a configured wait of 250m.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):\n * case class LocalRelation(output: Seq[Attribute], data: Seq[UnsafeRow] = Nil)\n

@viirya
Copy link
Member Author

viirya commented Dec 21, 2015

retest this please.

@SparkQA
Copy link

SparkQA commented Dec 21, 2015

Test build #48101 has finished for PR 10283 at commit 2500de3.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):\n * case class LocalRelation(output: Seq[Attribute], data: Seq[UnsafeRow] = Nil)\n

@viirya viirya closed this Dec 30, 2015
@viirya viirya reopened this Dec 30, 2015
@viirya viirya closed this Jan 5, 2016
@viirya viirya deleted the unsaferow-localtablescan branch December 27, 2023 18:32
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