-
Notifications
You must be signed in to change notification settings - Fork 29.3k
[SPARK-27269][SQL] File source v2 should validate data schema only #24203
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.
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
Changes from 3 commits
b5aea7f
da55493
43743b3
214bd8b
d8b2638
63466b1
4ca742d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,7 +24,7 @@ import org.apache.spark.sql.{AnalysisException, SparkSession} | |
| import org.apache.spark.sql.execution.datasources._ | ||
| import org.apache.spark.sql.sources.v2.{SupportsRead, SupportsWrite, Table, TableCapability} | ||
| import org.apache.spark.sql.sources.v2.TableCapability._ | ||
| import org.apache.spark.sql.types.StructType | ||
| import org.apache.spark.sql.types.{DataType, StructType} | ||
| import org.apache.spark.sql.util.CaseInsensitiveStringMap | ||
| import org.apache.spark.sql.util.SchemaUtils | ||
|
|
||
|
|
@@ -46,17 +46,27 @@ abstract class FileTable( | |
| sparkSession, rootPathsSpecified, caseSensitiveMap, userSpecifiedSchema, fileStatusCache) | ||
| } | ||
|
|
||
| lazy val dataSchema: StructType = userSpecifiedSchema.orElse { | ||
| inferSchema(fileIndex.allFiles()) | ||
| }.getOrElse { | ||
| throw new AnalysisException( | ||
| s"Unable to infer schema for $name. It must be specified manually.") | ||
| }.asNullable | ||
| lazy val dataSchema: StructType = userSpecifiedSchema.map { schema => | ||
| val partitionSchema = fileIndex.partitionSchema | ||
| val equality = sparkSession.sessionState.conf.resolver | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The naming is following DataSource.scala line 185. I think it is OK.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mean the one line written two year ago? All the other new instances use
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you search with |
||
| StructType(schema.filterNot(f => partitionSchema.exists(p => equality(p.name, f.name)))) | ||
| }.orElse { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indentation? (https://github.com/databricks/scala-style-guide#pattern-matching) |
||
| inferSchema(fileIndex.allFiles()) | ||
| }.getOrElse { | ||
| throw new AnalysisException( | ||
| s"Unable to infer schema for $name. It must be specified manually.") | ||
| }.asNullable | ||
|
|
||
| override lazy val schema: StructType = { | ||
| val caseSensitive = sparkSession.sessionState.conf.caseSensitiveAnalysis | ||
| SchemaUtils.checkColumnNameDuplication(dataSchema.fieldNames, | ||
| "in the data schema", caseSensitive) | ||
| dataSchema.foreach { field => | ||
|
dongjoon-hyun marked this conversation as resolved.
|
||
| if (!supportsDataType(field.dataType)) { | ||
| throw new AnalysisException( | ||
| s"$name data source does not support ${field.dataType.catalogString} data type.") | ||
|
HyukjinKwon marked this conversation as resolved.
Outdated
|
||
| } | ||
| } | ||
| val partitionSchema = fileIndex.partitionSchema | ||
| SchemaUtils.checkColumnNameDuplication(partitionSchema.fieldNames, | ||
| "in the partition schema", caseSensitive) | ||
|
|
@@ -72,6 +82,22 @@ abstract class FileTable( | |
| * Spark will require that user specify the schema manually. | ||
| */ | ||
| def inferSchema(files: Seq[FileStatus]): Option[StructType] | ||
|
|
||
| /** | ||
| * Returns whether this format supports the given [[DataType]] in write path. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| * By default all data types are supported. | ||
| */ | ||
| def supportsDataType(dataType: DataType): Boolean = true | ||
|
|
||
| /** | ||
| * The string that represents the format that this data source provider uses. This is | ||
| * overridden by children to provide a nice alias for the data source. For example: | ||
| * | ||
| * {{{ | ||
| * override def formatName(): String = "ORC" | ||
| * }}} | ||
| */ | ||
| def formatName: String | ||
| } | ||
|
|
||
| object FileTable { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one or more | ||
| * contributor license agreements. See the NOTICE file distributed with | ||
| * this work for additional information regarding copyright ownership. | ||
| * The ASF licenses this file to You under the Apache License, Version 2.0 | ||
| * (the "License"); you may not use this file except in compliance with | ||
| * the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| package org.apache.spark.sql.execution.datasources.v2 | ||
|
|
||
| import scala.collection.JavaConverters._ | ||
|
|
||
| import org.apache.hadoop.fs.FileStatus | ||
|
|
||
| import org.apache.spark.sql.{QueryTest, SparkSession} | ||
| import org.apache.spark.sql.sources.v2.reader.ScanBuilder | ||
| import org.apache.spark.sql.sources.v2.writer.WriteBuilder | ||
| import org.apache.spark.sql.test.{SharedSQLContext, SQLTestUtils} | ||
| import org.apache.spark.sql.types._ | ||
| import org.apache.spark.sql.util.CaseInsensitiveStringMap | ||
|
|
||
| class DummyFileTable( | ||
| sparkSession: SparkSession, | ||
| options: CaseInsensitiveStringMap, | ||
| paths: Seq[String], | ||
| expectedDataSchema: StructType, | ||
| userSpecifiedSchema: Option[StructType]) | ||
| extends FileTable(sparkSession, options, paths, userSpecifiedSchema) { | ||
| override def inferSchema(files: Seq[FileStatus]): Option[StructType] = Some(expectedDataSchema) | ||
|
|
||
| override def name(): String = "Dummy" | ||
|
|
||
| override def formatName: String = "Dummy" | ||
|
|
||
| override def newScanBuilder(options: CaseInsensitiveStringMap): ScanBuilder = null | ||
|
|
||
| override def newWriteBuilder(options: CaseInsensitiveStringMap): WriteBuilder = null | ||
|
|
||
| override def supportsDataType(dataType: DataType): Boolean = dataType == StringType | ||
| } | ||
|
|
||
| class FileTableSuite extends QueryTest with SharedSQLContext with SQLTestUtils { | ||
|
|
||
| test("Data type validation should check data schema only") { | ||
| withTempPath { dir => | ||
| val df = spark.createDataFrame(Seq(("a", 1), ("b", 2))).toDF("v", "p") | ||
| val pathName = dir.getCanonicalPath | ||
| df.write.partitionBy("p").text(pathName) | ||
| val options = new CaseInsensitiveStringMap(Map("path" -> pathName).asJava) | ||
| val expectedDataSchema = StructType(Seq(StructField("v", StringType, true))) | ||
| // DummyFileTable doesn't support Integer data type. | ||
| // However, the partition schema is handled by Spark, so it is allowed to contain | ||
| // Integer data type here. | ||
| val table = new DummyFileTable(spark, options, Seq(pathName), expectedDataSchema, None) | ||
| assert(table.dataSchema == expectedDataSchema) | ||
| val expectedPartitionSchema = StructType(Seq(StructField("p", IntegerType, true))) | ||
| assert(table.fileIndex.partitionSchema == expectedPartitionSchema) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit. additional space after |
||
| } | ||
| } | ||
|
|
||
| test("Returns correct data schema when user specified schema contains partition schema") { | ||
|
dongjoon-hyun marked this conversation as resolved.
|
||
| withTempPath { dir => | ||
| val df = spark.createDataFrame(Seq(("a", 1), ("b", 2))).toDF("v", "p") | ||
| val pathName = dir.getCanonicalPath | ||
| df.write.partitionBy("p").text(pathName) | ||
| val options = new CaseInsensitiveStringMap(Map("path" -> pathName).asJava) | ||
| val userSpecifiedSchema = Some(StructType(Seq( | ||
| StructField("v", StringType, true), | ||
| StructField("p", IntegerType, true)))) | ||
| val expectedDataSchema = StructType(Seq(StructField("v", StringType, true))) | ||
| val table = | ||
| new DummyFileTable(spark, options, Seq(pathName), expectedDataSchema, userSpecifiedSchema) | ||
| assert(table.dataSchema == expectedDataSchema) | ||
| } | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.