{"action":"created","comment":{"url":"https://api.github.com/repos/apache/spark/pulls/comments/89030586","pull_request_review_id":9583141,"id":89030586,"diff_hunk":"@@ -84,30 +84,95 @@ case class DataSource(\n   private val caseInsensitiveOptions = new CaseInsensitiveMap(options)\n \n   /**\n-   * Infer the schema of the given FileFormat, returns a pair of schema and partition column names.\n+   * Get the schema of the given FileFormat, if provided by `userSpecifiedSchema`, or try to infer\n+   * it. In the read path, only Hive managed tables provide the partition columns properly when\n+   * initializing this class. All other file based data sources will try to infer the partitioning,\n+   * and then cast the inferred types to user specified dataTypes if the partition columns exist\n+   * inside `userSpecifiedSchema`, otherwise we can hit data corruption bugs like SPARK-18510.\n+   * This method will try to skip file scanning whether `userSpecifiedSchema` and\n+   * `partitionColumns` are provided. Here are some code paths that use this method:\n+   *   1. `spark.read` (no schema): Most amount of work. Infer both schema and partitioning columns\n+   *   2. `spark.read.schema(userSpecifiedSchema)`: Parse partitioning columns, cast them to the\n+   *     dataTypes provided in `userSpecifiedSchema` if they exist or fallback to inferred\n+   *     dataType if they don't.\n+   *   3. `spark.readStream.schema(userSpecifiedSchema)`: For streaming use cases, users have to\n+   *     provide the schema. Here, we also perform partition inference like 2, and try to use\n+   *     dataTypes in `userSpecifiedSchema`. All subsequent triggers for this stream will re-use\n+   *     this information, therefore calls to this method should be very cheap, i.e. there won't\n+   *     be any further inference in any triggers.\n+   *   4. `df.saveAsTable(tableThatExisted)`: In this case, we call this method to resolve the\n+   *     existing table's partitioning scheme. This is achieved by not providing\n+   *     `userSpecifiedSchema`. For this case, we add the boolean `justPartitioning` for an early\n+   *     exit, if we don't care about the schema of the original table.\n+   *\n+   * Returns a pair of the data schema (excluding partition columns) and the schema of the partition\n+   * columns.\n+   *\n+   * @param justPartitioning Whether to exit early and provide just the schema partitioning. The\n+   *                         data schema is incorrect in this case and should not be used.\n    */\n-  private def inferFileFormatSchema(format: FileFormat): (StructType, Seq[String]) = {\n-    userSpecifiedSchema.map(_ -> partitionColumns).orElse {\n-      val allPaths = caseInsensitiveOptions.get(\"path\")\n+  private def getOrInferFileFormatSchema(\n+      format: FileFormat,\n+      justPartitioning: Boolean = false): (StructType, StructType) = {\n+    // the operations below are expensive therefore try not to do them if we don't need to\n+    lazy val tempFileCatalog = {\n+      val allPaths = caseInsensitiveOptions.get(\"path\") ++ paths\n       val globbedPaths = allPaths.toSeq.flatMap { path =>\n         val hdfsPath = new Path(path)\n         val fs = hdfsPath.getFileSystem(sparkSession.sessionState.newHadoopConf())\n         val qualified = hdfsPath.makeQualified(fs.getUri, fs.getWorkingDirectory)\n         SparkHadoopUtil.get.globPathIfNecessary(qualified)\n       }.toArray\n-      val fileCatalog = new InMemoryFileIndex(sparkSession, globbedPaths, options, None)\n-      val partitionSchema = fileCatalog.partitionSpec().partitionColumns\n-      val inferred = format.inferSchema(\n+      new InMemoryFileIndex(sparkSession, globbedPaths, options, None)\n+    }\n+    val partitionSchema = if (partitionColumns.isEmpty && catalogTable.isEmpty) {\n+      // Try to infer partitioning, because no DataSource in the read path provides the partitioning\n+      // columns properly unless it is a Hive DataSource\n+      val resolved = tempFileCatalog.partitionSchema.map { partitionField =>\n+        val equality = sparkSession.sessionState.conf.resolver\n+        // SPARK-18510: try to get schema from userSpecifiedSchema, otherwise fallback to inferred\n+        userSpecifiedSchema.flatMap(_.find(f => equality(f.name, partitionField.name))).getOrElse(\n+          partitionField)\n+      }\n+      StructType(resolved)\n+    } else {\n+      // in streaming mode, we have already inferred and registered partition columns, we will\n+      // never have to re-use this\n+      lazy val inferredPartitions = tempFileCatalog.partitionSchema","path":"sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSource.scala","position":null,"original_position":65,"commit_id":"97003e24ea377ae0d844e9f90f020b2111339661","original_commit_id":"330e148114f26484aba5e9cb16aac2f286cd05ba","user":{"login":"brkyvz","id":5243515,"avatar_url":"https://avatars.githubusercontent.com/u/5243515?v=3","gravatar_id":"","url":"https://api.github.com/users/brkyvz","html_url":"https://github.com/brkyvz","followers_url":"https://api.github.com/users/brkyvz/followers","following_url":"https://api.github.com/users/brkyvz/following{/other_user}","gists_url":"https://api.github.com/users/brkyvz/gists{/gist_id}","starred_url":"https://api.github.com/users/brkyvz/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/brkyvz/subscriptions","organizations_url":"https://api.github.com/users/brkyvz/orgs","repos_url":"https://api.github.com/users/brkyvz/repos","events_url":"https://api.github.com/users/brkyvz/events{/privacy}","received_events_url":"https://api.github.com/users/brkyvz/received_events","type":"User","site_admin":false},"body":"Since everything is already defined in `userSpecifiedSchema` once we create the `FileStreamSource`, we will never have to materialize this variable, because we will not use it.","created_at":"2016-11-22T02:02:02Z","updated_at":"2016-11-22T02:02:02Z","html_url":"https://github.com/apache/spark/pull/15951#discussion_r89030586","pull_request_url":"https://api.github.com/repos/apache/spark/pulls/15951","_links":{"self":{"href":"https://api.github.com/repos/apache/spark/pulls/comments/89030586"},"html":{"href":"https://github.com/apache/spark/pull/15951#discussion_r89030586"},"pull_request":{"href":"https://api.github.com/repos/apache/spark/pulls/15951"}}},"pull_request":{"url":"https://api.github.com/repos/apache/spark/pulls/15951","id":94535712,"html_url":"https://github.com/apache/spark/pull/15951","diff_url":"https://github.com/apache/spark/pull/15951.diff","patch_url":"https://github.com/apache/spark/pull/15951.patch","issue_url":"https://api.github.com/repos/apache/spark/issues/15951","number":15951,"state":"open","locked":false,"title":"[SPARK-18510] Fix data corruption from inferred partition column dataTypes","user":{"login":"brkyvz","id":5243515,"avatar_url":"https://avatars.githubusercontent.com/u/5243515?v=3","gravatar_id":"","url":"https://api.github.com/users/brkyvz","html_url":"https://github.com/brkyvz","followers_url":"https://api.github.com/users/brkyvz/followers","following_url":"https://api.github.com/users/brkyvz/following{/other_user}","gists_url":"https://api.github.com/users/brkyvz/gists{/gist_id}","starred_url":"https://api.github.com/users/brkyvz/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/brkyvz/subscriptions","organizations_url":"https://api.github.com/users/brkyvz/orgs","repos_url":"https://api.github.com/users/brkyvz/repos","events_url":"https://api.github.com/users/brkyvz/events{/privacy}","received_events_url":"https://api.github.com/users/brkyvz/received_events","type":"User","site_admin":false},"body":"## What changes were proposed in this pull request?\r\n\r\n### The Issue\r\n\r\nIf I specify my schema when doing\r\n```scala\r\nspark.read\r\n  .schema(someSchemaWherePartitionColumnsAreStrings)\r\n```\r\nbut if the partition inference can infer it as IntegerType or I assume LongType or DoubleType (basically fixed size types), then once UnsafeRows are generated, your data will be corrupted.\r\n\r\n### Proposed solution\r\n\r\nThe partition handling code path is kind of a mess. In my fix I'm probably adding to the mess, but at least trying to standardize the code path.\r\n\r\nThe real issue is that a user that uses the `spark.read` code path can never clearly specify what the partition columns are. If you try to specify the fields in `schema`, we practically ignore what the user provides, and fall back to our inferred data types. What happens in the end is data corruption.\r\n\r\nMy solution tries to fix this by always trying to infer partition columns the first time you specify the table. Once we find what the partition columns are, we try to find them in the user specified schema and use the dataType provided there, or fall back to the smallest common data type.\r\n\r\nWe will ALWAYS append partition columns to the user's schema, even if they didn't ask for it. We will only use the data type they provided if they specified it. While this is confusing, this has been the behavior since Spark 1.6, and I didn't want to change this behavior in the QA period of Spark 2.1. We may revisit this decision later.\r\n\r\nA side effect of this PR is that we won't need https://github.com/apache/spark/pull/15942 if this PR goes in.\r\n\r\n## How was this patch tested?\r\n\r\nRegression tests","created_at":"2016-11-20T22:35:11Z","updated_at":"2016-11-22T02:02:02Z","closed_at":null,"merged_at":null,"merge_commit_sha":"72ee55193884fb533450f9f6b75e7019812c87ad","assignee":null,"assignees":[],"milestone":null,"commits_url":"https://api.github.com/repos/apache/spark/pulls/15951/commits","review_comments_url":"https://api.github.com/repos/apache/spark/pulls/15951/comments","review_comment_url":"https://api.github.com/repos/apache/spark/pulls/comments{/number}","comments_url":"https://api.github.com/repos/apache/spark/issues/15951/comments","statuses_url":"https://api.github.com/repos/apache/spark/statuses/97003e24ea377ae0d844e9f90f020b2111339661","head":{"label":"brkyvz:partition-corruption","ref":"partition-corruption","sha":"97003e24ea377ae0d844e9f90f020b2111339661","user":{"login":"brkyvz","id":5243515,"avatar_url":"https://avatars.githubusercontent.com/u/5243515?v=3","gravatar_id":"","url":"https://api.github.com/users/brkyvz","html_url":"https://github.com/brkyvz","followers_url":"https://api.github.com/users/brkyvz/followers","following_url":"https://api.github.com/users/brkyvz/following{/other_user}","gists_url":"https://api.github.com/users/brkyvz/gists{/gist_id}","starred_url":"https://api.github.com/users/brkyvz/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/brkyvz/subscriptions","organizations_url":"https://api.github.com/users/brkyvz/orgs","repos_url":"https://api.github.com/users/brkyvz/repos","events_url":"https://api.github.com/users/brkyvz/events{/privacy}","received_events_url":"https://api.github.com/users/brkyvz/received_events","type":"User","site_admin":false},"repo":{"id":21919521,"name":"spark","full_name":"brkyvz/spark","owner":{"login":"brkyvz","id":5243515,"avatar_url":"https://avatars.githubusercontent.com/u/5243515?v=3","gravatar_id":"","url":"https://api.github.com/users/brkyvz","html_url":"https://github.com/brkyvz","followers_url":"https://api.github.com/users/brkyvz/followers","following_url":"https://api.github.com/users/brkyvz/following{/other_user}","gists_url":"https://api.github.com/users/brkyvz/gists{/gist_id}","starred_url":"https://api.github.com/users/brkyvz/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/brkyvz/subscriptions","organizations_url":"https://api.github.com/users/brkyvz/orgs","repos_url":"https://api.github.com/users/brkyvz/repos","events_url":"https://api.github.com/users/brkyvz/events{/privacy}","received_events_url":"https://api.github.com/users/brkyvz/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/brkyvz/spark","description":"Mirror of Apache Spark","fork":true,"url":"https://api.github.com/repos/brkyvz/spark","forks_url":"https://api.github.com/repos/brkyvz/spark/forks","keys_url":"https://api.github.com/repos/brkyvz/spark/keys{/key_id}","collaborators_url":"https://api.github.com/repos/brkyvz/spark/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/brkyvz/spark/teams","hooks_url":"https://api.github.com/repos/brkyvz/spark/hooks","issue_events_url":"https://api.github.com/repos/brkyvz/spark/issues/events{/number}","events_url":"https://api.github.com/repos/brkyvz/spark/events","assignees_url":"https://api.github.com/repos/brkyvz/spark/assignees{/user}","branches_url":"https://api.github.com/repos/brkyvz/spark/branches{/branch}","tags_url":"https://api.github.com/repos/brkyvz/spark/tags","blobs_url":"https://api.github.com/repos/brkyvz/spark/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/brkyvz/spark/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/brkyvz/spark/git/refs{/sha}","trees_url":"https://api.github.com/repos/brkyvz/spark/git/trees{/sha}","statuses_url":"https://api.github.com/repos/brkyvz/spark/statuses/{sha}","languages_url":"https://api.github.com/repos/brkyvz/spark/languages","stargazers_url":"https://api.github.com/repos/brkyvz/spark/stargazers","contributors_url":"https://api.github.com/repos/brkyvz/spark/contributors","subscribers_url":"https://api.github.com/repos/brkyvz/spark/subscribers","subscription_url":"https://api.github.com/repos/brkyvz/spark/subscription","commits_url":"https://api.github.com/repos/brkyvz/spark/commits{/sha}","git_commits_url":"https://api.github.com/repos/brkyvz/spark/git/commits{/sha}","comments_url":"https://api.github.com/repos/brkyvz/spark/comments{/number}","issue_comment_url":"https://api.github.com/repos/brkyvz/spark/issues/comments{/number}","contents_url":"https://api.github.com/repos/brkyvz/spark/contents/{+path}","compare_url":"https://api.github.com/repos/brkyvz/spark/compare/{base}...{head}","merges_url":"https://api.github.com/repos/brkyvz/spark/merges","archive_url":"https://api.github.com/repos/brkyvz/spark/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/brkyvz/spark/downloads","issues_url":"https://api.github.com/repos/brkyvz/spark/issues{/number}","pulls_url":"https://api.github.com/repos/brkyvz/spark/pulls{/number}","milestones_url":"https://api.github.com/repos/brkyvz/spark/milestones{/number}","notifications_url":"https://api.github.com/repos/brkyvz/spark/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/brkyvz/spark/labels{/name}","releases_url":"https://api.github.com/repos/brkyvz/spark/releases{/id}","deployments_url":"https://api.github.com/repos/brkyvz/spark/deployments","created_at":"2014-07-16T21:51:06Z","updated_at":"2015-05-08T16:54:55Z","pushed_at":"2016-11-22T01:59:45Z","git_url":"git://github.com/brkyvz/spark.git","ssh_url":"git@github.com:brkyvz/spark.git","clone_url":"https://github.com/brkyvz/spark.git","svn_url":"https://github.com/brkyvz/spark","homepage":null,"size":243682,"stargazers_count":0,"watchers_count":0,"language":"Scala","has_issues":false,"has_downloads":true,"has_wiki":false,"has_pages":false,"forks_count":0,"mirror_url":null,"open_issues_count":0,"forks":0,"open_issues":0,"watchers":0,"default_branch":"master"}},"base":{"label":"apache:master","ref":"master","sha":"6659ae555a464c7a16881b660265061481c0d25f","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-22T01:50:19Z","pushed_at":"2016-11-22T01:59:46Z","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":611806,"stargazers_count":10814,"watchers_count":10814,"language":"Scala","has_issues":false,"has_downloads":true,"has_wiki":false,"has_pages":false,"forks_count":10081,"mirror_url":"git://git.apache.org/spark.git","open_issues_count":454,"forks":10081,"open_issues":454,"watchers":10814,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/apache/spark/pulls/15951"},"html":{"href":"https://github.com/apache/spark/pull/15951"},"issue":{"href":"https://api.github.com/repos/apache/spark/issues/15951"},"comments":{"href":"https://api.github.com/repos/apache/spark/issues/15951/comments"},"review_comments":{"href":"https://api.github.com/repos/apache/spark/pulls/15951/comments"},"review_comment":{"href":"https://api.github.com/repos/apache/spark/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/apache/spark/pulls/15951/commits"},"statuses":{"href":"https://api.github.com/repos/apache/spark/statuses/97003e24ea377ae0d844e9f90f020b2111339661"}}}}
{"actor":{"display_login":"brkyvz"}}