[GitHub] rdblue commented on a change in pull request #23266: [SPARK-26313][SQL] move read related methods from Table to read related mix-in traits
rdblue commented on a change in pull request #23266: [SPARK-26313][SQL] move
read related methods from Table to read related mix-in traits
URL: https://github.com/apache/spark/pull/23266#discussion_r240749251
##
File path:
sql/core/src/main/java/org/apache/spark/sql/sources/v2/SupportsBatchRead.java
##
@@ -20,14 +20,27 @@
import org.apache.spark.annotation.Evolving;
import org.apache.spark.sql.sources.v2.reader.Scan;
import org.apache.spark.sql.sources.v2.reader.ScanBuilder;
+import org.apache.spark.sql.types.StructType;
/**
- * An empty mix-in interface for {@link Table}, to indicate this table
supports batch scan.
- *
- * If a {@link Table} implements this interface, its {@link
Table#newScanBuilder(DataSourceOptions)}
- * must return a {@link ScanBuilder} that builds {@link Scan} with {@link
Scan#toBatch()}
- * implemented.
- *
+ * A mix-in interface for {@link Table} to provide data reading ability of
batch processing.
*/
@Evolving
-public interface SupportsBatchRead extends Table { }
+public interface SupportsBatchRead extends Table {
+
+ /**
+ * Returns the schema of this table.
+ */
+ StructType schema();
Review comment:
Did we? What are the examples of sources that are write-only and don't need
schema validation? I could be wrong here, but I didn't think that there were
many.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] rdblue commented on a change in pull request #23266: [SPARK-26313][SQL] move read related methods from Table to read related mix-in traits
rdblue commented on a change in pull request #23266: [SPARK-26313][SQL] move
read related methods from Table to read related mix-in traits
URL: https://github.com/apache/spark/pull/23266#discussion_r240726681
##
File path:
sql/core/src/main/java/org/apache/spark/sql/sources/v2/SupportsBatchRead.java
##
@@ -20,14 +20,27 @@
import org.apache.spark.annotation.Evolving;
import org.apache.spark.sql.sources.v2.reader.Scan;
import org.apache.spark.sql.sources.v2.reader.ScanBuilder;
+import org.apache.spark.sql.types.StructType;
/**
- * An empty mix-in interface for {@link Table}, to indicate this table
supports batch scan.
- *
- * If a {@link Table} implements this interface, its {@link
Table#newScanBuilder(DataSourceOptions)}
- * must return a {@link ScanBuilder} that builds {@link Scan} with {@link
Scan#toBatch()}
- * implemented.
- *
+ * A mix-in interface for {@link Table} to provide data reading ability of
batch processing.
*/
@Evolving
-public interface SupportsBatchRead extends Table { }
+public interface SupportsBatchRead extends Table {
+
+ /**
+ * Returns the schema of this table.
+ */
+ StructType schema();
Review comment:
I think that the use case for a table with no schema is extremely narrow, if
not laughably unlikely.
Silly use cases should not cause us to make changes to an API when there is
a reasonable alternative: a source with no schema should return an empty schema
and signal that it wants to disable write validation (using a capability).
There are two advantages to this approach:
1. Implementations need to implement `schema` and validation rules are
enabled by default. If an implementation needs to remember to add a `HasSchema`
interface to turn on validation rules, then the default is wrong. The result
would be that people less familiar with the API will not have validated writes,
and that's a problem.
2. The API is simpler and easier to understand.
I'm -1 on moving this from `Table`.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] rdblue commented on a change in pull request #23266: [SPARK-26313][SQL] move read related methods from Table to read related mix-in traits
rdblue commented on a change in pull request #23266: [SPARK-26313][SQL] move
read related methods from Table to read related mix-in traits
URL: https://github.com/apache/spark/pull/23266#discussion_r240695794
##
File path:
sql/core/src/main/java/org/apache/spark/sql/sources/v2/SupportsBatchRead.java
##
@@ -20,14 +20,27 @@
import org.apache.spark.annotation.Evolving;
import org.apache.spark.sql.sources.v2.reader.Scan;
import org.apache.spark.sql.sources.v2.reader.ScanBuilder;
+import org.apache.spark.sql.types.StructType;
/**
- * An empty mix-in interface for {@link Table}, to indicate this table
supports batch scan.
- *
- * If a {@link Table} implements this interface, its {@link
Table#newScanBuilder(DataSourceOptions)}
- * must return a {@link ScanBuilder} that builds {@link Scan} with {@link
Scan#toBatch()}
- * implemented.
- *
+ * A mix-in interface for {@link Table} to provide data reading ability of
batch processing.
*/
@Evolving
-public interface SupportsBatchRead extends Table { }
+public interface SupportsBatchRead extends Table {
+
+ /**
+ * Returns the schema of this table.
+ */
+ StructType schema();
Review comment:
I should also note that writing to a source that can't be read and doesn't
have a consistent schema is far outside the primary uses of this API. Keep in
mind that we should design primarily for tables with schemas; it's great to
make that use case possible, but we should not alter the design too much to do
it.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] rdblue commented on a change in pull request #23266: [SPARK-26313][SQL] move read related methods from Table to read related mix-in traits
rdblue commented on a change in pull request #23266: [SPARK-26313][SQL] move
read related methods from Table to read related mix-in traits
URL: https://github.com/apache/spark/pull/23266#discussion_r240695027
##
File path:
sql/core/src/main/java/org/apache/spark/sql/sources/v2/SupportsBatchRead.java
##
@@ -20,14 +20,27 @@
import org.apache.spark.annotation.Evolving;
import org.apache.spark.sql.sources.v2.reader.Scan;
import org.apache.spark.sql.sources.v2.reader.ScanBuilder;
+import org.apache.spark.sql.types.StructType;
/**
- * An empty mix-in interface for {@link Table}, to indicate this table
supports batch scan.
- *
- * If a {@link Table} implements this interface, its {@link
Table#newScanBuilder(DataSourceOptions)}
- * must return a {@link ScanBuilder} that builds {@link Scan} with {@link
Scan#toBatch()}
- * implemented.
- *
+ * A mix-in interface for {@link Table} to provide data reading ability of
batch processing.
*/
@Evolving
-public interface SupportsBatchRead extends Table { }
+public interface SupportsBatchRead extends Table {
+
+ /**
+ * Returns the schema of this table.
+ */
+ StructType schema();
Review comment:
@cloud-fan, in that case, we're talking about a change to how we apply
validation rules. I've proposed a way to customize the validation rules for a
data source, using capabilities.
For example, we could add a capability that instructs Spark to not validate
the schema for a write. I don't think that's very useful, so I think it is more
likely that we would add a capability that instructs Spark to loosen the
validation in some way -- like the one I've already suggested that allows
omitting columns that are optional.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] rdblue commented on a change in pull request #23266: [SPARK-26313][SQL] move read related methods from Table to read related mix-in traits
rdblue commented on a change in pull request #23266: [SPARK-26313][SQL] move
read related methods from Table to read related mix-in traits
URL: https://github.com/apache/spark/pull/23266#discussion_r240407918
##
File path:
sql/core/src/main/java/org/apache/spark/sql/sources/v2/SupportsBatchRead.java
##
@@ -20,14 +20,27 @@
import org.apache.spark.annotation.Evolving;
import org.apache.spark.sql.sources.v2.reader.Scan;
import org.apache.spark.sql.sources.v2.reader.ScanBuilder;
+import org.apache.spark.sql.types.StructType;
/**
- * An empty mix-in interface for {@link Table}, to indicate this table
supports batch scan.
- *
- * If a {@link Table} implements this interface, its {@link
Table#newScanBuilder(DataSourceOptions)}
- * must return a {@link ScanBuilder} that builds {@link Scan} with {@link
Scan#toBatch()}
- * implemented.
- *
+ * A mix-in interface for {@link Table} to provide data reading ability of
batch processing.
*/
@Evolving
-public interface SupportsBatchRead extends Table { }
+public interface SupportsBatchRead extends Table {
+
+ /**
+ * Returns the schema of this table.
+ */
+ StructType schema();
Review comment:
I think `Table` should also have a `schema`. I agree that it is named, but
it is also a data source or sink of some kind, with a schema. I don't see the
value in moving the schema to a different interface, and I think that moving
the schema to an interface specific to the read path is worse because it causes
the problem that a table must be readable to be writable.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] rdblue commented on a change in pull request #23266: [SPARK-26313][SQL] move read related methods from Table to read related mix-in traits
rdblue commented on a change in pull request #23266: [SPARK-26313][SQL] move
read related methods from Table to read related mix-in traits
URL: https://github.com/apache/spark/pull/23266#discussion_r240395676
##
File path:
sql/core/src/main/java/org/apache/spark/sql/sources/v2/SupportsBatchRead.java
##
@@ -20,14 +20,27 @@
import org.apache.spark.annotation.Evolving;
import org.apache.spark.sql.sources.v2.reader.Scan;
import org.apache.spark.sql.sources.v2.reader.ScanBuilder;
+import org.apache.spark.sql.types.StructType;
/**
- * An empty mix-in interface for {@link Table}, to indicate this table
supports batch scan.
- *
- * If a {@link Table} implements this interface, its {@link
Table#newScanBuilder(DataSourceOptions)}
- * must return a {@link ScanBuilder} that builds {@link Scan} with {@link
Scan#toBatch()}
- * implemented.
- *
+ * A mix-in interface for {@link Table} to provide data reading ability of
batch processing.
*/
@Evolving
-public interface SupportsBatchRead extends Table { }
+public interface SupportsBatchRead extends Table {
+
+ /**
+ * Returns the schema of this table.
+ */
+ StructType schema();
Review comment:
@dongjoon-hyun, the problem that you pointed out was that the schema
shouldn't require a reader.
That's what I'm saying here, too: the table should have a schema and it
shouldn't need to implement the read path to validate a write using the table
schema.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] rdblue commented on a change in pull request #23266: [SPARK-26313][SQL] move read related methods from Table to read related mix-in traits
rdblue commented on a change in pull request #23266: [SPARK-26313][SQL] move
read related methods from Table to read related mix-in traits
URL: https://github.com/apache/spark/pull/23266#discussion_r240379459
##
File path:
sql/core/src/main/java/org/apache/spark/sql/sources/v2/SupportsBatchRead.java
##
@@ -20,14 +20,27 @@
import org.apache.spark.annotation.Evolving;
import org.apache.spark.sql.sources.v2.reader.Scan;
import org.apache.spark.sql.sources.v2.reader.ScanBuilder;
+import org.apache.spark.sql.types.StructType;
/**
- * An empty mix-in interface for {@link Table}, to indicate this table
supports batch scan.
- *
- * If a {@link Table} implements this interface, its {@link
Table#newScanBuilder(DataSourceOptions)}
- * must return a {@link ScanBuilder} that builds {@link Scan} with {@link
Scan#toBatch()}
- * implemented.
- *
+ * A mix-in interface for {@link Table} to provide data reading ability of
batch processing.
*/
@Evolving
-public interface SupportsBatchRead extends Table { }
+public interface SupportsBatchRead extends Table {
+
+ /**
+ * Returns the schema of this table.
+ */
+ StructType schema();
+
+ /**
+ * Returns a {@link ScanBuilder} which can be used to build a {@link Scan}
later. The built
Review comment:
Why does this say "later"?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] rdblue commented on a change in pull request #23266: [SPARK-26313][SQL] move read related methods from Table to read related mix-in traits
rdblue commented on a change in pull request #23266: [SPARK-26313][SQL] move
read related methods from Table to read related mix-in traits
URL: https://github.com/apache/spark/pull/23266#discussion_r240379327
##
File path:
sql/core/src/main/java/org/apache/spark/sql/sources/v2/SupportsBatchRead.java
##
@@ -20,14 +20,27 @@
import org.apache.spark.annotation.Evolving;
import org.apache.spark.sql.sources.v2.reader.Scan;
import org.apache.spark.sql.sources.v2.reader.ScanBuilder;
+import org.apache.spark.sql.types.StructType;
/**
- * An empty mix-in interface for {@link Table}, to indicate this table
supports batch scan.
- *
- * If a {@link Table} implements this interface, its {@link
Table#newScanBuilder(DataSourceOptions)}
- * must return a {@link ScanBuilder} that builds {@link Scan} with {@link
Scan#toBatch()}
- * implemented.
- *
+ * A mix-in interface for {@link Table} to provide data reading ability of
batch processing.
*/
@Evolving
-public interface SupportsBatchRead extends Table { }
+public interface SupportsBatchRead extends Table {
+
+ /**
+ * Returns the schema of this table.
+ */
+ StructType schema();
+
+ /**
+ * Returns a {@link ScanBuilder} which can be used to build a {@link Scan}
later. The built
+ * {@link Scan} must implement {@link Scan#toBatch()}. Spark will call this
method for each data
+ * scanning query.
Review comment:
Spark will call this method to configure each scan. Using "scanning query"
implies that it will be called just once per query, which isn't true if the
table is scanned twice in a query.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] rdblue commented on a change in pull request #23266: [SPARK-26313][SQL] move read related methods from Table to read related mix-in traits
rdblue commented on a change in pull request #23266: [SPARK-26313][SQL] move
read related methods from Table to read related mix-in traits
URL: https://github.com/apache/spark/pull/23266#discussion_r240379095
##
File path:
sql/core/src/main/java/org/apache/spark/sql/sources/v2/SupportsBatchRead.java
##
@@ -20,14 +20,27 @@
import org.apache.spark.annotation.Evolving;
import org.apache.spark.sql.sources.v2.reader.Scan;
import org.apache.spark.sql.sources.v2.reader.ScanBuilder;
+import org.apache.spark.sql.types.StructType;
/**
- * An empty mix-in interface for {@link Table}, to indicate this table
supports batch scan.
- *
- * If a {@link Table} implements this interface, its {@link
Table#newScanBuilder(DataSourceOptions)}
- * must return a {@link ScanBuilder} that builds {@link Scan} with {@link
Scan#toBatch()}
- * implemented.
- *
+ * A mix-in interface for {@link Table} to provide data reading ability of
batch processing.
*/
@Evolving
-public interface SupportsBatchRead extends Table { }
+public interface SupportsBatchRead extends Table {
+
+ /**
+ * Returns the schema of this table.
+ */
+ StructType schema();
+
+ /**
+ * Returns a {@link ScanBuilder} which can be used to build a {@link Scan}
later. The built
+ * {@link Scan} must implement {@link Scan#toBatch()}. Spark will call this
method for each data
+ * scanning query.
+ *
+ * The builder can take some query specific information to do operators
pushdown, and keep these
Review comment:
This doesn't need to explain how `ScanBuilder` works.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] rdblue commented on a change in pull request #23266: [SPARK-26313][SQL] move read related methods from Table to read related mix-in traits
rdblue commented on a change in pull request #23266: [SPARK-26313][SQL] move
read related methods from Table to read related mix-in traits
URL: https://github.com/apache/spark/pull/23266#discussion_r240378760
##
File path:
sql/core/src/main/java/org/apache/spark/sql/sources/v2/SupportsBatchRead.java
##
@@ -20,14 +20,27 @@
import org.apache.spark.annotation.Evolving;
import org.apache.spark.sql.sources.v2.reader.Scan;
import org.apache.spark.sql.sources.v2.reader.ScanBuilder;
+import org.apache.spark.sql.types.StructType;
/**
- * An empty mix-in interface for {@link Table}, to indicate this table
supports batch scan.
- *
- * If a {@link Table} implements this interface, its {@link
Table#newScanBuilder(DataSourceOptions)}
- * must return a {@link ScanBuilder} that builds {@link Scan} with {@link
Scan#toBatch()}
- * implemented.
- *
+ * A mix-in interface for {@link Table} to provide data reading ability of
batch processing.
Review comment:
This should be "A mix-in interface for readable tables ... This adds
`newScanBuilder` used to create a scan for batch, micro-batch, or continuous
processing."
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] rdblue commented on a change in pull request #23266: [SPARK-26313][SQL] move read related methods from Table to read related mix-in traits
rdblue commented on a change in pull request #23266: [SPARK-26313][SQL] move
read related methods from Table to read related mix-in traits
URL: https://github.com/apache/spark/pull/23266#discussion_r240378370
##
File path:
sql/core/src/main/java/org/apache/spark/sql/sources/v2/SupportsBatchRead.java
##
@@ -20,14 +20,27 @@
import org.apache.spark.annotation.Evolving;
import org.apache.spark.sql.sources.v2.reader.Scan;
import org.apache.spark.sql.sources.v2.reader.ScanBuilder;
+import org.apache.spark.sql.types.StructType;
/**
- * An empty mix-in interface for {@link Table}, to indicate this table
supports batch scan.
- *
- * If a {@link Table} implements this interface, its {@link
Table#newScanBuilder(DataSourceOptions)}
- * must return a {@link ScanBuilder} that builds {@link Scan} with {@link
Scan#toBatch()}
- * implemented.
- *
+ * A mix-in interface for {@link Table} to provide data reading ability of
batch processing.
*/
@Evolving
-public interface SupportsBatchRead extends Table { }
+public interface SupportsBatchRead extends Table {
Review comment:
Why would `newScanBuilder` be exposed by a batch interface? I think this
should be `SupportsRead` instead.
Checking whether a table can be used for batch processing should be done
using a different interface.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] rdblue commented on a change in pull request #23266: [SPARK-26313][SQL] move read related methods from Table to read related mix-in traits
rdblue commented on a change in pull request #23266: [SPARK-26313][SQL] move
read related methods from Table to read related mix-in traits
URL: https://github.com/apache/spark/pull/23266#discussion_r240377785
##
File path:
sql/core/src/main/java/org/apache/spark/sql/sources/v2/SupportsBatchRead.java
##
@@ -20,14 +20,27 @@
import org.apache.spark.annotation.Evolving;
import org.apache.spark.sql.sources.v2.reader.Scan;
import org.apache.spark.sql.sources.v2.reader.ScanBuilder;
+import org.apache.spark.sql.types.StructType;
/**
- * An empty mix-in interface for {@link Table}, to indicate this table
supports batch scan.
- *
- * If a {@link Table} implements this interface, its {@link
Table#newScanBuilder(DataSourceOptions)}
- * must return a {@link ScanBuilder} that builds {@link Scan} with {@link
Scan#toBatch()}
- * implemented.
- *
+ * A mix-in interface for {@link Table} to provide data reading ability of
batch processing.
*/
@Evolving
-public interface SupportsBatchRead extends Table { }
+public interface SupportsBatchRead extends Table {
+
+ /**
+ * Returns the schema of this table.
+ */
+ StructType schema();
Review comment:
I think that `schema` should be a method on `Table`. Write-only tables still
need to access the table's schema to validate a write.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
