szehon-ho commented on code in PR #57865:
URL: https://github.com/apache/spark/pull/57865#discussion_r3806937264
##########
sql/core/src/main/scala/org/apache/spark/sql/classic/DataFrameWriter.scala:
##########
@@ -354,13 +358,14 @@ final class DataFrameWriter[T] private[sql](ds:
Dataset[T]) extends sql.DataFram
}
private def insertIntoCommand(catalog: CatalogPlugin, ident: Identifier):
LogicalPlan = {
- import org.apache.spark.sql.connector.catalog.CatalogV2Implicits._
-
- val table = catalog.asTableCatalog.loadTable(ident,
getWritePrivileges.toSet.asJava) match {
+ val tableOptions = new CaseInsensitiveStringMap(extraOptions.toMap.asJava)
+ val table = CatalogV2Util.loadTableForWrite(
+ catalog, ident, getWritePrivileges, tableOptions) match {
case _: V1Table =>
return insertIntoCommand(TableIdentifier(ident.name(),
ident.namespace().headOption))
case t =>
- DataSourceV2Relation.create(t, Some(catalog), Some(ident))
+ CatalogV2Util.rejectTimeTravelOptionsForWrite(catalog, ident,
tableOptions)
Review Comment:
Non-blocking. Moving the reject after the load also put it after a load that
can throw: `insertIntoCommand` doesn't catch `NoSuchTableException`, so
`df.write.option("versionAsOf", "1").insertInto("testcat.ns1.missing")` reports
`TABLE_OR_VIEW_NOT_FOUND`, where `saveAsTable` (`:508`), `save()` (`:180`) and
`DataFrameWriterV2` all report `UNSUPPORTED_FEATURE.TIME_TRAVEL`.
"Table not found" is arguably the better error, so no behavior change needed
-- but the new matrix test reads as exhaustive while using an existing table
for `insertInto` (`DataSourceV2OptionSuite:1459`). Either pin the
missing-target case or scope "rejected consistently" to existing targets.
##########
sql/core/src/test/scala/org/apache/spark/sql/connector/DatasourceV2SQLBase.scala:
##########
@@ -26,6 +26,8 @@ import org.apache.spark.sql.test.SharedSparkSession
trait DatasourceV2SQLBase extends SharedSparkSession with BeforeAndAfter {
+ protected def testCatalogClass: Class[_ <: CatalogPlugin] =
classOf[InMemoryCatalog]
Review Comment:
Non-blocking. Nothing overrides this hook -- `DataSourceV2OptionSuite` now
declares state keys via
`withSQLConf("spark.sql.catalog.testcat.tableStateOptionKeys" -> loadOption)`,
and the default here is the class that was hardcoded before. Worth reverting
this line and `:40` to `classOf[InMemoryCatalog]`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
