Skip to content

[SPARK-16004] [SQL] Correctly display "Last Access Time" of CatalogTable #13720

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

Closed
bomeng wants to merge 10 commits into from
Closed

[SPARK-16004] [SQL] Correctly display "Last Access Time" of CatalogTable #13720

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
358ac0d
improve the disply of CatalogTable information
Jun 16, 2016
0fe835d
fix the tests failure
Jun 17, 2016
25510f5
Merge remote-tracking branch 'upstream/master' into SPARK-16004
Jun 17, 2016
26b9d79
Merge remote-tracking branch 'upstream/master' into SPARK-16004
Jun 20, 2016
8e7b271
Merge remote-tracking branch 'upstream/master' into SPARK-16004
Jun 21, 2016
90c3ff0
revert user name
Jun 21, 2016
da8b77c
Merge remote-tracking branch 'upstream/master' into SPARK-16004
Jun 24, 2016
4c9e295
revert comment to use null
Jun 24, 2016
e93b72a
remove space
Jun 24, 2016
a72800c
add test cases
Jun 27, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/interface.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ case class CatalogTable(
sortColumnNames: Seq[String] = Seq.empty,
bucketColumnNames: Seq[String] = Seq.empty,
numBuckets: Int = -1,
owner: String = "",
owner: String = System.getProperty("user.name"),
Copy link
Member

@srowen srowen Jun 21, 2016

Choose a reason for hiding this comment

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

Is this definitely the owner? it doesn't seem like it would generally be the user happening to run the application.

Copy link
Contributor Author

@bomeng bomeng Jun 21, 2016

Choose a reason for hiding this comment

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

Let me check what HIVE does tomorrow and get it back to you.

Copy link
Contributor Author

@bomeng bomeng Jun 21, 2016

Choose a reason for hiding this comment

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

User name will be complicated. It is set from current SessionState authenticator. I do not believe we have reached that yet. I will revert this part.

createTime: Long = System.currentTimeMillis,
lastAccessTime: Long = -1,
properties: Map[String, String] = Map.empty,
Expand Down Expand Up @@ -180,7 +180,8 @@ case class CatalogTable(
Seq(s"Table: ${identifier.quotedString}",
if (owner.nonEmpty) s"Owner: $owner" else "",
s"Created: ${new Date(createTime).toString}",
s"Last Access: ${new Date(lastAccessTime).toString}",
"Last Access: " +
(if (lastAccessTime == -1) "UNKNOWN" else new Date(lastAccessTime).toString),
Copy link
Member

@srowen srowen Jun 21, 2016

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@bomeng bomeng Jun 21, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

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";
}

Copy link
Member

@srowen srowen Jun 25, 2016

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@cloud-fan cloud-fan Jun 28, 2016

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@liancheng liancheng Jun 28, 2016

Choose a reason for hiding this comment

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

Empty string looks fine to me.

Copy link
Member

@gatorsmile gatorsmile May 23, 2017

Choose a reason for hiding this comment

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

Please follow this?

s"Type: ${tableType.name}",
if (schema.nonEmpty) s"Schema: ${schema.mkString("[", ", ", "]")}" else "",
if (partitionColumnNames.nonEmpty) s"Partition Columns: $partitionColumns" else "",
Expand Down
5 changes: 3 additions & 2 deletions sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -468,7 +468,8 @@ case class DescribeTableCommand(table: TableIdentifier, isExtended: Boolean, isF
append(buffer, "Database:", table.database, "")
append(buffer, "Owner:", table.owner, "")
append(buffer, "Create Time:", new Date(table.createTime).toString, "")
append(buffer, "Last Access Time:", new Date(table.lastAccessTime).toString, "")
append(buffer, "Last Access Time:",
if (table.lastAccessTime == -1) "UNKNOWN" else new Date(table.lastAccessTime).toString, "")
append(buffer, "Location:", table.storage.locationUri.getOrElse(""), "")
append(buffer, "Table Type:", table.tableType.name, "")

Expand Down Expand Up @@ -522,7 +523,7 @@ case class DescribeTableCommand(table: TableIdentifier, isExtended: Boolean, isF

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(""))
Copy link
Contributor

@cloud-fan cloud-fan Jun 24, 2016

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@yhuai yhuai Jun 24, 2016

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@liancheng liancheng Jun 28, 2016

Choose a reason for hiding this comment

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

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

}
}

Expand Down
8 changes: 4 additions & 4 deletions sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveOperatorQueryableSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,13 +41,13 @@ class HiveOperatorQueryableSuite extends QueryTest with TestHiveSingleton {
checkAnswer(
sql("select * from mydesc"),
Seq(
Row("key", "int", null),
Row("value", "string", null)))
Row("key", "int", ""),
Row("value", "string", "")))

checkAnswer(
sql("select col_name, data_type, comment from mydesc"),
Seq(
Row("key", "int", null),
Row("value", "string", null)))
Row("key", "int", ""),
Row("value", "string", "")))
}
}
16 changes: 8 additions & 8 deletions sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveQuerySuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -798,12 +798,12 @@ class HiveQuerySuite extends HiveComparisonTest with BeforeAndAfter {
// Describe a table
assertResult(
Array(
Row("key", "int", null),
Row("value", "string", null),
Row("dt", "string", null),
Row("key", "int", ""),
Row("value", "string", ""),
Row("dt", "string", ""),
Row("# Partition Information", "", ""),
Row("# col_name", "data_type", "comment"),
Row("dt", "string", null))
Row("dt", "string", ""))
) {
sql("DESCRIBE test_describe_commands1")
.select('col_name, 'data_type, 'comment)
Expand All @@ -813,12 +813,12 @@ class HiveQuerySuite extends HiveComparisonTest with BeforeAndAfter {
// Describe a table with a fully qualified table name
assertResult(
Array(
Row("key", "int", null),
Row("value", "string", null),
Row("dt", "string", null),
Row("key", "int", ""),
Row("value", "string", ""),
Row("dt", "string", ""),
Row("# Partition Information", "", ""),
Row("# col_name", "data_type", "comment"),
Row("dt", "string", null))
Row("dt", "string", ""))
) {
sql("DESCRIBE default.test_describe_commands1")
.select('col_name, 'data_type, 'comment)
Expand Down