-
Notifications
You must be signed in to change notification settings - Fork 29.3k
[SPARK-53625][SS] Propagate metadata columns through projections to address ApplyCharTypePadding incompatibility #52375
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 2 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 |
|---|---|---|
|
|
@@ -31,7 +31,7 @@ import org.apache.spark.sql.functions._ | |
| import org.apache.spark.sql.internal.SQLConf | ||
| import org.apache.spark.sql.streaming.Trigger | ||
| import org.apache.spark.sql.test.SharedSparkSession | ||
| import org.apache.spark.sql.types.{IntegerType, LongType, StringType, StructField, StructType} | ||
| import org.apache.spark.sql.types.{IntegerType, LongType, MetadataBuilder, StringType, StructField, StructType} | ||
|
|
||
| class FileMetadataStructSuite extends QueryTest with SharedSparkSession { | ||
|
|
||
|
|
@@ -1133,4 +1133,98 @@ class FileMetadataStructSuite extends QueryTest with SharedSparkSession { | |
| assert(selectSingleRowDf.count() === 1) | ||
| } | ||
| } | ||
|
|
||
| Seq("true", "false").foreach { sideCharPadding => | ||
| test(s"SPARK-53625: file metadata in streaming with char type, " + | ||
| s"sideCharPadding=$sideCharPadding") { | ||
| withSQLConf(SQLConf.READ_SIDE_CHAR_PADDING.key -> sideCharPadding) { | ||
| withTempDir { dir => | ||
| import scala.jdk.CollectionConverters._ | ||
|
|
||
| val metadata = new MetadataBuilder() | ||
| .putString("__CHAR_VARCHAR_TYPE_STRING", "char(1)") | ||
| .build() | ||
| val charSchemaStruct = new StructType() | ||
| .add(StructField("char_col", StringType, metadata = metadata)) | ||
|
|
||
| val data: Seq[Row] = Seq(Row("A"), Row("B")) | ||
|
liviazhu marked this conversation as resolved.
Outdated
|
||
| val df = spark.createDataFrame(data.asJava, charSchemaStruct) | ||
| df.coalesce(1).write.format("json") | ||
| .save(dir.getCanonicalPath + "/source/new-streaming-data") | ||
|
|
||
| val streamDf = spark.readStream.format("json") | ||
| .schema(charSchemaStruct) | ||
| .load(dir.getCanonicalPath + "/source/new-streaming-data") | ||
| .select("*", "_metadata") | ||
|
|
||
| val streamQuery0 = streamDf | ||
| .writeStream.format("json") | ||
| .option("checkpointLocation", dir.getCanonicalPath + "/target/checkpoint") | ||
| .trigger(Trigger.AvailableNow()) | ||
| .start(dir.getCanonicalPath + "/target/new-streaming-data") | ||
|
|
||
| streamQuery0.awaitTermination() | ||
| assert(streamQuery0.lastProgress.numInputRows == 2L) | ||
|
|
||
| val newDF = spark.read.format("json") | ||
| .load(dir.getCanonicalPath + "/target/new-streaming-data") | ||
|
|
||
| val sourceFile = new File(dir, "/source/new-streaming-data").listFiles() | ||
| .filter(_.getName.endsWith(".json")).head | ||
| val sourceFileMetadata = Map( | ||
| METADATA_FILE_PATH -> sourceFile.toURI.toString, | ||
| METADATA_FILE_NAME -> sourceFile.getName, | ||
| METADATA_FILE_SIZE -> sourceFile.length(), | ||
| METADATA_FILE_BLOCK_START -> 0, | ||
| METADATA_FILE_BLOCK_LENGTH -> sourceFile.length(), | ||
| METADATA_FILE_MODIFICATION_TIME -> new Timestamp(sourceFile.lastModified()) | ||
| ) | ||
|
|
||
| // SELECT * will have: char_col, _metadata of /source/new-streaming-data | ||
| assert(newDF.select("*").columns.toSet == Set("char_col", "_metadata")) | ||
| // Verify the data is expected | ||
| checkAnswer( | ||
| newDF.select(col("char_col"), | ||
| col(METADATA_FILE_PATH), col(METADATA_FILE_NAME), | ||
| col(METADATA_FILE_SIZE), col(METADATA_FILE_BLOCK_START), | ||
| col(METADATA_FILE_BLOCK_LENGTH), | ||
| // since we are writing _metadata to a json file, | ||
| // we should explicitly cast the column to timestamp type | ||
| to_timestamp(col(METADATA_FILE_MODIFICATION_TIME))), | ||
| Seq( | ||
| Row( | ||
| "A", | ||
| sourceFileMetadata(METADATA_FILE_PATH), | ||
| sourceFileMetadata(METADATA_FILE_NAME), | ||
| sourceFileMetadata(METADATA_FILE_SIZE), | ||
| sourceFileMetadata(METADATA_FILE_BLOCK_START), | ||
| sourceFileMetadata(METADATA_FILE_BLOCK_LENGTH), | ||
| sourceFileMetadata(METADATA_FILE_MODIFICATION_TIME)), | ||
| Row( | ||
| "B", | ||
| sourceFileMetadata(METADATA_FILE_PATH), | ||
| sourceFileMetadata(METADATA_FILE_NAME), | ||
| sourceFileMetadata(METADATA_FILE_SIZE), | ||
| sourceFileMetadata(METADATA_FILE_BLOCK_START), | ||
| sourceFileMetadata(METADATA_FILE_BLOCK_LENGTH), | ||
| sourceFileMetadata(METADATA_FILE_MODIFICATION_TIME)) | ||
| ) | ||
| ) | ||
|
|
||
| checkAnswer( | ||
| newDF.where(s"$METADATA_FILE_SIZE > 0").select(METADATA_FILE_SIZE), | ||
|
Contributor
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 guess this is just a sanity check, right? Is this ever possible where a row is mapped to some file while the file has size 0?
Contributor
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. Yeah just a sanity check |
||
| Seq( | ||
| Row(sourceFileMetadata(METADATA_FILE_SIZE)), | ||
| Row(sourceFileMetadata(METADATA_FILE_SIZE))) | ||
| ) | ||
| checkAnswer( | ||
| newDF.where(s"$METADATA_FILE_SIZE > 0").select(METADATA_FILE_PATH), | ||
| Seq( | ||
| Row(sourceFileMetadata(METADATA_FILE_PATH)), | ||
| Row(sourceFileMetadata(METADATA_FILE_PATH))) | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
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.
This is too general and could mislead the impact of the config. We'd need to mention DSv1 and getBatch (or microbatch plan for the source) in the config name to scope it correctly.
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.
or we just remove the config. It's a bug fix and we get error anyway without this fix. It can't be worse.
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.
Makes sense to me. @liviazhu Let's remove this config.
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.
Done.