Skip to content

[SPARK-12164] [SQL] Decode the encoded values and then display #10215

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

Jump to bottom
Closed
wants to merge 5 commits into from
Closed

[SPARK-12164] [SQL] Decode the encoded values and then display #10215

wants to merge 5 commits into from

Conversation

@gatorsmile
Copy link
Member

@gatorsmile gatorsmile commented Dec 9, 2015

Based on the suggestions from @marmbrus @cloud-fan in #10165 , this PR is to print the decoded values(user objects) in Dataset.show

    implicit val kryoEncoder = Encoders.kryo[KryoClassData]
    val ds = Seq(KryoClassData("a", 1), KryoClassData("b", 2), KryoClassData("c", 3)).toDS()
    ds.show(20, false);

The current output is like

+--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
|value                                                                                                                                                                                 |
+--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
|[1, 0, 111, 114, 103, 46, 97, 112, 97, 99, 104, 101, 46, 115, 112, 97, 114, 107, 46, 115, 113, 108, 46, 75, 114, 121, 111, 67, 108, 97, 115, 115, 68, 97, 116, -31, 1, 1, -126, 97, 2]|
|[1, 0, 111, 114, 103, 46, 97, 112, 97, 99, 104, 101, 46, 115, 112, 97, 114, 107, 46, 115, 113, 108, 46, 75, 114, 121, 111, 67, 108, 97, 115, 115, 68, 97, 116, -31, 1, 1, -126, 98, 4]|
|[1, 0, 111, 114, 103, 46, 97, 112, 97, 99, 104, 101, 46, 115, 112, 97, 114, 107, 46, 115, 113, 108, 46, 75, 114, 121, 111, 67, 108, 97, 115, 115, 68, 97, 116, -31, 1, 1, -126, 99, 6]|
+--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+

After the fix, it will be like the below if and only if the users override the toString function in the class KryoClassData

override def toString: String = s"KryoClassData($a, $b)"
+-------------------+
|value              |
+-------------------+
|KryoClassData(a, 1)|
|KryoClassData(b, 2)|
|KryoClassData(c, 3)|
+-------------------+

If users do not override the toString function, the results will be like

+---------------------------------------+
|value                                  |
+---------------------------------------+
|org.apache.spark.sql.KryoClassData@68ef|
|org.apache.spark.sql.KryoClassData@6915|
|org.apache.spark.sql.KryoClassData@693b|
+---------------------------------------+

Question: Should we add another optional parameter in the function show? It will decide if the function show will display the hex values or the object values?

@SparkQA
Copy link

SparkQA commented Dec 9, 2015

Test build #47405 has finished for PR 10215 at commit 1e1ad19.

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

liancheng
liancheng reviewed Dec 11, 2015
View reviewed changes
sql/core/src/main/scala/org/apache/spark/sql/DataFrame.scala Outdated
Copy link
Contributor

@liancheng liancheng Dec 11, 2015

Choose a reason for hiding this comment

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

Please revert these white space changes (I hate IDEA 15 too...)

Copy link
Member Author

@gatorsmile gatorsmile Dec 11, 2015

Choose a reason for hiding this comment

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

Yeah. Just upgraded to 15.0.2 The problem still exists...

@SparkQA
Copy link

SparkQA commented Dec 11, 2015

Test build #47597 has finished for PR 10215 at commit 48e049b.

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

marmbrus
marmbrus reviewed Dec 15, 2015
View reviewed changes
sql/core/src/main/scala/org/apache/spark/sql/execution/Queryable.scala Outdated
Copy link
Contributor

@marmbrus marmbrus Dec 15, 2015

Choose a reason for hiding this comment

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

This indenting is wrong. https://cwiki.apache.org/confluence/display/SPARK/Spark+Code+Style+Guide#SparkCodeStyleGuide-Indentation

4 space for function args that wrap.

Copy link
Member Author

@gatorsmile gatorsmile Dec 15, 2015

Choose a reason for hiding this comment

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

Sorry, correct it now.

@marmbrus
Copy link
Contributor

marmbrus commented Dec 15, 2015

Can you add tests too? Probably in DataFrameSuite.

@gatorsmile
Copy link
Member Author

gatorsmile commented Dec 15, 2015

Sure, will do. Thanks!

Update: To reuse the existing class KryoData, I put a test case in DataSetSuite. Please let me know if I should move it to DataFrameSuite.

cloud-fan
cloud-fan reviewed Dec 15, 2015
View reviewed changes
sql/core/src/main/scala/org/apache/spark/sql/Dataset.scala Outdated
Copy link
Contributor

@cloud-fan cloud-fan Dec 15, 2015

Choose a reason for hiding this comment

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

we should use javadoc style instead of scaladoc. i.e.

/**
 *
 *
 */

Copy link
Member Author

@gatorsmile gatorsmile Dec 15, 2015

Choose a reason for hiding this comment

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

Yeah. Will correct all these. Thanks!

@gatorsmile
Copy link
Member Author

gatorsmile commented Dec 15, 2015

To simplify the possible code merge, I combined the code changes in another related PR: #10165. Thank you!

@gatorsmile gatorsmile mentioned this pull request Dec 15, 2015
Closed
@SparkQA
Copy link

SparkQA commented Dec 15, 2015

Test build #47717 has finished for PR 10215 at commit 1c33441.

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

marmbrus
marmbrus reviewed Dec 15, 2015
View reviewed changes
sql/core/src/main/scala/org/apache/spark/sql/Dataset.scala Outdated
Copy link
Contributor

@marmbrus marmbrus Dec 15, 2015

Choose a reason for hiding this comment

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

Please undo these changes.

Copy link
Member Author

@gatorsmile gatorsmile Dec 15, 2015

Choose a reason for hiding this comment

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

So sorry for introducing these inconsistency. Undo the changes.

@SparkQA
Copy link

SparkQA commented Dec 15, 2015

Test build #47745 has finished for PR 10215 at commit 5553b0b.

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

@marmbrus
Copy link
Contributor

marmbrus commented Dec 16, 2015

Thanks, merging to master.

@asfgit asfgit closed this in edf65cd Dec 16, 2015
@gatorsmile gatorsmile deleted the showDecodedValue branch December 30, 2015 17:14
marmbrus pushed a commit to marmbrus/spark that referenced this pull request Jan 7, 2016
@gatorsmile @marmbrus
[SPARK-12164][SQL] Decode the encoded values and then display
1a12f08 
Based on the suggestions from marmbrus cloud-fan in apache#10165 , this PR is to print the decoded values(user objects) in `Dataset.show`
```scala
    implicit val kryoEncoder = Encoders.kryo[KryoClassData]
    val ds = Seq(KryoClassData("a", 1), KryoClassData("b", 2), KryoClassData("c", 3)).toDS()
    ds.show(20, false);
```
The current output is like
```
+--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
|value                                                                                                                                                                                 |
+--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
|[1, 0, 111, 114, 103, 46, 97, 112, 97, 99, 104, 101, 46, 115, 112, 97, 114, 107, 46, 115, 113, 108, 46, 75, 114, 121, 111, 67, 108, 97, 115, 115, 68, 97, 116, -31, 1, 1, -126, 97, 2]|
|[1, 0, 111, 114, 103, 46, 97, 112, 97, 99, 104, 101, 46, 115, 112, 97, 114, 107, 46, 115, 113, 108, 46, 75, 114, 121, 111, 67, 108, 97, 115, 115, 68, 97, 116, -31, 1, 1, -126, 98, 4]|
|[1, 0, 111, 114, 103, 46, 97, 112, 97, 99, 104, 101, 46, 115, 112, 97, 114, 107, 46, 115, 113, 108, 46, 75, 114, 121, 111, 67, 108, 97, 115, 115, 68, 97, 116, -31, 1, 1, -126, 99, 6]|
+--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
```
After the fix, it will be like the below if and only if the users override the `toString` function in the class `KryoClassData`
```scala
override def toString: String = s"KryoClassData($a, $b)"
```
```
+-------------------+
|value              |
+-------------------+
|KryoClassData(a, 1)|
|KryoClassData(b, 2)|
|KryoClassData(c, 3)|
+-------------------+
```

If users do not override the `toString` function, the results will be like
```
+---------------------------------------+
|value                                  |
+---------------------------------------+
|org.apache.spark.sql.KryoClassData68ef|
|org.apache.spark.sql.KryoClassData6915|
|org.apache.spark.sql.KryoClassData693b|
+---------------------------------------+
```

Question: Should we add another optional parameter in the function `show`? It will decide if the function `show` will display the hex values or the object values?

Author: gatorsmile <[email protected]>

Closes apache#10215 from gatorsmile/showDecodedValue.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@gatorsmile @SparkQA @marmbrus @liancheng @cloud-fan