-
Notifications
You must be signed in to change notification settings - Fork 29.3k
[SPARK-19633][SS] FileSource read from FileSink #16987
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 1 commit
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 |
|---|---|---|
|
|
@@ -662,6 +662,114 @@ class FileStreamSourceSuite extends FileStreamSourceTest { | |
| } | ||
| } | ||
|
|
||
| test("read data from outputs of another streaming query") { | ||
| withSQLConf(SQLConf.FILE_SINK_LOG_COMPACT_INTERVAL.key -> "3") { | ||
| withTempDirs { case (dir, tmp) => | ||
|
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.
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. fixed |
||
| // q1 is a streaming query that reads from memory and writes to text files | ||
| val q1_source = MemoryStream[String] | ||
|
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: please don't use
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. fixed |
||
| val q1_checkpointDir = new File(dir, "q1_checkpointDir").getCanonicalPath | ||
| val q1_outputDir = new File(dir, "q1_outputDir").getCanonicalPath | ||
| val q1 = | ||
| q1_source | ||
| .toDF() | ||
| .writeStream | ||
| .option("checkpointLocation", q1_checkpointDir) | ||
| .format("text") | ||
| .start(q1_outputDir) | ||
|
|
||
| // q2 is a streaming query that reads q1's text outputs | ||
| val q2 = createFileStream("text", q1_outputDir).filter($"value" contains "keep") | ||
|
|
||
| def q1AddData(data: String*): StreamAction = | ||
| Execute { _ => | ||
| q1_source.addData(data) | ||
| q1.processAllAvailable() | ||
| } | ||
| def q2ProcessAllAvailable(): StreamAction = Execute { q2 => q2.processAllAvailable() } | ||
|
|
||
| testStream(q2)( | ||
| // batch 0 | ||
| q1AddData("drop1", "keep2"), | ||
| q2ProcessAllAvailable(), | ||
| CheckAnswer("keep2"), | ||
|
|
||
| // batch 1 | ||
| Assert { | ||
| // create a text file that won't be on q1's sink log | ||
| // thus even if its contents contains "keep", it should NOT appear in q2's answer | ||
| val shouldNotKeep = new File(q1_outputDir, "should_not_keep.txt") | ||
| stringToFile(shouldNotKeep, "should_not_keep!!!") | ||
| shouldNotKeep.exists() | ||
| }, | ||
| q1AddData("keep3"), | ||
| q2ProcessAllAvailable(), | ||
| CheckAnswer("keep2", "keep3"), | ||
|
|
||
| // batch 2: check that things work well when the sink log gets compacted | ||
| q1AddData("keep4"), | ||
| Assert { | ||
| // compact interval is 3, so file "2.compact" should exist | ||
| new File(q1_outputDir, s"${FileStreamSink.metadataDir}/2.compact").exists() | ||
| }, | ||
| q2ProcessAllAvailable(), | ||
| CheckAnswer("keep2", "keep3", "keep4"), | ||
|
|
||
| // stop q1 manually | ||
|
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:
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. fixed |
||
| Execute { _ => q1.stop() } | ||
| ) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| test("read partitioned data from outputs of another streaming query") { | ||
|
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. This test seems not necessary. It will pass even if the source doesn't use the partition information.
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. In the long term, we should write the partition information to the file sink log, then we can read it in the file source. However, it's out of scope. If you have time, you can think about it and submit a new PR after this one.
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. test removed -- Let me think about this write partition infommation thing :) |
||
| withTempDirs { case (dir, tmp) => | ||
| // q1 is a streaming query that reads from memory and writes to partitioned json files | ||
| val q1_source = MemoryStream[(String, String)] | ||
| val q1_checkpointDir = new File(dir, "q1_checkpointDir").getCanonicalPath | ||
| val q1_outputDir = new File(dir, "q1_outputDir").getCanonicalPath | ||
| val q1 = | ||
| q1_source | ||
| .toDF() | ||
| .select($"_1" as "partition", $"_2" as "value") | ||
| .writeStream | ||
| .option("checkpointLocation", q1_checkpointDir) | ||
| .partitionBy("partition") | ||
| .format("json") | ||
| .start(q1_outputDir) | ||
|
|
||
| // q2 is a streaming query that reads q1's partitioned json outputs | ||
| val schema = new StructType().add("value", StringType).add("partition", StringType) | ||
| val q2 = createFileStream("json", q1_outputDir, Some(schema)).filter($"value" contains "keep") | ||
|
|
||
| def q1AddData(data: (String, String)*): StreamAction = | ||
| Execute { _ => | ||
| q1_source.addData(data) | ||
| q1.processAllAvailable() | ||
| } | ||
| def q2ProcessAllAvailable(): StreamAction = Execute { q2 => q2.processAllAvailable() } | ||
|
|
||
| testStream(q2)( | ||
| // batch 0: append to a new partition=foo, should read value and partition | ||
| q1AddData(("foo", "drop1"), ("foo", "keep2")), | ||
| q2ProcessAllAvailable(), | ||
| CheckAnswer(("keep2", "foo")), | ||
|
|
||
| // batch 1: append to same partition=foo, should read value and partition | ||
| q1AddData(("foo", "keep3")), | ||
| q2ProcessAllAvailable(), | ||
| CheckAnswer(("keep2", "foo"), ("keep3", "foo")), | ||
|
|
||
| // batch 2: append to a different partition=bar, should read value and partition | ||
| q1AddData(("bar", "keep4")), | ||
| q2ProcessAllAvailable(), | ||
| CheckAnswer(("keep2", "foo"), ("keep3", "foo"), ("keep4", "bar")), | ||
|
|
||
| // stop q1 manually | ||
| Execute { _ => q1.stop() } | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| test("when schema inference is turned on, should read partition data") { | ||
| def createFile(content: String, src: File, tmp: File): Unit = { | ||
| val tempFile = Utils.tempFileWith(new File(tmp, "text")) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -208,6 +208,11 @@ trait StreamTest extends QueryTest with SharedSQLContext with Timeouts { | |
| } | ||
| } | ||
|
|
||
| /** Execute arbitrary code */ | ||
| case class Execute(val func: StreamExecution => Any) extends StreamAction { | ||
|
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. How about just make this extend
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. fixed, thanks! |
||
| override def toString: String = s"Execute(<func>)" | ||
| } | ||
|
|
||
| class StreamManualClock(time: Long = 0L) extends ManualClock(time) with Serializable { | ||
| private var waitStartTime: Option[Long] = None | ||
|
|
||
|
|
@@ -472,10 +477,16 @@ trait StreamTest extends QueryTest with SharedSQLContext with Timeouts { | |
|
|
||
| case a: AssertOnQuery => | ||
| verify(currentStream != null || lastStream != null, | ||
| "cannot assert when not stream has been started") | ||
| "cannot assert when no stream has been started") | ||
| val streamToAssert = Option(currentStream).getOrElse(lastStream) | ||
| verify(a.condition(streamToAssert), s"Assert on query failed: ${a.message}") | ||
|
|
||
| case exe: Execute => | ||
| verify(currentStream != null || lastStream != null, | ||
| "cannot execute when no stream has been started") | ||
| val streamToExecute = Option(currentStream).getOrElse(lastStream) | ||
| exe.func(streamToExecute) | ||
|
|
||
| case a: Assert => | ||
| val streamToAssert = Option(currentStream).getOrElse(lastStream) | ||
| verify({ a.run(); true }, s"Assert failed: ${a.message}") | ||
|
|
||
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.
Just found one corner case: if the query to write files has not yet started, the current folder will contain no files even it's an output folder of the file sink. I think we should always call
sourceHasMetadatauntil the folder is not empty.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.
Actually, why not just change
sourceHasMetadatato a method?sparkSession.sessionState.newHadoopConf()seems expensive but we can save it into a field.Uh oh!
There was an error while loading. Please reload this page.
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.
ah thanks! I was about to change it to a method which would stop detecting once we know for sure to use a metadatafileindex or a inmemoryfileindex and remember this information. will push an udpate soon.
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.
and add a dedicated test case of course