[SPARK-29331][SQL] create DS v2 Write at physical plan #26001
Conversation
|
Test build #111692 has finished for PR 26001 at commit
|
|
Test build #111708 has finished for PR 26001 at commit
|
| @@ -50,7 +50,7 @@ private[kafka010] object KafkaWriter extends Logging { | |||
| topic: Option[String] = None): Unit = { | |||
| schema.find(_.name == TOPIC_ATTRIBUTE_NAME).getOrElse( | |||
| if (topic.isEmpty) { | |||
| throw new AnalysisException(s"topic option required when no " + | |||
| throw new IllegalArgumentException(s"topic option required when no " + | |||
There was a problem hiding this comment.
Now we check the options at the physical plan phase, so this should not be AnalysisException anymore.
There was a problem hiding this comment.
If this is unacceptable, then we may need to have analysis time write info and runtime write info. Table.newWriteBuilder takes analysis time write info and WriteBuilder.build takes runtime write info.
I'm not sure if it's worth this complexity.
There was a problem hiding this comment.
Should it be SparkException? I think the last time we discussed these, it wasn't clear what type of exception to use after analysis. Maybe we need new exception types?
There was a problem hiding this comment.
We can use SparkException as well. IllegalArgumentException is a standard java exception which indicates invalid input, I think it's OK to use it even after analysis.
There was a problem hiding this comment.
I think we typically want to always raise SparkException because all exception types inherit from it. Unless we are throwing an exception from a method where there is an illegal argument, but that's not what is happening here.
There was a problem hiding this comment.
I think we typically want to always raise SparkException because all exception types inherit from it.
In Spark SQL no exceptions inherit from it. In fact SparkException was rarely used in Spark SQL before we adding the v2 commands. SparkException is defined in spark core and usually used when Spark fails to run a task.
In Spark SQL, AnalysisException and standard Java exceptions are more widely used.
There was a problem hiding this comment.
Sorry, I thought that AnalysisException inherited from SparkException. Looks like I was wrong.
|
also cc @rdblue |
|
Anything I can do to help push this PR forward? I'd love to get this in so I can finish the PR to add the number of partitions to DSv2 |
|
all the checks have passed in this PR. could it be merged? |
What changes were proposed in this pull request?
create DS v2 write at physical plan instead of logical plan, for streaming write.
Why are the changes needed?
We may need some physical information when creating DS v2 write, e.g. #25990 . This also matches batch write.
Does this PR introduce any user-facing change?
no
How was this patch tested?
existing tests