{"action":"created","comment":{"url":"https://api.github.com/repos/apache/spark/pulls/comments/88999529","pull_request_review_id":9553637,"id":88999529,"diff_hunk":"@@ -58,60 +61,127 @@ case class Statistics(\n   }\n }\n \n+\n /**\n- * Statistics for a column.\n+ * Statistics collected for a column.\n+ *\n+ * 1. Supported data types are defined in `ColumnStat.supportsType`.\n+ * 2. The JVM data type stored in min/max is the external data type (used in Row) for the\n+ * corresponding Catalyst data type. For example, for DateType we store java.sql.Date, and for\n+ * TimestampType we store java.sql.Timestamp.\n+ * 3. For integral types, they are all upcasted to longs, i.e. shorts are stored as longs.\n+ *\n+ * @param ndv number of distinct values\n+ * @param min minimum value\n+ * @param max maximum value\n+ * @param numNulls number of nulls\n+ * @param avgLen average length of the values. For fixed-length types, this should be a constant.\n+ * @param maxLen maximum length of the values. For fixed-length types, this should be a constant.\n  */\n-case class ColumnStat(statRow: InternalRow) {\n+// TODO: decide if we want to use bigint to represent ndv and numNulls.\n+case class ColumnStat(\n+    ndv: Long,\n+    min: Any,\n+    max: Any,\n+    numNulls: Long,\n+    avgLen: Long,\n+    maxLen: Long) {\n \n-  def forNumeric[T <: AtomicType](dataType: T): NumericColumnStat[T] = {\n-    NumericColumnStat(statRow, dataType)\n-  }\n-  def forString: StringColumnStat = StringColumnStat(statRow)\n-  def forBinary: BinaryColumnStat = BinaryColumnStat(statRow)\n-  def forBoolean: BooleanColumnStat = BooleanColumnStat(statRow)\n+  /**\n+   * Returns a map from string to string that can be used to serialize the column stats.\n+   * The key is the name of the field (e.g. \"ndv\" or \"min\"), and the value is the string\n+   * representation for the value. The deserialization side is defined in [[ColumnStat.fromMap]].\n+   *\n+   * As part of the protocol, the returned map always contains a key called \"version\".\n+   */\n+  def toMap: Map[String, String] = Map(\n+    \"version\" -> \"1\",\n+    \"ndv\" -> ndv.toString,\n+    \"min\" -> min.toString,\n+    \"max\" -> max.toString,\n+    \"numNulls\" -> numNulls.toString,\n+    \"avgLen\" -> avgLen.toString,\n+    \"maxLen\" -> maxLen.toString\n+  )\n+}\n+\n+\n+object ColumnStat extends Logging {\n \n-  override def toString: String = {\n-    // use Base64 for encoding\n-    Base64.encodeBase64String(statRow.asInstanceOf[UnsafeRow].getBytes)\n+  /** Returns true iff the we support gathering column statistics on column of the given type. */\n+  def supportsType(dataType: DataType): Boolean = dataType match {\n+    case _: NumericType | TimestampType | DateType | BooleanType => true\n+    case StringType | BinaryType => true\n+    case _ => false\n   }\n-}\n \n-object ColumnStat {\n-  def apply(numFields: Int, str: String): ColumnStat = {\n-    // use Base64 for decoding\n-    val bytes = Base64.decodeBase64(str)\n-    val unsafeRow = new UnsafeRow(numFields)\n-    unsafeRow.pointTo(bytes, bytes.length)\n-    ColumnStat(unsafeRow)\n+  /**\n+   * Creates a [[ColumnStat]] object from the given map. This is used to deserialize column stats\n+   * from some external storage. The serialization side is defined in [[ColumnStat.toMap]].\n+   */\n+  def fromMap(dataType: DataType, map: Map[String, String]): Option[ColumnStat] = {\n+    val str2val: (String => Any) = dataType match {\n+      case _: IntegralType => _.toLong\n+      case _: DecimalType => Decimal(_)\n+      case DoubleType | FloatType => _.toDouble\n+      case BooleanType => _.toBoolean\n+      case _ => identity\n+    }\n+\n+    try {\n+      Some(ColumnStat(\n+        ndv = map(\"ndv\").toLong,\n+        min = str2val(map.get(\"min\").orNull),\n+        max = str2val(map.get(\"max\").orNull),\n+        numNulls = map(\"numNulls\").toLong,\n+        avgLen = map.getOrElse(\"avgLen\", \"1\").toLong,\n+        maxLen = map.getOrElse(\"maxLen\", \"1\").toLong\n+      ))\n+    } catch {\n+      case NonFatal(e) =>\n+        logWarning(\"Failed to parse column statistics\", e)\n+        None\n+    }\n   }\n-}\n \n-case class NumericColumnStat[T <: AtomicType](statRow: InternalRow, dataType: T) {\n-  // The indices here must be consistent with `ColumnStatStruct.numericColumnStat`.\n-  val numNulls: Long = statRow.getLong(0)\n-  val max: T#InternalType = statRow.get(1, dataType).asInstanceOf[T#InternalType]\n-  val min: T#InternalType = statRow.get(2, dataType).asInstanceOf[T#InternalType]\n-  val ndv: Long = statRow.getLong(3)\n-}\n+  /**\n+   * ndv: Long, min: T, max: T, numNulls: Long, avgLen: Long, maxLen: Long\n+   */\n+  def statExprs(col: Attribute, relativeSD: Double): CreateNamedStruct = {\n+    def struct(exprs: Expression*): CreateNamedStruct = CreateStruct(exprs.map { expr =>\n+      expr.transformUp { case af: AggregateFunction => af.toAggregateExpression() }\n+    })\n+    val zero = Literal(0, LongType)\n+    val one = Literal(1, LongType)\n \n-case class StringColumnStat(statRow: InternalRow) {\n-  // The indices here must be consistent with `ColumnStatStruct.stringColumnStat`.\n-  val numNulls: Long = statRow.getLong(0)\n-  val avgColLen: Double = statRow.getDouble(1)\n-  val maxColLen: Long = statRow.getInt(2)\n-  val ndv: Long = statRow.getLong(3)\n-}\n+    // the approximate ndv should never be larger than the number of rows\n+    val ndv = Least(Seq(HyperLogLogPlusPlus(col, relativeSD), Count(one)))","path":"sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/Statistics.scala","position":151,"original_position":151,"commit_id":"662195d727e7bc3ffdc261c97ce01ec5e9c0193c","original_commit_id":"662195d727e7bc3ffdc261c97ce01ec5e9c0193c","user":{"login":"rxin","id":323388,"avatar_url":"https://avatars.githubusercontent.com/u/323388?v=3","gravatar_id":"","url":"https://api.github.com/users/rxin","html_url":"https://github.com/rxin","followers_url":"https://api.github.com/users/rxin/followers","following_url":"https://api.github.com/users/rxin/following{/other_user}","gists_url":"https://api.github.com/users/rxin/gists{/gist_id}","starred_url":"https://api.github.com/users/rxin/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/rxin/subscriptions","organizations_url":"https://api.github.com/users/rxin/orgs","repos_url":"https://api.github.com/users/rxin/repos","events_url":"https://api.github.com/users/rxin/events{/privacy}","received_events_url":"https://api.github.com/users/rxin/received_events","type":"User","site_admin":false},"body":"good point fixed","created_at":"2016-11-21T22:05:41Z","updated_at":"2016-11-21T22:05:41Z","html_url":"https://github.com/apache/spark/pull/15959#discussion_r88999529","pull_request_url":"https://api.github.com/repos/apache/spark/pulls/15959","_links":{"self":{"href":"https://api.github.com/repos/apache/spark/pulls/comments/88999529"},"html":{"href":"https://github.com/apache/spark/pull/15959#discussion_r88999529"},"pull_request":{"href":"https://api.github.com/repos/apache/spark/pulls/15959"}}},"pull_request":{"url":"https://api.github.com/repos/apache/spark/pulls/15959","id":94567691,"html_url":"https://github.com/apache/spark/pull/15959","diff_url":"https://github.com/apache/spark/pull/15959.diff","patch_url":"https://github.com/apache/spark/pull/15959.patch","issue_url":"https://api.github.com/repos/apache/spark/issues/15959","number":15959,"state":"open","locked":false,"title":"[SPARK-18522][SQL] Explicit contract for column stats serialization","user":{"login":"rxin","id":323388,"avatar_url":"https://avatars.githubusercontent.com/u/323388?v=3","gravatar_id":"","url":"https://api.github.com/users/rxin","html_url":"https://github.com/rxin","followers_url":"https://api.github.com/users/rxin/followers","following_url":"https://api.github.com/users/rxin/following{/other_user}","gists_url":"https://api.github.com/users/rxin/gists{/gist_id}","starred_url":"https://api.github.com/users/rxin/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/rxin/subscriptions","organizations_url":"https://api.github.com/users/rxin/orgs","repos_url":"https://api.github.com/users/rxin/repos","events_url":"https://api.github.com/users/rxin/events{/privacy}","received_events_url":"https://api.github.com/users/rxin/received_events","type":"User","site_admin":false},"body":"## What changes were proposed in this pull request?\r\nThe current implementation of column stats uses the base64 encoding of the internal UnsafeRow format to persist statistics (in table properties in Hive metastore). This is an internal format that is not stable across different versions of Spark and should NOT be used for persistence. In addition, it would be better if statistics stored in the catalog is human readable.\r\n\r\nThis pull request introduces the following changes:\r\n\r\n1. Created a single ColumnStat class to for all data types. All data types track the same set of statistics.\r\n2. Updated the implementation for stats collection to get rid of the dependency on internal data structures (e.g. InternalRow, or storing DateType as an int32). For example, previously dates were stored as a single integer, but are now stored as java.sql.Date. When we implement the next steps of CBO, we can add code to convert those back into internal types again.\r\n3. Documented clearly what JVM data types are being used to store what data.\r\n4. Defined a simple Map[String, String] interface for serializing and deserializing column stats into/from the catalog.\r\n5. Rearranged the method/function structure so it is more clear what the supported data types are, and also moved how stats are generated into ColumnStat class so they are easy to find.\r\n\r\n## How was this patch tested?\r\nTBD - I haven't updated any of the test cases yet and they would fail compilation.","created_at":"2016-11-21T07:49:02Z","updated_at":"2016-11-21T22:05:41Z","closed_at":null,"merged_at":null,"merge_commit_sha":"d7059cb3c5f71c17e3cefba3cfd057514e55bad0","assignee":null,"assignees":[],"milestone":null,"commits_url":"https://api.github.com/repos/apache/spark/pulls/15959/commits","review_comments_url":"https://api.github.com/repos/apache/spark/pulls/15959/comments","review_comment_url":"https://api.github.com/repos/apache/spark/pulls/comments{/number}","comments_url":"https://api.github.com/repos/apache/spark/issues/15959/comments","statuses_url":"https://api.github.com/repos/apache/spark/statuses/662195d727e7bc3ffdc261c97ce01ec5e9c0193c","head":{"label":"rxin:SPARK-18522","ref":"SPARK-18522","sha":"662195d727e7bc3ffdc261c97ce01ec5e9c0193c","user":{"login":"rxin","id":323388,"avatar_url":"https://avatars.githubusercontent.com/u/323388?v=3","gravatar_id":"","url":"https://api.github.com/users/rxin","html_url":"https://github.com/rxin","followers_url":"https://api.github.com/users/rxin/followers","following_url":"https://api.github.com/users/rxin/following{/other_user}","gists_url":"https://api.github.com/users/rxin/gists{/gist_id}","starred_url":"https://api.github.com/users/rxin/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/rxin/subscriptions","organizations_url":"https://api.github.com/users/rxin/orgs","repos_url":"https://api.github.com/users/rxin/repos","events_url":"https://api.github.com/users/rxin/events{/privacy}","received_events_url":"https://api.github.com/users/rxin/received_events","type":"User","site_admin":false},"repo":{"id":17192644,"name":"spark","full_name":"rxin/spark","owner":{"login":"rxin","id":323388,"avatar_url":"https://avatars.githubusercontent.com/u/323388?v=3","gravatar_id":"","url":"https://api.github.com/users/rxin","html_url":"https://github.com/rxin","followers_url":"https://api.github.com/users/rxin/followers","following_url":"https://api.github.com/users/rxin/following{/other_user}","gists_url":"https://api.github.com/users/rxin/gists{/gist_id}","starred_url":"https://api.github.com/users/rxin/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/rxin/subscriptions","organizations_url":"https://api.github.com/users/rxin/orgs","repos_url":"https://api.github.com/users/rxin/repos","events_url":"https://api.github.com/users/rxin/events{/privacy}","received_events_url":"https://api.github.com/users/rxin/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/rxin/spark","description":"Mirror of Apache Spark","fork":true,"url":"https://api.github.com/repos/rxin/spark","forks_url":"https://api.github.com/repos/rxin/spark/forks","keys_url":"https://api.github.com/repos/rxin/spark/keys{/key_id}","collaborators_url":"https://api.github.com/repos/rxin/spark/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/rxin/spark/teams","hooks_url":"https://api.github.com/repos/rxin/spark/hooks","issue_events_url":"https://api.github.com/repos/rxin/spark/issues/events{/number}","events_url":"https://api.github.com/repos/rxin/spark/events","assignees_url":"https://api.github.com/repos/rxin/spark/assignees{/user}","branches_url":"https://api.github.com/repos/rxin/spark/branches{/branch}","tags_url":"https://api.github.com/repos/rxin/spark/tags","blobs_url":"https://api.github.com/repos/rxin/spark/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/rxin/spark/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/rxin/spark/git/refs{/sha}","trees_url":"https://api.github.com/repos/rxin/spark/git/trees{/sha}","statuses_url":"https://api.github.com/repos/rxin/spark/statuses/{sha}","languages_url":"https://api.github.com/repos/rxin/spark/languages","stargazers_url":"https://api.github.com/repos/rxin/spark/stargazers","contributors_url":"https://api.github.com/repos/rxin/spark/contributors","subscribers_url":"https://api.github.com/repos/rxin/spark/subscribers","subscription_url":"https://api.github.com/repos/rxin/spark/subscription","commits_url":"https://api.github.com/repos/rxin/spark/commits{/sha}","git_commits_url":"https://api.github.com/repos/rxin/spark/git/commits{/sha}","comments_url":"https://api.github.com/repos/rxin/spark/comments{/number}","issue_comment_url":"https://api.github.com/repos/rxin/spark/issues/comments{/number}","contents_url":"https://api.github.com/repos/rxin/spark/contents/{+path}","compare_url":"https://api.github.com/repos/rxin/spark/compare/{base}...{head}","merges_url":"https://api.github.com/repos/rxin/spark/merges","archive_url":"https://api.github.com/repos/rxin/spark/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/rxin/spark/downloads","issues_url":"https://api.github.com/repos/rxin/spark/issues{/number}","pulls_url":"https://api.github.com/repos/rxin/spark/pulls{/number}","milestones_url":"https://api.github.com/repos/rxin/spark/milestones{/number}","notifications_url":"https://api.github.com/repos/rxin/spark/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/rxin/spark/labels{/name}","releases_url":"https://api.github.com/repos/rxin/spark/releases{/id}","deployments_url":"https://api.github.com/repos/rxin/spark/deployments","created_at":"2014-02-25T23:55:52Z","updated_at":"2016-09-12T14:40:11Z","pushed_at":"2016-11-21T07:51:09Z","git_url":"git://github.com/rxin/spark.git","ssh_url":"git@github.com:rxin/spark.git","clone_url":"https://github.com/rxin/spark.git","svn_url":"https://github.com/rxin/spark","homepage":null,"size":258007,"stargazers_count":6,"watchers_count":6,"language":"Scala","has_issues":false,"has_downloads":true,"has_wiki":false,"has_pages":false,"forks_count":12,"mirror_url":null,"open_issues_count":0,"forks":12,"open_issues":0,"watchers":6,"default_branch":"master"}},"base":{"label":"apache:master","ref":"master","sha":"658547974915ebcaae83e13e4c3bdf68d5426fda","user":{"login":"apache","id":47359,"avatar_url":"https://avatars.githubusercontent.com/u/47359?v=3","gravatar_id":"","url":"https://api.github.com/users/apache","html_url":"https://github.com/apache","followers_url":"https://api.github.com/users/apache/followers","following_url":"https://api.github.com/users/apache/following{/other_user}","gists_url":"https://api.github.com/users/apache/gists{/gist_id}","starred_url":"https://api.github.com/users/apache/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/apache/subscriptions","organizations_url":"https://api.github.com/users/apache/orgs","repos_url":"https://api.github.com/users/apache/repos","events_url":"https://api.github.com/users/apache/events{/privacy}","received_events_url":"https://api.github.com/users/apache/received_events","type":"Organization","site_admin":false},"repo":{"id":17165658,"name":"spark","full_name":"apache/spark","owner":{"login":"apache","id":47359,"avatar_url":"https://avatars.githubusercontent.com/u/47359?v=3","gravatar_id":"","url":"https://api.github.com/users/apache","html_url":"https://github.com/apache","followers_url":"https://api.github.com/users/apache/followers","following_url":"https://api.github.com/users/apache/following{/other_user}","gists_url":"https://api.github.com/users/apache/gists{/gist_id}","starred_url":"https://api.github.com/users/apache/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/apache/subscriptions","organizations_url":"https://api.github.com/users/apache/orgs","repos_url":"https://api.github.com/users/apache/repos","events_url":"https://api.github.com/users/apache/events{/privacy}","received_events_url":"https://api.github.com/users/apache/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/apache/spark","description":"Mirror of Apache Spark","fork":false,"url":"https://api.github.com/repos/apache/spark","forks_url":"https://api.github.com/repos/apache/spark/forks","keys_url":"https://api.github.com/repos/apache/spark/keys{/key_id}","collaborators_url":"https://api.github.com/repos/apache/spark/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/apache/spark/teams","hooks_url":"https://api.github.com/repos/apache/spark/hooks","issue_events_url":"https://api.github.com/repos/apache/spark/issues/events{/number}","events_url":"https://api.github.com/repos/apache/spark/events","assignees_url":"https://api.github.com/repos/apache/spark/assignees{/user}","branches_url":"https://api.github.com/repos/apache/spark/branches{/branch}","tags_url":"https://api.github.com/repos/apache/spark/tags","blobs_url":"https://api.github.com/repos/apache/spark/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/apache/spark/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/apache/spark/git/refs{/sha}","trees_url":"https://api.github.com/repos/apache/spark/git/trees{/sha}","statuses_url":"https://api.github.com/repos/apache/spark/statuses/{sha}","languages_url":"https://api.github.com/repos/apache/spark/languages","stargazers_url":"https://api.github.com/repos/apache/spark/stargazers","contributors_url":"https://api.github.com/repos/apache/spark/contributors","subscribers_url":"https://api.github.com/repos/apache/spark/subscribers","subscription_url":"https://api.github.com/repos/apache/spark/subscription","commits_url":"https://api.github.com/repos/apache/spark/commits{/sha}","git_commits_url":"https://api.github.com/repos/apache/spark/git/commits{/sha}","comments_url":"https://api.github.com/repos/apache/spark/comments{/number}","issue_comment_url":"https://api.github.com/repos/apache/spark/issues/comments{/number}","contents_url":"https://api.github.com/repos/apache/spark/contents/{+path}","compare_url":"https://api.github.com/repos/apache/spark/compare/{base}...{head}","merges_url":"https://api.github.com/repos/apache/spark/merges","archive_url":"https://api.github.com/repos/apache/spark/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/apache/spark/downloads","issues_url":"https://api.github.com/repos/apache/spark/issues{/number}","pulls_url":"https://api.github.com/repos/apache/spark/pulls{/number}","milestones_url":"https://api.github.com/repos/apache/spark/milestones{/number}","notifications_url":"https://api.github.com/repos/apache/spark/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/apache/spark/labels{/name}","releases_url":"https://api.github.com/repos/apache/spark/releases{/id}","deployments_url":"https://api.github.com/repos/apache/spark/deployments","created_at":"2014-02-25T08:00:08Z","updated_at":"2016-11-21T15:53:44Z","pushed_at":"2016-11-21T21:40:43Z","git_url":"git://github.com/apache/spark.git","ssh_url":"git@github.com:apache/spark.git","clone_url":"https://github.com/apache/spark.git","svn_url":"https://github.com/apache/spark","homepage":null,"size":612116,"stargazers_count":10812,"watchers_count":10812,"language":"Scala","has_issues":false,"has_downloads":true,"has_wiki":false,"has_pages":false,"forks_count":10080,"mirror_url":"git://git.apache.org/spark.git","open_issues_count":451,"forks":10080,"open_issues":451,"watchers":10812,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/apache/spark/pulls/15959"},"html":{"href":"https://github.com/apache/spark/pull/15959"},"issue":{"href":"https://api.github.com/repos/apache/spark/issues/15959"},"comments":{"href":"https://api.github.com/repos/apache/spark/issues/15959/comments"},"review_comments":{"href":"https://api.github.com/repos/apache/spark/pulls/15959/comments"},"review_comment":{"href":"https://api.github.com/repos/apache/spark/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/apache/spark/pulls/15959/commits"},"statuses":{"href":"https://api.github.com/repos/apache/spark/statuses/662195d727e7bc3ffdc261c97ce01ec5e9c0193c"}}}}
{"actor":{"display_login":"rxin"}}