[SPARK-16004] [SQL] Correctly display "Last Access Time" of CatalogTable #13720
Conversation
bomeng
changed the title
bomeng
changed the title
|
Test build #60668 has finished for PR 13720 at commit
|
|
Test build #60677 has finished for PR 13720 at commit
|
|
@srowen please review. thanks! |
| s"Created: ${new Date(createTime).toString}", | ||
| s"Last Access: ${new Date(lastAccessTime).toString}", | ||
| "Last Access: " + | ||
| (if (lastAccessTime == -1) "UNKNOWN" else new Date(lastAccessTime).toString), |
There was a problem hiding this comment.
I don't feel strongly about it, but seems like elsewhere an empty string is used for no values. This seems slightly preferable.
There was a problem hiding this comment.
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";
}
There was a problem hiding this comment.
Yes, but the rest of this code doesn't use that logic. It returns "" in code around this area.
There was a problem hiding this comment.
I prefer empty string here, cc @yhuai @liancheng what do you think?
There was a problem hiding this comment.
Empty string looks fine to me.
|
Test build #60968 has finished for PR 13720 at commit
|
|
@cloud-fan Is this one worth to be fixed? |
| 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("")) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Yea. If it is null, let's keep it as null. Changing a null to an empty string actually destroys the information.
There was a problem hiding this comment.
Agree, a column without any comment (null) is different from a column with a comment that is an empty string.
|
For the test, currently we only have one |
|
ok, i will work on it based on comments. Thanks. |
bomeng
changed the title
|
Test build #61196 has finished for PR 13720 at commit
|
|
Test build #61311 has finished for PR 13720 at commit
|
|
@cloud-fan please review again, thanks. |
|
@bomeng Cloud you resolve the conflicts? |
| } | ||
| } | ||
|
|
||
| test("Describe Table") { |
There was a problem hiding this comment.
This test case does not verify the issue you fixed, I think
|
ping @bomeng |
8 participants
What changes were proposed in this pull request?
A few issues found when running "describe extended | formatted [tableName]" command:
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.