[SPARK-16004] [SQL] Correctly display "Last Access Time" of CatalogTable by bomeng · Pull Request #13720 · apache/spark

bomeng · 2016-06-16T23:25:31Z

What changes were proposed in this pull request?

A few issues found when running "describe extended | formatted [tableName]" command:

After creation of a table, the last access time is incorrectly displayed something like "Last Access Time: |Wed Dec 31 15:59:59 PST 1969", I think we should display as "UNKNOWN" as Hive does; the reason is the lastAccessTime was set to -1 and it was converted to Date.
~~Comments fields display "null" instead of empty string when commend is None;~~

How was this patch tested?

Currently, I have manually tested them - it is very straight-forward to test, but hard to write test cases for them.

SparkQA · 2016-06-17T00:44:59Z

Test build #60668 has finished for PR 13720 at commit 358ac0d.

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

SparkQA · 2016-06-17T04:04:17Z

Test build #60677 has finished for PR 13720 at commit 25510f5.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- public class JavaIsotonicRegressionExample

bomeng · 2016-06-20T22:14:20Z

@srowen please review. thanks!

srowen · 2016-06-21T05:50:12Z

 s"Created: ${new Date(createTime).toString}",
- s"Last Access: ${new Date(lastAccessTime).toString}",
+ "Last Access: " +
+ (if (lastAccessTime == -1) "UNKNOWN" else new Date(lastAccessTime).toString),


I don't feel strongly about it, but seems like elsewhere an empty string is used for no values. This seems slightly preferable.

Here is the code from Hive (it is using 0 as initial last access value):
File: MetaDataFormatUtils.java
private static String formatDate(long timeInSeconds) {
if (timeInSeconds != 0) {
Date date = new Date(timeInSeconds * 1000);
return date.toString();
}
return "UNKNOWN";
}

Yes, but the rest of this code doesn't use that logic. It returns "" in code around this area.

I prefer empty string here, cc @yhuai @liancheng what do you think?

Empty string looks fine to me.

Please follow this?

SparkQA · 2016-06-22T00:11:44Z

Test build #60968 has finished for PR 13720 at commit 90c3ff0.

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

bomeng · 2016-06-23T23:09:02Z

@cloud-fan Is this one worth to be fixed?

cloud-fan · 2016-06-24T01:34:35Z

 private def describeSchema(schema: Seq[CatalogColumn], buffer: ArrayBuffer[Row]): Unit = {
 schema.foreach { column =>
- append(buffer, column.name, column.dataType.toLowerCase, column.comment.orNull)
+ append(buffer, column.name, column.dataType.toLowerCase, column.comment.getOrElse(""))


this is a behaviour changing. The result is not only used to display, but also used as a table to be queried later. I'm not sure it worth. cc @yhuai

Yea. If it is null, let's keep it as null. Changing a null to an empty string actually destroys the information.

Agree, a column without any comment (null) is different from a column with a comment that is an empty string.

cloud-fan · 2016-06-24T01:40:15Z

For the test, currently we only have one desc table test in HiveDDLSuite, It will be good if we can have an individual test suite for it.

bomeng · 2016-06-24T03:47:15Z

ok, i will work on it based on comments. Thanks.

SparkQA · 2016-06-24T22:56:22Z

Test build #61196 has finished for PR 13720 at commit e93b72a.

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

SparkQA · 2016-06-27T20:02:15Z

Test build #61311 has finished for PR 13720 at commit a72800c.

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

bomeng · 2016-06-27T20:48:17Z

@cloud-fan please review again, thanks.

gatorsmile · 2017-05-23T20:30:34Z

@bomeng Cloud you resolve the conflicts?

gatorsmile · 2017-05-23T20:35:04Z

 }
 }

+ test("Describe Table") {


This test case does not verify the issue you fixed, I think

HyukjinKwon · 2017-06-02T14:22:11Z

ping @bomeng

# What changes were proposed in this pull request? This PR proposes to close stale PRs, mostly the same instances with apache#18017Closesapache#11459Closesapache#13833Closesapache#13720Closesapache#12506Closesapache#12456Closesapache#12252Closesapache#17689Closesapache#17791Closesapache#18163Closesapache#17640Closesapache#17926Closesapache#18163Closesapache#12506Closesapache#18044Closesapache#14036Closesapache#15831Closesapache#14461Closesapache#17638Closesapache#18222 Added: Closesapache#18045Closesapache#18061Closesapache#18010Closesapache#18041Closesapache#18124Closesapache#18130Closesapache#12217 Added: Closesapache#16291Closesapache#17480Closesapache#14995 Added: Closesapache#12835Closesapache#17141 ## How was this patch tested? N/A Author: hyukjinkwon <gurwls223@gmail.com> Closesapache#18223 from HyukjinKwon/close-stale-prs.

improve the disply of CatalogTable information
358ac0d

bomeng changed the title ~~[SPAKR-16004] [SQL] improve the disply of CatalogTable information~~[SPAKR-16004] [SQL] improve the display of CatalogTable informationJun 16, 2016

bomeng changed the title ~~[SPAKR-16004] [SQL] improve the display of CatalogTable information~~[SPARK-16004] [SQL] improve the display of CatalogTable informationJun 16, 2016

bomeng added 2 commits June 16, 2016 18:59

fix the tests failure
0fe835d

Merge remote-tracking branch 'upstream/master' intoSPARK-16004
25510f5

Merge remote-tracking branch 'upstream/master' intoSPARK-16004
26b9d79

srowen reviewed Jun 21, 2016
View reviewed changes

bomeng added 2 commits June 21, 2016 14:48

Merge remote-tracking branch 'upstream/master' intoSPARK-16004
8e7b271

revert user name
90c3ff0

cloud-fan reviewed Jun 24, 2016
View reviewed changes

bomeng added 3 commits June 24, 2016 13:20

Merge remote-tracking branch 'upstream/master' intoSPARK-16004
da8b77c

revert comment to use null
4c9e295

remove space
e93b72a

bomeng changed the title ~~[SPARK-16004] [SQL] improve the display of CatalogTable information~~[SPARK-16004] [SQL] Correctly display "Last Access Time" of CatalogTableJun 24, 2016

add test cases
a72800c

gatorsmile reviewed May 23, 2017
View reviewed changes

HyukjinKwon mentioned this pull request Jun 7, 2017
[INFRA] Close stale PRs #18223
Closed

asfgit closed this in b771fedJun 8, 2017

Uh oh!

Conversation

bomeng commented Jun 16, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Jun 17, 2016

Uh oh!

SparkQA commented Jun 17, 2016

Uh oh!

bomeng commented Jun 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bomengJun 21, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 22, 2016

Uh oh!

bomeng commented Jun 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Jun 24, 2016

Uh oh!

bomeng commented Jun 24, 2016

Uh oh!

SparkQA commented Jun 24, 2016

Uh oh!

SparkQA commented Jun 27, 2016

Uh oh!

bomeng commented Jun 27, 2016

Uh oh!

gatorsmile commented May 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon commented Jun 2, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

bomeng commented Jun 16, 2016•
edited
Loading

bomengJun 21, 2016•
edited
Loading