Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/20387#discussion_r166392277
--- Diff:
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DataSourceV2Relation.scala
---
@@ -17,17 +17,151 @@
package org.apache.spark.sql.execution.datasources.v2
+import java.util.UUID
+
+import scala.collection.JavaConverters._
+import scala.collection.mutable
+
+import org.apache.spark.sql.{AnalysisException, SaveMode}
+import org.apache.spark.sql.catalyst.TableIdentifier
import org.apache.spark.sql.catalyst.analysis.MultiInstanceRelation
-import org.apache.spark.sql.catalyst.expressions.AttributeReference
-import org.apache.spark.sql.catalyst.plans.logical.{LeafNode, Statistics}
-import org.apache.spark.sql.sources.v2.reader._
+import org.apache.spark.sql.catalyst.expressions.{AttributeReference,
Expression}
+import org.apache.spark.sql.catalyst.plans.logical.{LeafNode, LogicalPlan,
Statistics}
+import org.apache.spark.sql.execution.datasources.DataSourceStrategy
+import org.apache.spark.sql.sources.{DataSourceRegister, Filter}
+import org.apache.spark.sql.sources.v2.{DataSourceOptions, DataSourceV2,
ReadSupport, ReadSupportWithSchema, WriteSupport}
+import org.apache.spark.sql.sources.v2.reader.{DataSourceReader,
SupportsPushDownCatalystFilters, SupportsPushDownFilters,
SupportsPushDownRequiredColumns, SupportsReportStatistics}
+import org.apache.spark.sql.sources.v2.writer.DataSourceWriter
+import org.apache.spark.sql.types.StructType
case class DataSourceV2Relation(
- fullOutput: Seq[AttributeReference],
- reader: DataSourceReader)
- extends LeafNode with MultiInstanceRelation with DataSourceReaderHolder {
+ source: DataSourceV2,
+ options: Map[String, String],
+ path: Option[String] = None,
+ table: Option[TableIdentifier] = None,
--- End diff --
I agree that `DataFrameReader` should not be the only place that creates
`DataSourceV2Relation`, so handling `DataSourceOptions` in `DataFrameReader` is
a bad idea.
But handling `DataSourceOptions` in `DataSourceV2Relation` is not the only
option. Like data source v1(see `DataSource`), we can have a central place to
deal with the data source stuff, which can also minimize the number of places
that need to handle `DataSourceOptions`.
The actual benefits I see are:
1. It's easier to write catalyst rules to match a `DataSourceV2Relation`
with a specific table identifier.
2. It's easier to reuse `DataSourceV2Relation` if we have data source v3
later, because we don't have any v2 specific stuff in the constructor.
For 1, I think we can solve it by defining a `DataSourceV2Relation.unapply`
to match the table identifier. For 2, I think it may not worth to consider it
at this point.
There might be more benefits I was missing, please correct me if I was
wrong, thanks!
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
