-
Notifications
You must be signed in to change notification settings - Fork 28.5k
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
[SPARK-51290][SQL] Enable filling default values in DSv2 writes #50044
base: master
Are you sure you want to change the base?
Changes from all commits
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 |
---|---|---|
|
@@ -423,8 +423,8 @@ abstract class V2WriteAnalysisSuiteBase extends AnalysisTest { | |
assertNotResolved(parsedPlan) | ||
assertAnalysisErrorCondition( | ||
parsedPlan, | ||
expectedErrorCondition = "INCOMPATIBLE_DATA_FOR_TABLE.CANNOT_FIND_DATA", | ||
expectedMessageParameters = Map("tableName" -> "`table-name`", "colName" -> "`x`") | ||
expectedErrorCondition = "INCOMPATIBLE_DATA_FOR_TABLE.EXTRA_COLUMNS", | ||
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. This is because of |
||
expectedMessageParameters = Map("tableName" -> "`table-name`", "extraColumns" -> "`a`, `b`") | ||
) | ||
} | ||
|
||
|
@@ -438,8 +438,8 @@ abstract class V2WriteAnalysisSuiteBase extends AnalysisTest { | |
assertNotResolved(parsedPlan) | ||
assertAnalysisErrorCondition( | ||
parsedPlan, | ||
expectedErrorCondition = "INCOMPATIBLE_DATA_FOR_TABLE.CANNOT_FIND_DATA", | ||
expectedMessageParameters = Map("tableName" -> "`table-name`", "colName" -> "`x`") | ||
expectedErrorCondition = "INCOMPATIBLE_DATA_FOR_TABLE.EXTRA_COLUMNS", | ||
expectedMessageParameters = Map("tableName" -> "`table-name`", "extraColumns" -> "`X`") | ||
) | ||
} | ||
|
||
|
@@ -513,12 +513,14 @@ abstract class V2WriteAnalysisSuiteBase extends AnalysisTest { | |
|
||
val parsedPlan = byName(table, query) | ||
|
||
assertNotResolved(parsedPlan) | ||
assertAnalysisErrorCondition( | ||
parsedPlan, | ||
expectedErrorCondition = "INCOMPATIBLE_DATA_FOR_TABLE.CANNOT_FIND_DATA", | ||
expectedMessageParameters = Map("tableName" -> "`table-name`", "colName" -> "`x`") | ||
) | ||
withSQLConf(SQLConf.USE_NULLS_FOR_MISSING_DEFAULT_COLUMN_VALUES.key -> "false") { | ||
assertNotResolved(parsedPlan) | ||
assertAnalysisErrorCondition( | ||
parsedPlan, | ||
expectedErrorCondition = "INCOMPATIBLE_DATA_FOR_TABLE.CANNOT_FIND_DATA", | ||
expectedMessageParameters = Map("tableName" -> "`table-name`", "colName" -> "`x`") | ||
) | ||
} | ||
} | ||
|
||
test("byName: insert safe cast") { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,7 +28,7 @@ import com.google.common.base.Objects | |
|
||
import org.apache.spark.sql.catalyst.InternalRow | ||
import org.apache.spark.sql.catalyst.expressions.{GenericInternalRow, JoinedRow, MetadataStructFieldWithLogicalName} | ||
import org.apache.spark.sql.catalyst.util.{CharVarcharUtils, DateTimeUtils} | ||
import org.apache.spark.sql.catalyst.util.{CharVarcharUtils, DateTimeUtils, ResolveDefaultColumns} | ||
import org.apache.spark.sql.connector.distributions.{Distribution, Distributions} | ||
import org.apache.spark.sql.connector.expressions._ | ||
import org.apache.spark.sql.connector.metric.{CustomMetric, CustomSumMetric, CustomTaskMetric} | ||
|
@@ -141,7 +141,8 @@ abstract class InMemoryBaseTable( | |
schema: StructType, | ||
row: InternalRow): (Any, DataType) = { | ||
val index = schema.fieldIndex(fieldNames(0)) | ||
val value = row.toSeq(schema).apply(index) | ||
val field = schema(index) | ||
val value = row.get(index, field.dataType) | ||
if (fieldNames.length > 1) { | ||
(value, schema(index).dataType) match { | ||
case (row: InternalRow, nestedSchema: StructType) => | ||
|
@@ -400,18 +401,23 @@ abstract class InMemoryBaseTable( | |
val sizeInBytes = numRows * rowSizeInBytes | ||
|
||
val numOfCols = tableSchema.fields.length | ||
val dataTypes = tableSchema.fields.map(_.dataType) | ||
val colValueSets = new Array[util.HashSet[Object]](numOfCols) | ||
val colValueSets = new Array[util.HashSet[Any]](numOfCols) | ||
val numOfNulls = new Array[Long](numOfCols) | ||
for (i <- 0 until numOfCols) { | ||
colValueSets(i) = new util.HashSet[Object] | ||
colValueSets(i) = new util.HashSet[Any] | ||
} | ||
|
||
inputPartitions.foreach(inputPartition => | ||
inputPartition.rows.foreach(row => | ||
for (i <- 0 until numOfCols) { | ||
colValueSets(i).add(row.get(i, dataTypes(i))) | ||
if (row.isNullAt(i)) { | ||
val field = tableSchema(i) | ||
val colValue = if (i < row.numFields) { | ||
row.get(i, field.dataType) | ||
} else { | ||
ResolveDefaultColumns.getExistenceDefaultValue(field) | ||
} | ||
colValueSets(i).add(colValue) | ||
if (colValue == null) { | ||
numOfNulls(i) += 1 | ||
} | ||
} | ||
|
@@ -718,6 +724,11 @@ private class BufferedRowsReader( | |
schema: StructType, | ||
row: InternalRow): Any = { | ||
val index = schema.fieldIndex(field.name) | ||
|
||
if (index >= row.numFields) { | ||
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. This is needed for support for adding columns with default values to the end. 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. This is method 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. This is needed to read data inserted prior to adding columns to the schema. If that happens, there would be extra columns in the schema and we have to default new columns using the existence default value. |
||
return ResolveDefaultColumns.getExistenceDefaultValue(field) | ||
} | ||
|
||
field.dataType match { | ||
case StructType(fields) => | ||
if (row.isNullAt(index)) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -122,7 +122,7 @@ class BasicInMemoryTableCatalog extends TableCatalog { | |
override def alterTable(ident: Identifier, changes: TableChange*): Table = { | ||
val table = loadTable(ident).asInstanceOf[InMemoryTable] | ||
val properties = CatalogV2Util.applyPropertiesChanges(table.properties, changes) | ||
val schema = CatalogV2Util.applySchemaChanges(table.schema, changes, None, "ALTER TABLE") | ||
val schema = CatalogV2Util.applySchemaChanges(table.schema, changes, Some(name), "ALTER TABLE") | ||
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 we need to add memory table provider to 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. Oh, nvm, the name is given when initializing the catalog. 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. Correct. |
||
val finalPartitioning = CatalogV2Util.applyClusterByChanges(table.partitioning, schema, changes) | ||
|
||
// fail if the last column in the schema was dropped | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,6 +37,14 @@ trait AlterTableTests extends SharedSparkSession with QueryErrorsBase { | |
|
||
protected val catalogAndNamespace: String | ||
|
||
protected def catalog: String = { | ||
if (catalogAndNamespace.nonEmpty) { | ||
catalogAndNamespace.split('.').headOption.getOrElse("spark_catalog") | ||
} else { | ||
"spark_catalog" | ||
} | ||
} | ||
|
||
protected val v2Format: String | ||
|
||
private def fullTableName(tableName: String): String = { | ||
|
@@ -328,7 +336,7 @@ trait AlterTableTests extends SharedSparkSession with QueryErrorsBase { | |
} | ||
|
||
test("SPARK-39383 DEFAULT columns on V2 data sources with ALTER TABLE ADD/ALTER COLUMN") { | ||
withSQLConf(SQLConf.DEFAULT_COLUMN_ALLOWED_PROVIDERS.key -> s"$v2Format, ") { | ||
withSQLConf(SQLConf.DEFAULT_COLUMN_ALLOWED_PROVIDERS.key -> s"$v2Format,$catalog") { | ||
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. How does this conf affect the testing v2 in-memory catalog? I thought it's only for v1 file source. 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. I think that it is because https://github.com/apache/spark/pull/50044/files#r1968081855 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. @viirya is correct. We previously passed |
||
val t = fullTableName("table_name") | ||
withTable("t") { | ||
sql(s"create table $t (a string) using $v2Format") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there is value in validating if the catalog defines
SUPPORT_COLUMN_DEFAULT_VALUE
in capabilities during writes. If a connector includes default value metadata in its schema, it should be enough to fill default values. The flag exists for ALTER and CREATE/REPLACE statements.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea true, Spark fills the default values during table writing and it works for all catalogs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean
supportColDefaultValue
is true or false doesn't matter for v2 here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see. You mean to check for the flag
SUPPORT_COLUMN_DEFAULT_VALUE
here for the catalog.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, I don't see value in checking
SUPPORT_COLUMN_DEFAULT_VALUE
here.