Skip to content

[SPARK-6917] [SQL] DecimalType is not read back when non-native type exists #6558

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

Closed
wants to merge 4 commits into from

Conversation

davies
Copy link
Contributor

@davies davies commented Jun 1, 2015

cc @yhuai

@JoshRosen
Copy link
Contributor

Regression test?

@yhuai
Copy link
Contributor

yhuai commented Jun 1, 2015

Yeah, let's add a test.

@davies
Copy link
Contributor Author

davies commented Jun 1, 2015

regression test added.

@SparkQA
Copy link

SparkQA commented Jun 1, 2015

Test build #33917 has finished for PR 6558 at commit 3b4a94f.

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

@SparkQA
Copy link

SparkQA commented Jun 2, 2015

Test build #33927 has finished for PR 6558 at commit b43845c.

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

@@ -244,7 +244,7 @@ private[parquet] abstract class CatalystConverter extends GroupConverter {
* Read a decimal value from a Parquet Binary into "dest". Only supports decimals that fit in
* a long (i.e. precision <= 18)
*/
protected[parquet] def readDecimal(dest: Decimal, value: Binary, ctype: DecimalType): Unit = {
protected[parquet] def readDecimal(dest: Decimal, value: Binary, ctype: DecimalType): Decimal = {
Copy link
Contributor

Choose a reason for hiding this comment

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

oh, I just realized that when we reuse the Decimal value, we do not really need to use the returned value. But, we have another place that needs the returned value. Can we add a comment at here?

@davies
Copy link
Contributor Author

davies commented Jun 2, 2015

@yhuai fixed.

@SparkQA
Copy link

SparkQA commented Jun 2, 2015

Test build #33937 has finished for PR 6558 at commit 48cc57c.

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

@SparkQA
Copy link

SparkQA commented Jun 2, 2015

Test build #33938 has finished for PR 6558 at commit c877ca8.

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

@asfgit asfgit closed this in bcb47ad Jun 2, 2015
asfgit pushed a commit that referenced this pull request Jun 2, 2015
…exists

cc yhuai

Author: Davies Liu <[email protected]>

Closes #6558 from davies/decimalType and squashes the following commits:

c877ca8 [Davies Liu] Update ParquetConverter.scala
48cc57c [Davies Liu] Update ParquetConverter.scala
b43845c [Davies Liu] add test
3b4a94f [Davies Liu] DecimalType is not read back when non-native type exists

(cherry picked from commit bcb47ad)
Signed-off-by: Reynold Xin <[email protected]>
jeanlyn pushed a commit to jeanlyn/spark that referenced this pull request Jun 12, 2015
…exists

cc yhuai

Author: Davies Liu <[email protected]>

Closes apache#6558 from davies/decimalType and squashes the following commits:

c877ca8 [Davies Liu] Update ParquetConverter.scala
48cc57c [Davies Liu] Update ParquetConverter.scala
b43845c [Davies Liu] add test
3b4a94f [Davies Liu] DecimalType is not read back when non-native type exists
nemccarthy pushed a commit to nemccarthy/spark that referenced this pull request Jun 19, 2015
…exists

cc yhuai

Author: Davies Liu <[email protected]>

Closes apache#6558 from davies/decimalType and squashes the following commits:

c877ca8 [Davies Liu] Update ParquetConverter.scala
48cc57c [Davies Liu] Update ParquetConverter.scala
b43845c [Davies Liu] add test
3b4a94f [Davies Liu] DecimalType is not read back when non-native type exists
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.

4 participants