[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

2018-12-11 Thread GitBox
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

2018-12-11 Thread GitBox
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

2018-12-11 Thread GitBox
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

2018-12-11 Thread GitBox
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

2018-12-10 Thread GitBox
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

2018-12-10 Thread GitBox
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

2018-12-10 Thread GitBox
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

2018-12-10 Thread GitBox
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

2018-12-10 Thread GitBox
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

2018-12-10 Thread GitBox
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

2018-12-10 Thread GitBox
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

2018-12-10 Thread GitBox
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