You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by marmbrus <gi...@git.apache.org> on 2014/09/21 03:38:10 UTC

[GitHub] spark pull request: [WIP][SPARK-3247][SQL] An API for adding forei...

GitHub user marmbrus opened a pull request:

    https://github.com/apache/spark/pull/2475

    [WIP][SPARK-3247][SQL] An API for adding foreign data sources to Spark SQL

    **Work in progress - APIs may change**
    
    This PR introduces a new set of APIs to Spark SQL that allow other developers to add support for reading data from new sources.  As an example, a library is included for reading data encoded using Avro.
    
    New sources must implement the interface `BaseRelation`, which is responsible for describing the schema of the data.  This base relation must also implement at least one `Scan` interface, which is responsible for producing an RDD containing row objects.  The various Scan interfaces allow for optimizations such as column pruning and filter push down, when the underlying data source can handle these operations.
    
    External data sources can be accessed using either the programatic API or using pure SQL.  For example, the included Avro library could be called from Scala query DSL as follows:
    
    ```scala
    import org.apache.spark.sql.avro._
    
    val results = TestSQLContext
      .avroFile("../hive/src/test/resources/data/files/episodes.avro")
      .select('title)
      .collect()
    ```
    
    The same can be done in pure SQL, for example from the SQL command line or JDBC interface.
    
    ```sql
    CREATE FOREIGN TEMPORARY TABLE avroTable
    USING org.apache.spark.sql.avro
    OPTIONS (path "../hive/src/test/resources/data/files/episodes.avro");
    
    SELECT * FROM avroTable;
    ```
    
    TODO:
     - [ ] Move command refactoring into separate PR
     - [ ] Transition parquet and json support to new API
     - [ ] Figure out how to package data sources and their dependencies
     - [ ] Examples / implementation of more advanced scan types
     - [ ] Support for foreign catalogs
     - [ ] Introspection like `describe` for foreign tables.
     - [ ] More tests

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/marmbrus/spark foreign

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/spark/pull/2475.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #2475
    
----
commit 47d542cc0238fba04b6c4e4456393d812d559c4e
Author: Michael Armbrust <mi...@databricks.com>
Date:   2014-09-20T23:20:50Z

    First draft of foreign data API

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [WIP][SPARK-3247][SQL] An API for adding forei...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-61037073
  
      [Test build #22509 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/22509/consoleFull) for   PR 2475 at commit [`0fd3a07`](https://github.com/apache/spark/commit/0fd3a07a29a58e763b6e0a0ad26ca860efdbb979).
     * This patch **fails Scala style tests**.
     * This patch merges cleanly.
     * This patch adds the following public classes _(experimental)_:
      * `case class StructField(name: String, dataType: DataType, nullable: Boolean = true) `
      * `trait RunnableCommand extends logical.Command `
      * `case class ExecutedCommand(cmd: RunnableCommand) extends SparkPlan `
      * `  protected case class Keyword(str: String)`
      * `            sys.error(s"Failed to load class for data source: $provider")`
      * `  trait RelationProvider `
      * `  abstract class BaseRelation `
      * `  trait TableScan `
      * `  trait PrunedScan `
      * `  trait FilteredScan `



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r19696134
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/sources/interfaces.scala ---
    @@ -0,0 +1,86 @@
    +/*
    +* Licensed to the Apache Software Foundation (ASF) under one or more
    +* contributor license agreements.  See the NOTICE file distributed with
    +* this work for additional information regarding copyright ownership.
    +* The ASF licenses this file to You under the Apache License, Version 2.0
    +* (the "License"); you may not use this file except in compliance with
    +* the License.  You may obtain a copy of the License at
    +*
    +*    http://www.apache.org/licenses/LICENSE-2.0
    +*
    +* Unless required by applicable law or agreed to in writing, software
    +* distributed under the License is distributed on an "AS IS" BASIS,
    +* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    +* See the License for the specific language governing permissions and
    +* limitations under the License.
    +*/
    +package org.apache.spark.sql.sources
    +
    +import org.apache.spark.annotation.DeveloperApi
    +import org.apache.spark.rdd.RDD
    +import org.apache.spark.sql.catalyst.expressions.{Expression, Attribute}
    +
    +/**
    + * Implemented by objects that produce relations for a specific kind of data source.  When
    + * Spark SQL is given a DDL operation with a USING clause specified, this interface is used to
    + * pass in the parameters specified by a user.
    + *
    + * Users may specify the fully qualified class name of a given data source.  When that class is
    + * not found Spark SQL will append the class name `DefaultSource` to the path, allowing for
    + * less verbose invocation.  For example, 'org.apache.spark.sql.json' would resolve to the
    + * data source 'org.apache.spark.sql.json.DefaultSource'
    + *
    + * A new instance of this class with be instantiated each time a DDL call is made.
    + */
    +@DeveloperApi
    +trait RelationProvider {
    +  /** Returns a new base relation with the given parameters. */
    +  def createRelation(sqlContext: SQLContext, parameters: Map[String, String]): BaseRelation
    +}
    +
    +/**
    + * Represents a collection of tuples with a known schema.  Classes that extend BaseRelation must
    + * be able to produce the schema of their data in the form of a [[StructType]]  In order to be
    + * executed, a BaseRelation must also mix in at least one of the Scan traits.
    + *
    + * BaseRelations must also define a equality function that only returns true when the two
    + * instances will return the same data.  This equality function is used when determining when
    + * it is safe to substitute cached results for a given relation.
    + */
    +@DeveloperApi
    +abstract class BaseRelation {
    +  def sqlContext: SQLContext
    +  def schema: StructType
    +}
    +
    +/**
    + * Mixed into a BaseRelation that can produce all of its tuples as an RDD of Row objects.
    + */
    +@DeveloperApi
    +abstract class TableScan extends BaseRelation {
    +  def buildScan(): RDD[Row]
    +}
    +
    +/**
    + * Mixed into a BaseRelation that can eliminate unneeded columns before producing an RDD
    + * containing all of its tuples as Row objects.
    + */
    +@DeveloperApi
    +abstract class PrunedScan extends BaseRelation {
    +  def buildScan(requiredColumns: Seq[Attribute]): RDD[Row]
    +}
    +
    +/**
    + * Mixed into a BaseRelation that can eliminate unneeded columns and filter using selected
    + * predicates before producing an RDD containing all matching tuples as Row objects.
    + *
    + * The pushed down filters are currently purely an optimization as they will all be evaluated
    + * again.  This means it is safe to use them with methods that produce false positives such
    + * as filtering partitions based on a bloom filter.
    + */
    +@DeveloperApi
    +abstract class FilteredScan extends BaseRelation {
    --- End diff --
    
    Yeah, I mean I want to avoid a proliferation of different interfaces we have to support.  The three of these seems like pretty reasonable tiers.  I'll rename it incase we ever add the Filtered only option.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-61220252
  
      [Test build #22593 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/22593/consoleFull) for   PR 2475 at commit [`22963ef`](https://github.com/apache/spark/commit/22963efddc59099a14d49228654db8ba0760f87b).
     * This patch merges cleanly.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-61387420
  
      [Test build #22711 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/22711/consoleFull) for   PR 2475 at commit [`1d41bb5`](https://github.com/apache/spark/commit/1d41bb50e8f5bc889b9019716462db8358be78f8).
     * This patch merges cleanly.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-61334955
  
      [Test build #22641 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/22641/consoleFull) for   PR 2475 at commit [`70da6d9`](https://github.com/apache/spark/commit/70da6d941b222385aa437bf7bfe6cabddad09cf8).
     * This patch **fails Scala style tests**.
     * This patch merges cleanly.
     * This patch adds the following public classes _(experimental)_:
      * `abstract class GenericStrategy[PhysicalPlan <: TreeNode[PhysicalPlan]] extends Logging `
      * `case class StructField(name: String, dataType: DataType, nullable: Boolean = true) `
      * `trait RunnableCommand extends logical.Command `
      * `case class ExecutedCommand(cmd: RunnableCommand) extends SparkPlan `
      * `  protected case class Keyword(str: String)`
      * `            sys.error(s"Failed to load class for data source: $provider")`
      * `case class EqualTo(attribute: String, value: Any) extends Filter`
      * `trait RelationProvider `
      * `abstract class BaseRelation `
      * `abstract class TableScan extends BaseRelation `
      * `abstract class PrunedScan extends BaseRelation `
      * `abstract class FilteredScan extends BaseRelation `



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [WIP][SPARK-3247][SQL] An API for adding forei...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-57355068
  
    Test FAILed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/21045/


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [WIP][SPARK-3247][SQL] An API for adding forei...

Posted by liancheng <gi...@git.apache.org>.
Github user liancheng commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-57281203
  
    Considering from a user's perspective, mixing all kinds of data sources is cool, but correspond to `TableScan`, we also need a `TableInsert` trait to close the loop. Usually a foreign data source should implement `TableScan`, and optionally implement `TableInsert`, which should be generally easier to implement than the former.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by liancheng <gi...@git.apache.org>.
Github user liancheng commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r19659305
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/sources/interfaces.scala ---
    @@ -0,0 +1,86 @@
    +/*
    +* Licensed to the Apache Software Foundation (ASF) under one or more
    +* contributor license agreements.  See the NOTICE file distributed with
    +* this work for additional information regarding copyright ownership.
    +* The ASF licenses this file to You under the Apache License, Version 2.0
    +* (the "License"); you may not use this file except in compliance with
    +* the License.  You may obtain a copy of the License at
    +*
    +*    http://www.apache.org/licenses/LICENSE-2.0
    +*
    +* Unless required by applicable law or agreed to in writing, software
    +* distributed under the License is distributed on an "AS IS" BASIS,
    +* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    +* See the License for the specific language governing permissions and
    +* limitations under the License.
    +*/
    +package org.apache.spark.sql.sources
    +
    +import org.apache.spark.annotation.DeveloperApi
    +import org.apache.spark.rdd.RDD
    +import org.apache.spark.sql.catalyst.expressions.{Expression, Attribute}
    +
    +/**
    + * Implemented by objects that produce relations for a specific kind of data source.  When
    + * Spark SQL is given a DDL operation with a USING clause specified, this interface is used to
    + * pass in the parameters specified by a user.
    + *
    + * Users may specify the fully qualified class name of a given data source.  When that class is
    + * not found Spark SQL will append the class name `DefaultSource` to the path, allowing for
    + * less verbose invocation.  For example, 'org.apache.spark.sql.json' would resolve to the
    + * data source 'org.apache.spark.sql.json.DefaultSource'
    + *
    + * A new instance of this class with be instantiated each time a DDL call is made.
    + */
    +@DeveloperApi
    +trait RelationProvider {
    +  /** Returns a new base relation with the given parameters. */
    +  def createRelation(sqlContext: SQLContext, parameters: Map[String, String]): BaseRelation
    +}
    +
    +/**
    + * Represents a collection of tuples with a known schema.  Classes that extend BaseRelation must
    + * be able to produce the schema of their data in the form of a [[StructType]]  In order to be
    + * executed, a BaseRelation must also mix in at least one of the Scan traits.
    --- End diff --
    
    All scan traits have been converted to abstract classes. Need to update this comment.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-61196071
  
    Test FAILed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22579/
    Test FAILed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-61220318
  
      [Test build #22593 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/22593/consoleFull) for   PR 2475 at commit [`22963ef`](https://github.com/apache/spark/commit/22963efddc59099a14d49228654db8ba0760f87b).
     * This patch **fails Scala style tests**.
     * This patch merges cleanly.
     * This patch adds the following public classes _(experimental)_:
      * `case class StructField(name: String, dataType: DataType, nullable: Boolean = true) `
      * `trait RunnableCommand extends logical.Command `
      * `case class ExecutedCommand(cmd: RunnableCommand) extends SparkPlan `
      * `  protected case class Keyword(str: String)`
      * `            sys.error(s"Failed to load class for data source: $provider")`
      * `trait RelationProvider `
      * `abstract class BaseRelation `
      * `abstract class TableScan extends BaseRelation `
      * `abstract class PrunedScan extends BaseRelation `
      * `abstract class FilteredScan extends BaseRelation `



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by mateiz <gi...@git.apache.org>.
Github user mateiz commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r19646622
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/sources/package.scala ---
    @@ -0,0 +1,95 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.spark.sql
    +
    +import org.apache.spark.annotation.DeveloperApi
    +import org.apache.spark.rdd.RDD
    +import org.apache.spark.sql.catalyst.expressions.{Expression, Attribute}
    +
    +/**
    + * A set of APIs for adding data sources to Spark SQL.
    + */
    +package object sources {
    +
    +  /**
    +   * Implemented by objects that produce relations for a specific kind of data source.  When
    +   * Spark SQL is given a DDL operation with a USING clause specified, this interface is used to
    +   * pass in the parameters specified by a user.
    +   *
    +   * Users may specify the fully qualified class name of a given data source.  When that class is
    +   * not found Spark SQL will append the class name `DefaultSource` to the path, allowing for
    +   * less verbose invocation.  For example, 'org.apache.spark.sql.json' would resolve to the
    +   * data source 'org.apache.spark.sql.json.DefaultSource'
    +   */
    +  trait RelationProvider {
    +    /** Returns a new base relation with the given parameters. */
    +    def createRelation(sqlContext: SQLContext, parameters: Map[String, String]): BaseRelation
    +  }
    +
    +  /**
    +   * Represents a collection of tuples with a known schema.  Classes that extend BaseRelation must
    +   * be able to produce the schema of their data in the form of a [[StructType]]  In order to be
    +   * executed, a BaseRelation must also mix in at least one of the Scan traits.
    +   *
    +   * BaseRelations must also define a equality function that only returns true when the two
    +   * instances will return the same data.  This equality function is used when determining when
    +   * it is safe to substitute cached results for a given relation.
    +   */
    +  @DeveloperApi
    +  abstract class BaseRelation {
    +    def sqlContext: SQLContext
    +    def schema: StructType
    +  }
    +
    +  /**
    +   * Mixed into a BaseRelation that can produce all of its tuples as an RDD of Row objects.
    +   */
    +  @DeveloperApi
    +  trait TableScan {
    +    self: BaseRelation =>
    --- End diff --
    
    I see, that seems like it's not that useful, since you can always detect whether all attributes are selected. Abstract classes seemed easier to understand. What do other people think? @rxin @yhuai 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [WIP][SPARK-3247][SQL] An API for adding forei...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-61037075
  
    Test FAILed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22509/
    Test FAILed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-61198593
  
      [Test build #22577 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/22577/consoleFull) for   PR 2475 at commit [`de3b68c`](https://github.com/apache/spark/commit/de3b68c4d28c7cf603c886b99476a2510ce307e8).
     * This patch **passes all tests**.
     * This patch merges cleanly.
     * This patch adds the following public classes _(experimental)_:
      * `case class StructField(name: String, dataType: DataType, nullable: Boolean = true) `
      * `trait RunnableCommand extends logical.Command `
      * `case class ExecutedCommand(cmd: RunnableCommand) extends SparkPlan `
      * `  protected case class Keyword(str: String)`
      * `            sys.error(s"Failed to load class for data source: $provider")`
      * `  trait RelationProvider `
      * `  abstract class BaseRelation `
      * `  trait TableScan `
      * `  trait PrunedScan `
      * `  trait FilteredScan `



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by mateiz <gi...@git.apache.org>.
Github user mateiz commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r19650306
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/sources/package.scala ---
    @@ -0,0 +1,95 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.spark.sql
    +
    +import org.apache.spark.annotation.DeveloperApi
    +import org.apache.spark.rdd.RDD
    +import org.apache.spark.sql.catalyst.expressions.{Expression, Attribute}
    +
    +/**
    + * A set of APIs for adding data sources to Spark SQL.
    + */
    +package object sources {
    +
    +  /**
    +   * Implemented by objects that produce relations for a specific kind of data source.  When
    +   * Spark SQL is given a DDL operation with a USING clause specified, this interface is used to
    +   * pass in the parameters specified by a user.
    +   *
    +   * Users may specify the fully qualified class name of a given data source.  When that class is
    +   * not found Spark SQL will append the class name `DefaultSource` to the path, allowing for
    +   * less verbose invocation.  For example, 'org.apache.spark.sql.json' would resolve to the
    +   * data source 'org.apache.spark.sql.json.DefaultSource'
    +   */
    +  trait RelationProvider {
    +    /** Returns a new base relation with the given parameters. */
    +    def createRelation(sqlContext: SQLContext, parameters: Map[String, String]): BaseRelation
    +  }
    +
    +  /**
    +   * Represents a collection of tuples with a known schema.  Classes that extend BaseRelation must
    +   * be able to produce the schema of their data in the form of a [[StructType]]  In order to be
    +   * executed, a BaseRelation must also mix in at least one of the Scan traits.
    +   *
    +   * BaseRelations must also define a equality function that only returns true when the two
    +   * instances will return the same data.  This equality function is used when determining when
    +   * it is safe to substitute cached results for a given relation.
    +   */
    +  @DeveloperApi
    +  abstract class BaseRelation {
    +    def sqlContext: SQLContext
    +    def schema: StructType
    +  }
    +
    +  /**
    +   * Mixed into a BaseRelation that can produce all of its tuples as an RDD of Row objects.
    +   */
    +  @DeveloperApi
    +  trait TableScan {
    +    self: BaseRelation =>
    +
    +    def buildScan(): RDD[Row]
    +  }
    +
    +  /**
    +   * Mixed into a BaseRelation that can eliminate unneeded columns before producing an RDD
    +   * containing all of its tuples as Row objects.
    +   */
    +  @DeveloperApi
    +  trait PrunedScan {
    +    self: BaseRelation =>
    +
    +    def buildScan(requiredColumns: Seq[Attribute]): RDD[Row]
    +  }
    +
    +  /**
    +   * Mixed into a BaseRelation that can eliminate unneeded columns and filter using selected
    +   * predicates before producing an RDD containing all matching tuples as Row objects.
    +   *
    +   * The pushed down filters are currently purely an optimization as they will all be evaluated
    +   * again.  This means it is safe to use them with methods that produce false positives such
    +   * as filtering partitions based on a bloom filter.
    --- End diff --
    
    Ah, good point about unsupported expressions. I like this proposal with the filter types.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by liancheng <gi...@git.apache.org>.
Github user liancheng commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r19658936
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/sources/interfaces.scala ---
    @@ -0,0 +1,86 @@
    +/*
    +* Licensed to the Apache Software Foundation (ASF) under one or more
    +* contributor license agreements.  See the NOTICE file distributed with
    +* this work for additional information regarding copyright ownership.
    +* The ASF licenses this file to You under the Apache License, Version 2.0
    +* (the "License"); you may not use this file except in compliance with
    +* the License.  You may obtain a copy of the License at
    +*
    +*    http://www.apache.org/licenses/LICENSE-2.0
    +*
    +* Unless required by applicable law or agreed to in writing, software
    +* distributed under the License is distributed on an "AS IS" BASIS,
    +* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    +* See the License for the specific language governing permissions and
    +* limitations under the License.
    +*/
    +package org.apache.spark.sql.sources
    +
    +import org.apache.spark.annotation.DeveloperApi
    +import org.apache.spark.rdd.RDD
    +import org.apache.spark.sql.catalyst.expressions.{Expression, Attribute}
    +
    +/**
    + * Implemented by objects that produce relations for a specific kind of data source.  When
    + * Spark SQL is given a DDL operation with a USING clause specified, this interface is used to
    + * pass in the parameters specified by a user.
    + *
    + * Users may specify the fully qualified class name of a given data source.  When that class is
    + * not found Spark SQL will append the class name `DefaultSource` to the path, allowing for
    + * less verbose invocation.  For example, 'org.apache.spark.sql.json' would resolve to the
    + * data source 'org.apache.spark.sql.json.DefaultSource'
    + *
    + * A new instance of this class with be instantiated each time a DDL call is made.
    + */
    +@DeveloperApi
    +trait RelationProvider {
    +  /** Returns a new base relation with the given parameters. */
    +  def createRelation(sqlContext: SQLContext, parameters: Map[String, String]): BaseRelation
    +}
    +
    +/**
    + * Represents a collection of tuples with a known schema.  Classes that extend BaseRelation must
    + * be able to produce the schema of their data in the form of a [[StructType]]  In order to be
    + * executed, a BaseRelation must also mix in at least one of the Scan traits.
    + *
    + * BaseRelations must also define a equality function that only returns true when the two
    + * instances will return the same data.  This equality function is used when determining when
    + * it is safe to substitute cached results for a given relation.
    --- End diff --
    
    Need a default `sameResult: BaseRelation => Boolean` implementation here and hook it in `LogicalRelation`, e.g.:
    
    ```scala
    def sameResult(otherRelation: BaseRelation) = this eq otherRelation
    ```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by scwf <gi...@git.apache.org>.
Github user scwf commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r19654557
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/sources/interfaces.scala ---
    @@ -0,0 +1,86 @@
    +/*
    +* Licensed to the Apache Software Foundation (ASF) under one or more
    +* contributor license agreements.  See the NOTICE file distributed with
    +* this work for additional information regarding copyright ownership.
    +* The ASF licenses this file to You under the Apache License, Version 2.0
    +* (the "License"); you may not use this file except in compliance with
    +* the License.  You may obtain a copy of the License at
    +*
    +*    http://www.apache.org/licenses/LICENSE-2.0
    +*
    +* Unless required by applicable law or agreed to in writing, software
    +* distributed under the License is distributed on an "AS IS" BASIS,
    +* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    +* See the License for the specific language governing permissions and
    +* limitations under the License.
    +*/
    +package org.apache.spark.sql.sources
    +
    +import org.apache.spark.annotation.DeveloperApi
    +import org.apache.spark.rdd.RDD
    +import org.apache.spark.sql.catalyst.expressions.{Expression, Attribute}
    +
    --- End diff --
    
    missing some imports here
    ```
    import org.apache.spark.sql.catalyst.expressions.{Row, Expression, Attribute}
    import org.apache.spark.sql.SQLContext
    import org.apache.spark.sql.catalyst.types.StructType
    ```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by liancheng <gi...@git.apache.org>.
Github user liancheng commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r19659690
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/sources/interfaces.scala ---
    @@ -0,0 +1,86 @@
    +/*
    +* Licensed to the Apache Software Foundation (ASF) under one or more
    +* contributor license agreements.  See the NOTICE file distributed with
    +* this work for additional information regarding copyright ownership.
    +* The ASF licenses this file to You under the Apache License, Version 2.0
    +* (the "License"); you may not use this file except in compliance with
    +* the License.  You may obtain a copy of the License at
    +*
    +*    http://www.apache.org/licenses/LICENSE-2.0
    +*
    +* Unless required by applicable law or agreed to in writing, software
    +* distributed under the License is distributed on an "AS IS" BASIS,
    +* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    +* See the License for the specific language governing permissions and
    +* limitations under the License.
    +*/
    +package org.apache.spark.sql.sources
    +
    +import org.apache.spark.annotation.DeveloperApi
    +import org.apache.spark.rdd.RDD
    +import org.apache.spark.sql.catalyst.expressions.{Expression, Attribute}
    +
    +/**
    + * Implemented by objects that produce relations for a specific kind of data source.  When
    + * Spark SQL is given a DDL operation with a USING clause specified, this interface is used to
    + * pass in the parameters specified by a user.
    + *
    + * Users may specify the fully qualified class name of a given data source.  When that class is
    + * not found Spark SQL will append the class name `DefaultSource` to the path, allowing for
    + * less verbose invocation.  For example, 'org.apache.spark.sql.json' would resolve to the
    + * data source 'org.apache.spark.sql.json.DefaultSource'
    + *
    + * A new instance of this class with be instantiated each time a DDL call is made.
    + */
    +@DeveloperApi
    +trait RelationProvider {
    +  /** Returns a new base relation with the given parameters. */
    +  def createRelation(sqlContext: SQLContext, parameters: Map[String, String]): BaseRelation
    +}
    +
    +/**
    + * Represents a collection of tuples with a known schema.  Classes that extend BaseRelation must
    + * be able to produce the schema of their data in the form of a [[StructType]]  In order to be
    + * executed, a BaseRelation must also mix in at least one of the Scan traits.
    + *
    + * BaseRelations must also define a equality function that only returns true when the two
    + * instances will return the same data.  This equality function is used when determining when
    + * it is safe to substitute cached results for a given relation.
    + */
    +@DeveloperApi
    +abstract class BaseRelation {
    +  def sqlContext: SQLContext
    +  def schema: StructType
    +}
    +
    +/**
    + * Mixed into a BaseRelation that can produce all of its tuples as an RDD of Row objects.
    --- End diff --
    
    Comment needs update, since it not a trait any more. The same to `PrunedScan` and `FilteredScan`.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r19648788
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/sources/package.scala ---
    @@ -0,0 +1,95 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.spark.sql
    +
    +import org.apache.spark.annotation.DeveloperApi
    +import org.apache.spark.rdd.RDD
    +import org.apache.spark.sql.catalyst.expressions.{Expression, Attribute}
    +
    +/**
    + * A set of APIs for adding data sources to Spark SQL.
    + */
    +package object sources {
    +
    +  /**
    +   * Implemented by objects that produce relations for a specific kind of data source.  When
    +   * Spark SQL is given a DDL operation with a USING clause specified, this interface is used to
    +   * pass in the parameters specified by a user.
    +   *
    +   * Users may specify the fully qualified class name of a given data source.  When that class is
    +   * not found Spark SQL will append the class name `DefaultSource` to the path, allowing for
    +   * less verbose invocation.  For example, 'org.apache.spark.sql.json' would resolve to the
    +   * data source 'org.apache.spark.sql.json.DefaultSource'
    +   */
    +  trait RelationProvider {
    +    /** Returns a new base relation with the given parameters. */
    +    def createRelation(sqlContext: SQLContext, parameters: Map[String, String]): BaseRelation
    +  }
    +
    +  /**
    +   * Represents a collection of tuples with a known schema.  Classes that extend BaseRelation must
    +   * be able to produce the schema of their data in the form of a [[StructType]]  In order to be
    +   * executed, a BaseRelation must also mix in at least one of the Scan traits.
    +   *
    +   * BaseRelations must also define a equality function that only returns true when the two
    +   * instances will return the same data.  This equality function is used when determining when
    +   * it is safe to substitute cached results for a given relation.
    +   */
    +  @DeveloperApi
    +  abstract class BaseRelation {
    +    def sqlContext: SQLContext
    +    def schema: StructType
    +  }
    +
    +  /**
    +   * Mixed into a BaseRelation that can produce all of its tuples as an RDD of Row objects.
    +   */
    +  @DeveloperApi
    +  trait TableScan {
    +    self: BaseRelation =>
    +
    +    def buildScan(): RDD[Row]
    --- End diff --
    
    i think we need more documentation on the semantics of the returned RDD. I don't know what we should say yet, but one thing to add is whether Row can be reused.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-61222343
  
      [Test build #22592 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/22592/consoleFull) for   PR 2475 at commit [`b069146`](https://github.com/apache/spark/commit/b06914625d709f0f1ad7a1be8ce6a2cff38644f1).
     * This patch **passes all tests**.
     * This patch merges cleanly.
     * This patch adds the following public classes _(experimental)_:
      * `case class StructField(name: String, dataType: DataType, nullable: Boolean = true) `
      * `trait RunnableCommand extends logical.Command `
      * `case class ExecutedCommand(cmd: RunnableCommand) extends SparkPlan `
      * `  protected case class Keyword(str: String)`
      * `            sys.error(s"Failed to load class for data source: $provider")`
      * `  trait RelationProvider `
      * `  abstract class BaseRelation `
      * `  abstract class TableScan extends BaseRelation `
      * `  abstract class PrunedScan extends BaseRelation `
      * `  abstract class FilteredScan extends BaseRelation `



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:

    https://github.com/apache/spark/pull/2475


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-61222348
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22592/
    Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-61198655
  
      [Test build #22581 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/22581/consoleFull) for   PR 2475 at commit [`34f836a`](https://github.com/apache/spark/commit/34f836ad799bc470a58c2603ddc4fc2b5fadc121).
     * This patch **fails Spark unit tests**.
     * This patch merges cleanly.
     * This patch adds the following public classes _(experimental)_:
      * `case class StructField(name: String, dataType: DataType, nullable: Boolean = true) `
      * `trait RunnableCommand extends logical.Command `
      * `case class ExecutedCommand(cmd: RunnableCommand) extends SparkPlan `
      * `  protected case class Keyword(str: String)`
      * `            sys.error(s"Failed to load class for data source: $provider")`
      * `  trait RelationProvider `
      * `  abstract class BaseRelation `
      * `  trait TableScan `
      * `  trait PrunedScan `
      * `  trait FilteredScan `



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-61195325
  
      [Test build #22581 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/22581/consoleFull) for   PR 2475 at commit [`34f836a`](https://github.com/apache/spark/commit/34f836ad799bc470a58c2603ddc4fc2b5fadc121).
     * This patch merges cleanly.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-61420018
  
    Jenkins, retest this please.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-61343404
  
      [Test build #22653 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/22653/consoleFull) for   PR 2475 at commit [`e3e690e`](https://github.com/apache/spark/commit/e3e690e28c31e1cf4852f7d484fb9fcdb7fa0ea1).
     * This patch **passes all tests**.
     * This patch merges cleanly.
     * This patch adds the following public classes _(experimental)_:
      * `abstract class GenericStrategy[PhysicalPlan <: TreeNode[PhysicalPlan]] extends Logging `
      * `case class StructField(name: String, dataType: DataType, nullable: Boolean = true) `
      * `trait RunnableCommand extends logical.Command `
      * `case class ExecutedCommand(cmd: RunnableCommand) extends SparkPlan `
      * `  protected case class Keyword(str: String)`
      * `            sys.error(s"Failed to load class for data source: $provider")`
      * `case class EqualTo(attribute: String, value: Any) extends Filter`
      * `trait RelationProvider `
      * `abstract class BaseRelation `
      * `abstract class TableScan extends BaseRelation `
      * `abstract class PrunedScan extends BaseRelation `
      * `abstract class PrunedFilteredScan extends BaseRelation `



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r19646027
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/sources/package.scala ---
    @@ -0,0 +1,95 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.spark.sql
    +
    +import org.apache.spark.annotation.DeveloperApi
    +import org.apache.spark.rdd.RDD
    +import org.apache.spark.sql.catalyst.expressions.{Expression, Attribute}
    +
    +/**
    + * A set of APIs for adding data sources to Spark SQL.
    + */
    +package object sources {
    +
    +  /**
    +   * Implemented by objects that produce relations for a specific kind of data source.  When
    +   * Spark SQL is given a DDL operation with a USING clause specified, this interface is used to
    +   * pass in the parameters specified by a user.
    +   *
    +   * Users may specify the fully qualified class name of a given data source.  When that class is
    +   * not found Spark SQL will append the class name `DefaultSource` to the path, allowing for
    +   * less verbose invocation.  For example, 'org.apache.spark.sql.json' would resolve to the
    +   * data source 'org.apache.spark.sql.json.DefaultSource'
    +   */
    +  trait RelationProvider {
    +    /** Returns a new base relation with the given parameters. */
    +    def createRelation(sqlContext: SQLContext, parameters: Map[String, String]): BaseRelation
    +  }
    +
    +  /**
    +   * Represents a collection of tuples with a known schema.  Classes that extend BaseRelation must
    +   * be able to produce the schema of their data in the form of a [[StructType]]  In order to be
    +   * executed, a BaseRelation must also mix in at least one of the Scan traits.
    +   *
    +   * BaseRelations must also define a equality function that only returns true when the two
    +   * instances will return the same data.  This equality function is used when determining when
    +   * it is safe to substitute cached results for a given relation.
    +   */
    +  @DeveloperApi
    +  abstract class BaseRelation {
    +    def sqlContext: SQLContext
    +    def schema: StructType
    +  }
    +
    +  /**
    +   * Mixed into a BaseRelation that can produce all of its tuples as an RDD of Row objects.
    +   */
    +  @DeveloperApi
    +  trait TableScan {
    +    self: BaseRelation =>
    --- End diff --
    
    The self type annotation is unnecessary I guess.  I'm not sure how that plays with binary compatibility, but I can remove it if you are concerned.
    
    The optimizer doesn't take advantage of this yet, but I could see cases where you might want to implement multiple of these (i.e. you cache the RDD when its a full table scan, but for column pruned cases you don't).  At least that is why I made them traits.  I guess this might be unnecessary.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r19645438
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/json/JSONRelation.scala ---
    @@ -0,0 +1,52 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +
    +package org.apache.spark.sql.json
    +
    +import org.apache.spark.sql.{SQLConf, SQLContext}
    +import org.apache.spark.sql.sources._
    +
    +private[sql] class DefaultSource extends RelationProvider {
    +  /** Returns a new base relation with the given parameters. */
    +  override def createRelation(
    +      sqlContext: SQLContext,
    +      parameters: Map[String, String]): BaseRelation = {
    +    val fileName =
    +      parameters.getOrElse("fileName", sys.error(s"Option 'fileName' not specified"))
    +
    +    val samplingRatio =
    --- End diff --
    
    also no need to line wrap


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by yhuai <gi...@git.apache.org>.
Github user yhuai commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r19687744
  
    --- Diff: sql/core/src/test/scala/org/apache/spark/sql/sources/FilteredScanSuite.scala ---
    @@ -0,0 +1,117 @@
    +/*
    +* Licensed to the Apache Software Foundation (ASF) under one or more
    +* contributor license agreements.  See the NOTICE file distributed with
    +* this work for additional information regarding copyright ownership.
    +* The ASF licenses this file to You under the Apache License, Version 2.0
    +* (the "License"); you may not use this file except in compliance with
    +* the License.  You may obtain a copy of the License at
    +*
    +*    http://www.apache.org/licenses/LICENSE-2.0
    +*
    +* Unless required by applicable law or agreed to in writing, software
    +* distributed under the License is distributed on an "AS IS" BASIS,
    +* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    +* See the License for the specific language governing permissions and
    +* limitations under the License.
    +*/
    +
    +package org.apache.spark.sql.sources
    +
    +import org.apache.spark.sql.catalyst.expressions.{Row => _, _}
    +import org.apache.spark.sql._
    +
    +class FilteredScanSource extends RelationProvider {
    +  override def createRelation(
    +      sqlContext: SQLContext,
    +      parameters: Map[String, String]): BaseRelation = {
    +    SimpleFilteredScan(parameters("from").toInt, parameters("to").toInt)(sqlContext)
    +  }
    +}
    +
    +case class SimpleFilteredScan(from: Int, to: Int)(@transient val sqlContext: SQLContext)
    +  extends FilteredScan {
    +
    +  override def schema =
    +    StructType(
    +      StructField("a", IntegerType, nullable = false) ::
    +      StructField("b", IntegerType, nullable = false) :: Nil)
    +
    +  override def buildScan(requiredColumns: Seq[Attribute], filters: Seq[Expression]) = {
    +    val rowBuilders = requiredColumns.map(_.name).map {
    +      case "a" => (i: Int) => Seq(i)
    +      case "b" => (i: Int) => Seq(i * 2)
    +    }
    +
    +    val filter = filters.collect {
    +      case Seq(EqualTo(a: AttributeReference, l: Literal)) if a.name == "a" =>
    +        (i: Int) => i == l.value
    +      case Seq(EqualTo(l: Literal, a: AttributeReference)) if a.name == "a" =>
    +        (i: Int) => i == l.value
    +    }.headOption.getOrElse((_: Int) => true)
    +
    +    sqlContext.sparkContext.parallelize(from to to).filter(filter).map(i =>
    +      Row.fromSeq(rowBuilders.map(_(i)).reduceOption(_ ++ _).getOrElse(Seq.empty)))
    +  }
    +}
    +
    +class FilteredScanSuite extends DataSourceTest {
    +
    +  import caseInsensisitiveContext._
    +
    +  before {
    +    sql(
    +      """
    +        |CREATE TEMPORARY TABLE oneToTenFiltered
    +        |USING org.apache.spark.sql.sources.FilteredScanSource
    +        |OPTIONS (
    +        |  from '1',
    +        |  to '10'
    +        |)
    +      """.stripMargin)
    +  }
    +
    +  sqlTest(
    +    "SELECT * FROM oneToTenFiltered",
    +    (1 to 10).map(i => Row(i, i * 2)).toSeq)
    +
    +  sqlTest(
    +    "SELECT a, b FROM oneToTenFiltered",
    +    (1 to 10).map(i => Row(i, i * 2)).toSeq)
    +
    +  sqlTest(
    +    "SELECT b, a FROM oneToTenFiltered",
    +    (1 to 10).map(i => Row(i * 2, i)).toSeq)
    +
    +  sqlTest(
    +    "SELECT a FROM oneToTenFiltered",
    +    (1 to 10).map(i => Row(i)).toSeq)
    +
    +  sqlTest(
    +    "SELECT b FROM oneToTenFiltered",
    +    (1 to 10).map(i => Row(i * 2)).toSeq)
    +
    +  sqlTest(
    +    "SELECT a * 2 FROM oneToTenFiltered",
    +    (1 to 10).map(i => Row(i * 2)).toSeq)
    +
    +  sqlTest(
    +    "SELECT A AS b FROM oneToTenFiltered",
    +    (1 to 10).map(i => Row(i)).toSeq)
    +
    +  sqlTest(
    +    "SELECT x.b, y.a FROM oneToTenFiltered x JOIN oneToTenFiltered y ON x.a = y.b",
    +    (1 to 5).map(i => Row(i * 4, i)).toSeq)
    +
    +  sqlTest(
    +    "SELECT x.a, y.b FROM oneToTenFiltered x JOIN oneToTenFiltered y ON x.a = y.b",
    +    (2 to 10 by 2).map(i => Row(i, i)).toSeq)
    +
    +  sqlTest(
    +    "SELECT * FROM oneToTenFiltered WHERE a = 1",
    +    Seq(1).map(i => Row(i, i * 2)).toSeq)
    --- End diff --
    
    For the completeness of the tests, should we also check the number of rows emitted by `PhysicalRDD`?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-61389566
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22713/
    Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [WIP][SPARK-3247][SQL] An API for adding forei...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-57355064
  
      [QA tests have finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/21045/consoleFull) for   PR 2475 at commit [`0e45dea`](https://github.com/apache/spark/commit/0e45deab3bf8815a5f6b06950c4bdf9083485793).
     * This patch **fails** unit tests.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by scwf <gi...@git.apache.org>.
Github user scwf commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r19654586
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/sources/interfaces.scala ---
    @@ -0,0 +1,86 @@
    +/*
    +* Licensed to the Apache Software Foundation (ASF) under one or more
    +* contributor license agreements.  See the NOTICE file distributed with
    +* this work for additional information regarding copyright ownership.
    +* The ASF licenses this file to You under the Apache License, Version 2.0
    +* (the "License"); you may not use this file except in compliance with
    +* the License.  You may obtain a copy of the License at
    +*
    +*    http://www.apache.org/licenses/LICENSE-2.0
    +*
    +* Unless required by applicable law or agreed to in writing, software
    +* distributed under the License is distributed on an "AS IS" BASIS,
    +* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    +* See the License for the specific language governing permissions and
    +* limitations under the License.
    +*/
    +package org.apache.spark.sql.sources
    +
    +import org.apache.spark.annotation.DeveloperApi
    +import org.apache.spark.rdd.RDD
    +import org.apache.spark.sql.catalyst.expressions.{Expression, Attribute}
    +
    +/**
    + * Implemented by objects that produce relations for a specific kind of data source.  When
    + * Spark SQL is given a DDL operation with a USING clause specified, this interface is used to
    + * pass in the parameters specified by a user.
    + *
    + * Users may specify the fully qualified class name of a given data source.  When that class is
    + * not found Spark SQL will append the class name `DefaultSource` to the path, allowing for
    + * less verbose invocation.  For example, 'org.apache.spark.sql.json' would resolve to the
    + * data source 'org.apache.spark.sql.json.DefaultSource'
    + *
    + * A new instance of this class with be instantiated each time a DDL call is made.
    + */
    +@DeveloperApi
    +trait RelationProvider {
    +  /** Returns a new base relation with the given parameters. */
    +  def createRelation(sqlContext: SQLContext, parameters: Map[String, String]): BaseRelation
    +}
    +
    +/**
    + * Represents a collection of tuples with a known schema.  Classes that extend BaseRelation must
    + * be able to produce the schema of their data in the form of a [[StructType]]  In order to be
    + * executed, a BaseRelation must also mix in at least one of the Scan traits.
    + *
    + * BaseRelations must also define a equality function that only returns true when the two
    + * instances will return the same data.  This equality function is used when determining when
    + * it is safe to substitute cached results for a given relation.
    + */
    +@DeveloperApi
    +abstract class BaseRelation {
    +  def sqlContext: SQLContext
    +  def schema: StructType
    +}
    +
    +/**
    + * Mixed into a BaseRelation that can produce all of its tuples as an RDD of Row objects.
    + */
    +@DeveloperApi
    +abstract class TableScan extends BaseRelation {
    +  def buildScan(): RDD[Row]
    +}
    +
    +/**
    + * Mixed into a BaseRelation that can eliminate unneeded columns before producing an RDD
    + * containing all of its tuples as Row objects.
    + */
    +@DeveloperApi
    +abstract class PrunedScan extends BaseRelation {
    +  def buildScan(requiredColumns: Seq[Attribute]): RDD[Row]
    +}
    +
    +/**
    + * Mixed into a BaseRelation that can eliminate unneeded columns and filter using selected
    + * predicates before producing an RDD containing all matching tuples as Row objects.
    + *
    + * The pushed down filters are currently purely an optimization as they will all be evaluated
    + * again.  This means it is safe to use them with methods that produce false positives such
    + * as filtering partitions based on a bloom filter.
    + */
    +@DeveloperApi
    +abstract class FilteredScan extends BaseRelation {
    +  def buildScan(
    +                 requiredColumns: Seq[Attribute],
    +                 filters: Seq[Expression]): RDD[Row]
    +}
    --- End diff --
    
    empty line and indentation off


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-61220321
  
    Test FAILed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22593/
    Test FAILed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [WIP][SPARK-3247][SQL] An API for adding forei...

Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r18122111
  
    --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/commands.scala ---
    @@ -79,4 +80,6 @@ case class DescribeCommand(
     /**
      * Returned for the "CACHE TABLE tableName AS SELECT .." command.
      */
    -case class CacheTableAsSelectCommand(tableName: String, plan: LogicalPlan) extends Command
    +case class CacheTableAsSelectCommand(tableName: String, plan: LogicalPlan) extends Command {
    +  val output = Nil
    --- End diff --
    
    override def?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [WIP][SPARK-3247][SQL] An API for adding forei...

Posted by yhuai <gi...@git.apache.org>.
Github user yhuai commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r18580229
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkStrategies.scala ---
    @@ -254,6 +254,8 @@ private[sql] abstract class SparkStrategies extends QueryPlanner[SparkPlan] {
         def numPartitions = self.numPartitions
     
         def apply(plan: LogicalPlan): Seq[SparkPlan] = plan match {
    +      case l @ foreign.LogicalRelation(t: foreign.TableScan) =>
    +        ExistingRdd(l.output, t.buildScan()) :: Nil
    --- End diff --
    
    Will we add rules for `PrunedScan` and `FilteredScan` in this PR?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [WIP][SPARK-3247][SQL] An API for adding forei...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-57354469
  
      [QA tests have started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/21045/consoleFull) for   PR 2475 at commit [`0e45dea`](https://github.com/apache/spark/commit/0e45deab3bf8815a5f6b06950c4bdf9083485793).
     * This patch merges cleanly.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by liancheng <gi...@git.apache.org>.
Github user liancheng commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-61228429
  
    Oh, I see, the last Jenkins build success was actually for the commit right before the last one.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by yhuai <gi...@git.apache.org>.
Github user yhuai commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r19685462
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/sources/LogicalRelation.scala ---
    @@ -0,0 +1,56 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.spark.sql.sources
    +
    +import org.apache.spark.sql.catalyst.analysis.MultiInstanceRelation
    +import org.apache.spark.sql.catalyst.expressions.{AttributeMap, Expression, Attribute}
    +import org.apache.spark.sql.catalyst.plans.logical.{Statistics, LogicalPlan}
    +import org.apache.spark.sql.catalyst.trees.LeafNode
    +
    +/**
    + * Used to link a [[BaseRelation]] in to a logical query plan.
    + */
    +private[sql] case class LogicalRelation(relation: BaseRelation)
    +  extends LogicalPlan
    +  with LeafNode[LogicalPlan]
    --- End diff --
    
    Should we extend `logical.LeafNode` instead of using `trees.LeafNode`?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r19646670
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/sources/package.scala ---
    @@ -0,0 +1,95 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.spark.sql
    +
    +import org.apache.spark.annotation.DeveloperApi
    +import org.apache.spark.rdd.RDD
    +import org.apache.spark.sql.catalyst.expressions.{Expression, Attribute}
    +
    +/**
    + * A set of APIs for adding data sources to Spark SQL.
    + */
    +package object sources {
    +
    +  /**
    +   * Implemented by objects that produce relations for a specific kind of data source.  When
    +   * Spark SQL is given a DDL operation with a USING clause specified, this interface is used to
    +   * pass in the parameters specified by a user.
    +   *
    +   * Users may specify the fully qualified class name of a given data source.  When that class is
    +   * not found Spark SQL will append the class name `DefaultSource` to the path, allowing for
    +   * less verbose invocation.  For example, 'org.apache.spark.sql.json' would resolve to the
    +   * data source 'org.apache.spark.sql.json.DefaultSource'
    +   */
    +  trait RelationProvider {
    +    /** Returns a new base relation with the given parameters. */
    +    def createRelation(sqlContext: SQLContext, parameters: Map[String, String]): BaseRelation
    +  }
    +
    +  /**
    +   * Represents a collection of tuples with a known schema.  Classes that extend BaseRelation must
    +   * be able to produce the schema of their data in the form of a [[StructType]]  In order to be
    +   * executed, a BaseRelation must also mix in at least one of the Scan traits.
    +   *
    +   * BaseRelations must also define a equality function that only returns true when the two
    +   * instances will return the same data.  This equality function is used when determining when
    +   * it is safe to substitute cached results for a given relation.
    +   */
    +  @DeveloperApi
    +  abstract class BaseRelation {
    +    def sqlContext: SQLContext
    +    def schema: StructType
    +  }
    +
    +  /**
    +   * Mixed into a BaseRelation that can produce all of its tuples as an RDD of Row objects.
    +   */
    +  @DeveloperApi
    +  trait TableScan {
    +    self: BaseRelation =>
    +
    +    def buildScan(): RDD[Row]
    +  }
    +
    +  /**
    +   * Mixed into a BaseRelation that can eliminate unneeded columns before producing an RDD
    +   * containing all of its tuples as Row objects.
    +   */
    +  @DeveloperApi
    +  trait PrunedScan {
    +    self: BaseRelation =>
    +
    +    def buildScan(requiredColumns: Seq[Attribute]): RDD[Row]
    +  }
    +
    +  /**
    +   * Mixed into a BaseRelation that can eliminate unneeded columns and filter using selected
    +   * predicates before producing an RDD containing all matching tuples as Row objects.
    +   *
    +   * The pushed down filters are currently purely an optimization as they will all be evaluated
    +   * again.  This means it is safe to use them with methods that produce false positives such
    +   * as filtering partitions based on a bloom filter.
    --- End diff --
    
    This used to be strict and let you select the filters you could handle, but then Reynold and I decided to simplify it.  It's pretty hard to design an interface that allows you to handle all three cases cleanly:
     - Supported filters
     - Unsupported filters
     - Advisory filters (used to optimize but not guaranteed to be evaluated)
    
    Since filter evaluation is pretty cheap (we double evaluate filters pushed down to parquet now) we decided to do this.
    
    What were you thinking the semantics of isStrict would be?  Do we still pass them all down?
    
    ```scala
    /** Returns true if all filters can be handled by the base source */
    def isStrict(filters: Seq[Expression]): Boolean
    ```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r19695119
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/sources/ddl.scala ---
    @@ -0,0 +1,110 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +
    +package org.apache.spark.sql.sources
    +
    +import org.apache.spark.Logging
    +import org.apache.spark.sql.SQLContext
    +import org.apache.spark.sql.execution.RunnableCommand
    +import org.apache.spark.util.Utils
    +
    +import scala.language.implicitConversions
    +import scala.util.parsing.combinator.lexical.StdLexical
    +import scala.util.parsing.combinator.syntactical.StandardTokenParsers
    +import scala.util.parsing.combinator.PackratParsers
    +
    +import org.apache.spark.sql.catalyst.plans.logical._
    +import org.apache.spark.sql.catalyst.SqlLexical
    +
    +/**
    + * A parser for foreign DDL commands.
    + */
    +private[sql] class DDLParser extends StandardTokenParsers with PackratParsers with Logging {
    +
    +  def apply(input: String): Option[LogicalPlan] = {
    +    phrase(ddl)(new lexical.Scanner(input)) match {
    +      case Success(r, x) => Some(r)
    +      case x =>
    +        logDebug(s"Not recognized as DDL: $x")
    +        None
    +    }
    +  }
    +
    +  protected case class Keyword(str: String)
    +
    +  protected implicit def asParser(k: Keyword): Parser[String] =
    +    lexical.allCaseVersions(k.str).map(x => x : Parser[String]).reduce(_ | _)
    +
    +  protected val CREATE = Keyword("CREATE")
    +  protected val TEMPORARY = Keyword("TEMPORARY")
    +  protected val TABLE = Keyword("TABLE")
    +  protected val USING = Keyword("USING")
    +  protected val OPTIONS = Keyword("OPTIONS")
    +
    +  // Use reflection to find the reserved words defined in this class.
    +  protected val reservedWords =
    +    this.getClass
    +      .getMethods
    +      .filter(_.getReturnType == classOf[Keyword])
    +      .map(_.invoke(this).asInstanceOf[Keyword].str)
    +
    +  override val lexical = new SqlLexical(reservedWords)
    +
    +  protected lazy val ddl: Parser[LogicalPlan] = createTable
    +
    +  /**
    +   * CREATE FOREIGN TEMPORARY TABLE avroTable
    +   * USING org.apache.spark.sql.avro
    +   * OPTIONS (path "../hive/src/test/resources/data/files/episodes.avro")
    +   */
    +  protected lazy val createTable: Parser[LogicalPlan] =
    +    CREATE ~> TEMPORARY ~> TABLE ~> ident ~
    +    USING ~ className ~
    +    OPTIONS ~ options ^^ {
    +      case tableName ~ _ ~ provider ~ _ ~ opts =>
    +        CreateTableUsing(tableName, provider, opts)
    +    }
    +
    +  protected lazy val options: Parser[Map[String, String]] =
    +  "(" ~> repsep(pair, ",") <~ ")" ^^ { case s: Seq[(String, String)] => s.toMap }
    +
    +  protected lazy val className: Parser[String] = repsep(ident, ".") ^^ { case s => s.mkString(".")}
    +
    +  protected lazy val pair: Parser[(String, String)] = ident ~ stringLit ^^ { case k ~ v => (k,v) }
    +}
    +
    +private[sql] case class CreateTableUsing(
    +    tableName: String,
    +    provider: String,
    +    options: Map[String, String]) extends RunnableCommand {
    +
    +  def run(sqlContext: SQLContext) = {
    +    val loader = Utils.getContextOrSparkClassLoader
    --- End diff --
    
    fixed


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [WIP][SPARK-3247][SQL] An API for adding forei...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-56285964
  
      [QA tests have finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/20615/consoleFull) for   PR 2475 at commit [`47d542c`](https://github.com/apache/spark/commit/47d542cc0238fba04b6c4e4456393d812d559c4e).
     * This patch **fails** unit tests.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [WIP][SPARK-3247][SQL] An API for adding forei...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-56484980
  
      [QA tests have finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/20692/consoleFull) for   PR 2475 at commit [`c249907`](https://github.com/apache/spark/commit/c249907cf50bcb9f0fed189ef6954558c5efbe12).
     * This patch **fails** unit tests.
     * This patch merges cleanly.
     * This patch adds the following public classes _(experimental)_:
      * `case class CacheCommand(tableName: String, doCache: Boolean) extends Command `
      * `case class CacheTableAsSelectCommand(tableName: String, plan: LogicalPlan) extends Command `
      * `case class StructField(name: String, dataType: DataType, nullable: Boolean = true) `
      * `  implicit class AvroContext(sqlContext: SQLContext) `
      * `  case class AvroRelation(location: String)(val sqlContext: SQLContext)`
      * `trait RunnableCommand extends logical.Command `
      * `case class ExecutedCommand(cmd: RunnableCommand) extends SparkPlan `
      * `class DDLParser extends StandardTokenParsers with PackratParsers with Logging `
      * `  protected case class Keyword(str: String)`
      * `case class CreateForeignTable(`
      * `trait DataSource `
      * `  abstract class BaseRelation `
      * `  trait TableScan `
      * `  trait PrunedScan `
      * `  trait FilteredScan `
      * `  case class LogicalRelation(relation: BaseRelation)`



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by mateiz <gi...@git.apache.org>.
Github user mateiz commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r19645705
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/sources/package.scala ---
    @@ -0,0 +1,95 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.spark.sql
    +
    +import org.apache.spark.annotation.DeveloperApi
    +import org.apache.spark.rdd.RDD
    +import org.apache.spark.sql.catalyst.expressions.{Expression, Attribute}
    +
    +/**
    + * A set of APIs for adding data sources to Spark SQL.
    + */
    +package object sources {
    +
    +  /**
    +   * Implemented by objects that produce relations for a specific kind of data source.  When
    +   * Spark SQL is given a DDL operation with a USING clause specified, this interface is used to
    +   * pass in the parameters specified by a user.
    +   *
    +   * Users may specify the fully qualified class name of a given data source.  When that class is
    +   * not found Spark SQL will append the class name `DefaultSource` to the path, allowing for
    +   * less verbose invocation.  For example, 'org.apache.spark.sql.json' would resolve to the
    +   * data source 'org.apache.spark.sql.json.DefaultSource'
    +   */
    +  trait RelationProvider {
    --- End diff --
    
    You should document that a new instance of this is created for each create table statement. Unless you didn't intend that, but in that case you'd need to keep these objects in a map somewhere, which is annoying.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r19648604
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/sources/package.scala ---
    @@ -0,0 +1,95 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.spark.sql
    +
    +import org.apache.spark.annotation.DeveloperApi
    +import org.apache.spark.rdd.RDD
    +import org.apache.spark.sql.catalyst.expressions.{Expression, Attribute}
    +
    +/**
    + * A set of APIs for adding data sources to Spark SQL.
    + */
    +package object sources {
    +
    +  /**
    +   * Implemented by objects that produce relations for a specific kind of data source.  When
    +   * Spark SQL is given a DDL operation with a USING clause specified, this interface is used to
    +   * pass in the parameters specified by a user.
    +   *
    +   * Users may specify the fully qualified class name of a given data source.  When that class is
    +   * not found Spark SQL will append the class name `DefaultSource` to the path, allowing for
    +   * less verbose invocation.  For example, 'org.apache.spark.sql.json' would resolve to the
    +   * data source 'org.apache.spark.sql.json.DefaultSource'
    +   */
    +  trait RelationProvider {
    +    /** Returns a new base relation with the given parameters. */
    +    def createRelation(sqlContext: SQLContext, parameters: Map[String, String]): BaseRelation
    +  }
    +
    +  /**
    +   * Represents a collection of tuples with a known schema.  Classes that extend BaseRelation must
    +   * be able to produce the schema of their data in the form of a [[StructType]]  In order to be
    +   * executed, a BaseRelation must also mix in at least one of the Scan traits.
    +   *
    +   * BaseRelations must also define a equality function that only returns true when the two
    +   * instances will return the same data.  This equality function is used when determining when
    +   * it is safe to substitute cached results for a given relation.
    +   */
    +  @DeveloperApi
    +  abstract class BaseRelation {
    +    def sqlContext: SQLContext
    +    def schema: StructType
    +  }
    +
    +  /**
    +   * Mixed into a BaseRelation that can produce all of its tuples as an RDD of Row objects.
    +   */
    +  @DeveloperApi
    +  trait TableScan {
    +    self: BaseRelation =>
    +
    +    def buildScan(): RDD[Row]
    +  }
    +
    +  /**
    +   * Mixed into a BaseRelation that can eliminate unneeded columns before producing an RDD
    +   * containing all of its tuples as Row objects.
    +   */
    +  @DeveloperApi
    +  trait PrunedScan {
    +    self: BaseRelation =>
    +
    +    def buildScan(requiredColumns: Seq[Attribute]): RDD[Row]
    +  }
    +
    +  /**
    +   * Mixed into a BaseRelation that can eliminate unneeded columns and filter using selected
    +   * predicates before producing an RDD containing all matching tuples as Row objects.
    +   *
    +   * The pushed down filters are currently purely an optimization as they will all be evaluated
    +   * again.  This means it is safe to use them with methods that produce false positives such
    +   * as filtering partitions based on a bloom filter.
    --- End diff --
    
    We could also add the following (now or down the road), which preserves the current simple semantics and allows for optimization.
    
    ```scala
    abstract class FilterType
    case object SupportedFilter extends FilterType
    case object AdvisoryFilter extends FilterType
    case object UnsupportedFilter extends FilterType
    
    def checkFilter(filter: Seq[Expression): FilterType = AdvisoryFilter
    ```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-61420077
  
      [Test build #22765 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/22765/consoleFull) for   PR 2475 at commit [`1ed6010`](https://github.com/apache/spark/commit/1ed60106b05bd39d5a29fa0e1ac8f98c19d2412a).
     * This patch merges cleanly.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [WIP][SPARK-3247][SQL] An API for adding forei...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-61037005
  
      [Test build #22509 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/22509/consoleFull) for   PR 2475 at commit [`0fd3a07`](https://github.com/apache/spark/commit/0fd3a07a29a58e763b6e0a0ad26ca860efdbb979).
     * This patch merges cleanly.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by scwf <gi...@git.apache.org>.
Github user scwf commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r19654020
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/sources/ddl.scala ---
    @@ -0,0 +1,110 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +
    +package org.apache.spark.sql.sources
    +
    +import org.apache.spark.Logging
    +import org.apache.spark.sql.SQLContext
    +import org.apache.spark.sql.execution.RunnableCommand
    +import org.apache.spark.util.Utils
    +
    +import scala.language.implicitConversions
    +import scala.util.parsing.combinator.lexical.StdLexical
    +import scala.util.parsing.combinator.syntactical.StandardTokenParsers
    +import scala.util.parsing.combinator.PackratParsers
    +
    +import org.apache.spark.sql.catalyst.plans.logical._
    +import org.apache.spark.sql.catalyst.SqlLexical
    +
    +/**
    + * A parser for foreign DDL commands.
    + */
    +private[sql] class DDLParser extends StandardTokenParsers with PackratParsers with Logging {
    +
    +  def apply(input: String): Option[LogicalPlan] = {
    +    phrase(ddl)(new lexical.Scanner(input)) match {
    +      case Success(r, x) => Some(r)
    +      case x =>
    +        logDebug(s"Not recognized as DDL: $x")
    +        None
    +    }
    +  }
    +
    +  protected case class Keyword(str: String)
    +
    +  protected implicit def asParser(k: Keyword): Parser[String] =
    +    lexical.allCaseVersions(k.str).map(x => x : Parser[String]).reduce(_ | _)
    +
    +  protected val CREATE = Keyword("CREATE")
    +  protected val TEMPORARY = Keyword("TEMPORARY")
    +  protected val TABLE = Keyword("TABLE")
    +  protected val USING = Keyword("USING")
    +  protected val OPTIONS = Keyword("OPTIONS")
    +
    +  // Use reflection to find the reserved words defined in this class.
    +  protected val reservedWords =
    +    this.getClass
    +      .getMethods
    +      .filter(_.getReturnType == classOf[Keyword])
    +      .map(_.invoke(this).asInstanceOf[Keyword].str)
    +
    +  override val lexical = new SqlLexical(reservedWords)
    +
    +  protected lazy val ddl: Parser[LogicalPlan] = createTable
    +
    +  /**
    +   * CREATE FOREIGN TEMPORARY TABLE avroTable
    +   * USING org.apache.spark.sql.avro
    +   * OPTIONS (path "../hive/src/test/resources/data/files/episodes.avro")
    +   */
    +  protected lazy val createTable: Parser[LogicalPlan] =
    +    CREATE ~> TEMPORARY ~> TABLE ~> ident ~
    +    USING ~ className ~
    +    OPTIONS ~ options ^^ {
    +      case tableName ~ _ ~ provider ~ _ ~ opts =>
    +        CreateTableUsing(tableName, provider, opts)
    +    }
    +
    +  protected lazy val options: Parser[Map[String, String]] =
    +  "(" ~> repsep(pair, ",") <~ ")" ^^ { case s: Seq[(String, String)] => s.toMap }
    --- End diff --
    
    2 spaces missed?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [WIP][SPARK-3247][SQL] An API for adding forei...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-56484529
  
      [QA tests have started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/20692/consoleFull) for   PR 2475 at commit [`c249907`](https://github.com/apache/spark/commit/c249907cf50bcb9f0fed189ef6954558c5efbe12).
     * This patch merges cleanly.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [WIP][SPARK-3247][SQL] An API for adding forei...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-58130997
  
      [QA tests have finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/21361/consoleFull) for   PR 2475 at commit [`b818ee8`](https://github.com/apache/spark/commit/b818ee82a9e0d87c625b6fee1009ad4195708afa).
     * This patch **fails** unit tests.
     * This patch **does not** merge cleanly!



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [WIP][SPARK-3247][SQL] An API for adding forei...

Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-57043489
  
    Moving some of the existing data sources (parquet or json) into using this api would be a great way to test this api's design.
    
    @yhuai


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by mateiz <gi...@git.apache.org>.
Github user mateiz commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r19645761
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/sources/package.scala ---
    @@ -0,0 +1,95 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.spark.sql
    +
    +import org.apache.spark.annotation.DeveloperApi
    +import org.apache.spark.rdd.RDD
    +import org.apache.spark.sql.catalyst.expressions.{Expression, Attribute}
    +
    +/**
    + * A set of APIs for adding data sources to Spark SQL.
    + */
    +package object sources {
    +
    +  /**
    +   * Implemented by objects that produce relations for a specific kind of data source.  When
    +   * Spark SQL is given a DDL operation with a USING clause specified, this interface is used to
    +   * pass in the parameters specified by a user.
    +   *
    +   * Users may specify the fully qualified class name of a given data source.  When that class is
    +   * not found Spark SQL will append the class name `DefaultSource` to the path, allowing for
    +   * less verbose invocation.  For example, 'org.apache.spark.sql.json' would resolve to the
    +   * data source 'org.apache.spark.sql.json.DefaultSource'
    +   */
    +  trait RelationProvider {
    --- End diff --
    
    Additionally this should probably also be `@DeveloperApi` like the other stuff.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-61219607
  
      [Test build #22592 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/22592/consoleFull) for   PR 2475 at commit [`b069146`](https://github.com/apache/spark/commit/b06914625d709f0f1ad7a1be8ce6a2cff38644f1).
     * This patch merges cleanly.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by yhuai <gi...@git.apache.org>.
Github user yhuai commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r19686480
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/sources/ddl.scala ---
    @@ -0,0 +1,110 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +
    +package org.apache.spark.sql.sources
    +
    +import org.apache.spark.Logging
    +import org.apache.spark.sql.SQLContext
    +import org.apache.spark.sql.execution.RunnableCommand
    +import org.apache.spark.util.Utils
    +
    +import scala.language.implicitConversions
    +import scala.util.parsing.combinator.lexical.StdLexical
    +import scala.util.parsing.combinator.syntactical.StandardTokenParsers
    +import scala.util.parsing.combinator.PackratParsers
    +
    +import org.apache.spark.sql.catalyst.plans.logical._
    +import org.apache.spark.sql.catalyst.SqlLexical
    +
    +/**
    + * A parser for foreign DDL commands.
    + */
    +private[sql] class DDLParser extends StandardTokenParsers with PackratParsers with Logging {
    +
    +  def apply(input: String): Option[LogicalPlan] = {
    +    phrase(ddl)(new lexical.Scanner(input)) match {
    +      case Success(r, x) => Some(r)
    +      case x =>
    +        logDebug(s"Not recognized as DDL: $x")
    +        None
    --- End diff --
    
    I think we need to add some logics to generate error messages for invalid DDL statements in future.
    Right now, if a user writes an invalid DDL statement, the error message will be generated and thrown by the SQL parser. For example, for the following invalid DDL statement,
    ```
    CREATE TEMPORARY TABLE jsonTableSQL
    USING org.apache.spark.sql.json
    a string causing parsing error
    OPTIONS (
      fileName '/home/michael/data.json'
    )
    ```
    the error message is 
    ```
    java.lang.RuntimeException: [2.1] failure: ``INSERT'' expected but identifier create found
    ```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by mateiz <gi...@git.apache.org>.
Github user mateiz commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r19645938
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/sources/package.scala ---
    @@ -0,0 +1,95 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.spark.sql
    +
    +import org.apache.spark.annotation.DeveloperApi
    +import org.apache.spark.rdd.RDD
    +import org.apache.spark.sql.catalyst.expressions.{Expression, Attribute}
    +
    +/**
    + * A set of APIs for adding data sources to Spark SQL.
    + */
    +package object sources {
    +
    +  /**
    +   * Implemented by objects that produce relations for a specific kind of data source.  When
    +   * Spark SQL is given a DDL operation with a USING clause specified, this interface is used to
    +   * pass in the parameters specified by a user.
    +   *
    +   * Users may specify the fully qualified class name of a given data source.  When that class is
    +   * not found Spark SQL will append the class name `DefaultSource` to the path, allowing for
    +   * less verbose invocation.  For example, 'org.apache.spark.sql.json' would resolve to the
    +   * data source 'org.apache.spark.sql.json.DefaultSource'
    +   */
    +  trait RelationProvider {
    +    /** Returns a new base relation with the given parameters. */
    +    def createRelation(sqlContext: SQLContext, parameters: Map[String, String]): BaseRelation
    +  }
    +
    +  /**
    +   * Represents a collection of tuples with a known schema.  Classes that extend BaseRelation must
    +   * be able to produce the schema of their data in the form of a [[StructType]]  In order to be
    +   * executed, a BaseRelation must also mix in at least one of the Scan traits.
    +   *
    +   * BaseRelations must also define a equality function that only returns true when the two
    +   * instances will return the same data.  This equality function is used when determining when
    +   * it is safe to substitute cached results for a given relation.
    +   */
    +  @DeveloperApi
    +  abstract class BaseRelation {
    +    def sqlContext: SQLContext
    +    def schema: StructType
    +  }
    +
    +  /**
    +   * Mixed into a BaseRelation that can produce all of its tuples as an RDD of Row objects.
    +   */
    +  @DeveloperApi
    +  trait TableScan {
    +    self: BaseRelation =>
    +
    +    def buildScan(): RDD[Row]
    +  }
    +
    +  /**
    +   * Mixed into a BaseRelation that can eliminate unneeded columns before producing an RDD
    +   * containing all of its tuples as Row objects.
    +   */
    +  @DeveloperApi
    +  trait PrunedScan {
    +    self: BaseRelation =>
    +
    +    def buildScan(requiredColumns: Seq[Attribute]): RDD[Row]
    +  }
    +
    +  /**
    +   * Mixed into a BaseRelation that can eliminate unneeded columns and filter using selected
    +   * predicates before producing an RDD containing all matching tuples as Row objects.
    +   *
    +   * The pushed down filters are currently purely an optimization as they will all be evaluated
    +   * again.  This means it is safe to use them with methods that produce false positives such
    +   * as filtering partitions based on a bloom filter.
    --- End diff --
    
    We should add an option for FilteredScans to guarantee that they've applied all the filters and avoid this checking. Maybe there can be a method in the class like isStrict. Otherwise we can't change the semantics here in the future (I guess we could create another class).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by liancheng <gi...@git.apache.org>.
Github user liancheng commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r19703203
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/sources/interfaces.scala ---
    @@ -0,0 +1,86 @@
    +/*
    +* Licensed to the Apache Software Foundation (ASF) under one or more
    +* contributor license agreements.  See the NOTICE file distributed with
    +* this work for additional information regarding copyright ownership.
    +* The ASF licenses this file to You under the Apache License, Version 2.0
    +* (the "License"); you may not use this file except in compliance with
    +* the License.  You may obtain a copy of the License at
    +*
    +*    http://www.apache.org/licenses/LICENSE-2.0
    +*
    +* Unless required by applicable law or agreed to in writing, software
    +* distributed under the License is distributed on an "AS IS" BASIS,
    +* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    +* See the License for the specific language governing permissions and
    +* limitations under the License.
    +*/
    +package org.apache.spark.sql.sources
    +
    +import org.apache.spark.annotation.DeveloperApi
    +import org.apache.spark.rdd.RDD
    +import org.apache.spark.sql.catalyst.expressions.{Expression, Attribute}
    +
    +/**
    + * Implemented by objects that produce relations for a specific kind of data source.  When
    + * Spark SQL is given a DDL operation with a USING clause specified, this interface is used to
    + * pass in the parameters specified by a user.
    + *
    + * Users may specify the fully qualified class name of a given data source.  When that class is
    + * not found Spark SQL will append the class name `DefaultSource` to the path, allowing for
    + * less verbose invocation.  For example, 'org.apache.spark.sql.json' would resolve to the
    + * data source 'org.apache.spark.sql.json.DefaultSource'
    + *
    + * A new instance of this class with be instantiated each time a DDL call is made.
    + */
    +@DeveloperApi
    +trait RelationProvider {
    +  /** Returns a new base relation with the given parameters. */
    +  def createRelation(sqlContext: SQLContext, parameters: Map[String, String]): BaseRelation
    +}
    +
    +/**
    + * Represents a collection of tuples with a known schema.  Classes that extend BaseRelation must
    + * be able to produce the schema of their data in the form of a [[StructType]]  In order to be
    + * executed, a BaseRelation must also mix in at least one of the Scan traits.
    + *
    + * BaseRelations must also define a equality function that only returns true when the two
    + * instances will return the same data.  This equality function is used when determining when
    + * it is safe to substitute cached results for a given relation.
    --- End diff --
    
    The `sameResult` I mentioned above receives a `BaseRelation` rather than a `LogicalPlan`. But anyway I see your point that an equality function is sufficient in this case.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-61193556
  
      [Test build #22577 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/22577/consoleFull) for   PR 2475 at commit [`de3b68c`](https://github.com/apache/spark/commit/de3b68c4d28c7cf603c886b99476a2510ce307e8).
     * This patch merges cleanly.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r19684904
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/sources/LogicalRelation.scala ---
    @@ -0,0 +1,56 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.spark.sql.sources
    +
    +import org.apache.spark.sql.catalyst.analysis.MultiInstanceRelation
    +import org.apache.spark.sql.catalyst.expressions.{AttributeMap, Expression, Attribute}
    +import org.apache.spark.sql.catalyst.plans.logical.{Statistics, LogicalPlan}
    +import org.apache.spark.sql.catalyst.trees.LeafNode
    +
    +/**
    + * Used to link a [[BaseRelation]] in to a logical query plan.
    + */
    +private[sql] case class LogicalRelation(relation: BaseRelation)
    +  extends LogicalPlan
    +  with LeafNode[LogicalPlan]
    --- End diff --
    
    BaseRelation is not a TreeNode, so yes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r19702317
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/sources/filters.scala ---
    @@ -0,0 +1,22 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +
    +package org.apache.spark.sql.sources
    +
    +abstract sealed class Filter
    +
    +case class EqualTo(attribute: String, value: Any) extends Filter
    --- End diff --
    
    +1 on this change to decouple it from the expression hierarchy. 
    
    presumably we would want > and < as well?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-61334847
  
      [Test build #22641 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/22641/consoleFull) for   PR 2475 at commit [`70da6d9`](https://github.com/apache/spark/commit/70da6d941b222385aa437bf7bfe6cabddad09cf8).
     * This patch merges cleanly.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r19646282
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/sources/package.scala ---
    @@ -0,0 +1,95 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.spark.sql
    +
    +import org.apache.spark.annotation.DeveloperApi
    +import org.apache.spark.rdd.RDD
    +import org.apache.spark.sql.catalyst.expressions.{Expression, Attribute}
    +
    +/**
    + * A set of APIs for adding data sources to Spark SQL.
    + */
    +package object sources {
    +
    +  /**
    +   * Implemented by objects that produce relations for a specific kind of data source.  When
    +   * Spark SQL is given a DDL operation with a USING clause specified, this interface is used to
    +   * pass in the parameters specified by a user.
    +   *
    +   * Users may specify the fully qualified class name of a given data source.  When that class is
    +   * not found Spark SQL will append the class name `DefaultSource` to the path, allowing for
    +   * less verbose invocation.  For example, 'org.apache.spark.sql.json' would resolve to the
    +   * data source 'org.apache.spark.sql.json.DefaultSource'
    +   */
    +  trait RelationProvider {
    +    /** Returns a new base relation with the given parameters. */
    +    def createRelation(sqlContext: SQLContext, parameters: Map[String, String]): BaseRelation
    +  }
    +
    +  /**
    +   * Represents a collection of tuples with a known schema.  Classes that extend BaseRelation must
    +   * be able to produce the schema of their data in the form of a [[StructType]]  In order to be
    +   * executed, a BaseRelation must also mix in at least one of the Scan traits.
    +   *
    +   * BaseRelations must also define a equality function that only returns true when the two
    +   * instances will return the same data.  This equality function is used when determining when
    +   * it is safe to substitute cached results for a given relation.
    +   */
    +  @DeveloperApi
    +  abstract class BaseRelation {
    +    def sqlContext: SQLContext
    +    def schema: StructType
    +  }
    +
    +  /**
    +   * Mixed into a BaseRelation that can produce all of its tuples as an RDD of Row objects.
    +   */
    +  @DeveloperApi
    +  trait TableScan {
    +    self: BaseRelation =>
    +
    +    def buildScan(): RDD[Row]
    +  }
    +
    +  /**
    +   * Mixed into a BaseRelation that can eliminate unneeded columns before producing an RDD
    +   * containing all of its tuples as Row objects.
    +   */
    +  @DeveloperApi
    +  trait PrunedScan {
    +    self: BaseRelation =>
    +
    +    def buildScan(requiredColumns: Seq[Attribute]): RDD[Row]
    --- End diff --
    
    I guess it could be limited to `AttributeReference` at this point in query execution.  `Attribute` is just the generic version that can also be unresolved.  From a usage standpoint they have essentially the same interfaces so all internal APIs are typed as `Attribute`
    
    I considered using field names instead of `Attribute`, but that seems a little weird with `Expression` being exposed below.  This does not support nested data access as that is represented by `GetField` expressions. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r19646994
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/sources/LogicalRelation.scala ---
    @@ -0,0 +1,56 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.spark.sql.sources
    +
    +import org.apache.spark.sql.catalyst.analysis.MultiInstanceRelation
    +import org.apache.spark.sql.catalyst.expressions.{AttributeMap, Expression, Attribute}
    +import org.apache.spark.sql.catalyst.plans.logical.{Statistics, LogicalPlan}
    +import org.apache.spark.sql.catalyst.trees.LeafNode
    +
    +/**
    + * Used to link a [[BaseRelation]] in to a logical query plan.
    + */
    +private[sql] case class LogicalRelation(relation: BaseRelation)
    +  extends LogicalPlan
    +  with LeafNode[LogicalPlan]
    +  with MultiInstanceRelation {
    +
    +  val output = relation.schema.toAttributes
    --- End diff --
    
    override?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-61196797
  
    @yhuai not all datasets will be path based.  Consider the case of a JDBC based data source.  We should be consistent it naming though,  I'll change avroFile to match.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-61393229
  
      [Test build #22731 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/22731/consoleFull) for   PR 2475 at commit [`1ed6010`](https://github.com/apache/spark/commit/1ed60106b05bd39d5a29fa0e1ac8f98c19d2412a).
     * This patch **passes all tests**.
     * This patch merges cleanly.
     * This patch adds the following public classes _(experimental)_:
      * `class DecimalType(DataType):`
      * `case class UnscaledValue(child: Expression) extends UnaryExpression `
      * `case class MakeDecimal(child: Expression, precision: Int, scale: Int) extends UnaryExpression `
      * `case class MutableLiteral(var value: Any, dataType: DataType, nullable: Boolean = true)`
      * `abstract class GenericStrategy[PhysicalPlan <: TreeNode[PhysicalPlan]] extends Logging `
      * `case class PrecisionInfo(precision: Int, scale: Int)`
      * `case class DecimalType(precisionInfo: Option[PrecisionInfo]) extends FractionalType `
      * `final class Decimal extends Ordered[Decimal] with Serializable `
      * `  trait DecimalIsConflicted extends Numeric[Decimal] `
      * `trait RunnableCommand extends logical.Command `
      * `case class ExecutedCommand(cmd: RunnableCommand) extends SparkPlan `
      * `  protected case class Keyword(str: String)`
      * `            sys.error(s"Failed to load class for data source: $provider")`
      * `case class EqualTo(attribute: String, value: Any) extends Filter`
      * `case class GreaterThan(attribute: String, value: Any) extends Filter`
      * `case class GreaterThanOrEqual(attribute: String, value: Any) extends Filter`
      * `case class LessThan(attribute: String, value: Any) extends Filter`
      * `case class LessThanOrEqual(attribute: String, value: Any) extends Filter`
      * `trait RelationProvider `
      * `abstract class BaseRelation `
      * `abstract class TableScan extends BaseRelation `
      * `abstract class PrunedScan extends BaseRelation `
      * `abstract class PrunedFilteredScan extends BaseRelation `



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by yhuai <gi...@git.apache.org>.
Github user yhuai commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-61195407
  
    Since we probably always need to specify the location of a data set, do we want to define a keyword instead of allowing `RelationProvider`s using different keys? Seems JSON relation uses `fileName` as the key and avro relation uses `path`.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [WIP][SPARK-3247][SQL] An API for adding forei...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-58130788
  
      [QA tests have started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/21361/consoleFull) for   PR 2475 at commit [`b818ee8`](https://github.com/apache/spark/commit/b818ee82a9e0d87c625b6fee1009ad4195708afa).
     * This patch **does not** merge cleanly!


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r19648912
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/sources/package.scala ---
    @@ -0,0 +1,95 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.spark.sql
    +
    +import org.apache.spark.annotation.DeveloperApi
    +import org.apache.spark.rdd.RDD
    +import org.apache.spark.sql.catalyst.expressions.{Expression, Attribute}
    +
    +/**
    + * A set of APIs for adding data sources to Spark SQL.
    + */
    +package object sources {
    +
    +  /**
    +   * Implemented by objects that produce relations for a specific kind of data source.  When
    +   * Spark SQL is given a DDL operation with a USING clause specified, this interface is used to
    +   * pass in the parameters specified by a user.
    +   *
    +   * Users may specify the fully qualified class name of a given data source.  When that class is
    +   * not found Spark SQL will append the class name `DefaultSource` to the path, allowing for
    +   * less verbose invocation.  For example, 'org.apache.spark.sql.json' would resolve to the
    +   * data source 'org.apache.spark.sql.json.DefaultSource'
    +   */
    +  trait RelationProvider {
    +    /** Returns a new base relation with the given parameters. */
    +    def createRelation(sqlContext: SQLContext, parameters: Map[String, String]): BaseRelation
    +  }
    +
    +  /**
    +   * Represents a collection of tuples with a known schema.  Classes that extend BaseRelation must
    +   * be able to produce the schema of their data in the form of a [[StructType]]  In order to be
    +   * executed, a BaseRelation must also mix in at least one of the Scan traits.
    +   *
    +   * BaseRelations must also define a equality function that only returns true when the two
    +   * instances will return the same data.  This equality function is used when determining when
    +   * it is safe to substitute cached results for a given relation.
    +   */
    +  @DeveloperApi
    +  abstract class BaseRelation {
    +    def sqlContext: SQLContext
    +    def schema: StructType
    +  }
    +
    +  /**
    +   * Mixed into a BaseRelation that can produce all of its tuples as an RDD of Row objects.
    +   */
    +  @DeveloperApi
    +  trait TableScan {
    +    self: BaseRelation =>
    +
    +    def buildScan(): RDD[Row]
    --- End diff --
    
    It would be safe to reuse the `Row`, but it seems a little vacuous to state that when there is no public API to mutate a row.  Should we expose that too? I'm a little hesitant given that it been changing quite a bit recently.
    
    It is always safe to state this down the road.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [WIP][SPARK-3247][SQL] An API for adding forei...

Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r18122129
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/avro.scala ---
    @@ -0,0 +1,110 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.spark.sql
    +
    +import org.apache.avro.Schema
    +import org.apache.avro.file.DataFileReader
    +import org.apache.avro.generic.{GenericRecord, GenericDatumReader}
    +import org.apache.avro.mapred.FsInput
    +
    +import org.apache.hadoop.fs.Path
    +
    +import org.apache.spark.sql.foreign._
    +import org.apache.spark.sql.test.TestSQLContext
    +
    +import scala.collection.JavaConversions._
    +
    +/**
    + * An example of a Spark SQL foreign data source for reading data stored in Avro.  Currently this
    + * is only for illustrative purposes and much of the data type conversion logic still needs to be
    + * written.
    + */
    +package object avro extends DataSource {
    +
    +  /**
    +   * Creates a new relation for data store in avro given a `path` as a parameter.
    +   */
    +  def createRelation(sqlContext: SQLContext, parameters: Map[String, String]) = {
    --- End diff --
    
    override


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [WIP][SPARK-3247][SQL] An API for adding forei...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-56484984
  
    Test FAILed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/20692/


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [WIP][SPARK-3247][SQL] An API for adding forei...

Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r18122112
  
    --- Diff: sql/core/pom.xml ---
    @@ -36,6 +36,16 @@
       </properties>
     
       <dependencies>
    +      <dependency>
    --- End diff --
    
    indent is off here


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-61198661
  
    Test FAILed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22581/
    Test FAILed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by yhuai <gi...@git.apache.org>.
Github user yhuai commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r19684775
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/sources/LogicalRelation.scala ---
    @@ -0,0 +1,56 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.spark.sql.sources
    +
    +import org.apache.spark.sql.catalyst.analysis.MultiInstanceRelation
    +import org.apache.spark.sql.catalyst.expressions.{AttributeMap, Expression, Attribute}
    +import org.apache.spark.sql.catalyst.plans.logical.{Statistics, LogicalPlan}
    +import org.apache.spark.sql.catalyst.trees.LeafNode
    +
    +/**
    + * Used to link a [[BaseRelation]] in to a logical query plan.
    + */
    +private[sql] case class LogicalRelation(relation: BaseRelation)
    +  extends LogicalPlan
    +  with LeafNode[LogicalPlan]
    --- End diff --
    
    Is a `LogicalRelation` a `LeafNode`?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-61173764
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22561/
    Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by liancheng <gi...@git.apache.org>.
Github user liancheng commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r19657566
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/sources/ddl.scala ---
    @@ -0,0 +1,110 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +
    +package org.apache.spark.sql.sources
    +
    +import org.apache.spark.Logging
    +import org.apache.spark.sql.SQLContext
    +import org.apache.spark.sql.execution.RunnableCommand
    +import org.apache.spark.util.Utils
    +
    +import scala.language.implicitConversions
    +import scala.util.parsing.combinator.lexical.StdLexical
    +import scala.util.parsing.combinator.syntactical.StandardTokenParsers
    +import scala.util.parsing.combinator.PackratParsers
    +
    +import org.apache.spark.sql.catalyst.plans.logical._
    +import org.apache.spark.sql.catalyst.SqlLexical
    +
    +/**
    + * A parser for foreign DDL commands.
    + */
    +private[sql] class DDLParser extends StandardTokenParsers with PackratParsers with Logging {
    +
    +  def apply(input: String): Option[LogicalPlan] = {
    +    phrase(ddl)(new lexical.Scanner(input)) match {
    +      case Success(r, x) => Some(r)
    +      case x =>
    +        logDebug(s"Not recognized as DDL: $x")
    +        None
    +    }
    +  }
    +
    +  protected case class Keyword(str: String)
    +
    +  protected implicit def asParser(k: Keyword): Parser[String] =
    +    lexical.allCaseVersions(k.str).map(x => x : Parser[String]).reduce(_ | _)
    +
    +  protected val CREATE = Keyword("CREATE")
    +  protected val TEMPORARY = Keyword("TEMPORARY")
    +  protected val TABLE = Keyword("TABLE")
    +  protected val USING = Keyword("USING")
    +  protected val OPTIONS = Keyword("OPTIONS")
    +
    +  // Use reflection to find the reserved words defined in this class.
    +  protected val reservedWords =
    +    this.getClass
    +      .getMethods
    +      .filter(_.getReturnType == classOf[Keyword])
    +      .map(_.invoke(this).asInstanceOf[Keyword].str)
    +
    +  override val lexical = new SqlLexical(reservedWords)
    +
    +  protected lazy val ddl: Parser[LogicalPlan] = createTable
    +
    +  /**
    +   * CREATE FOREIGN TEMPORARY TABLE avroTable
    +   * USING org.apache.spark.sql.avro
    +   * OPTIONS (path "../hive/src/test/resources/data/files/episodes.avro")
    +   */
    +  protected lazy val createTable: Parser[LogicalPlan] =
    +    CREATE ~> TEMPORARY ~> TABLE ~> ident ~
    +    USING ~ className ~
    +    OPTIONS ~ options ^^ {
    +      case tableName ~ _ ~ provider ~ _ ~ opts =>
    +        CreateTableUsing(tableName, provider, opts)
    +    }
    +
    +  protected lazy val options: Parser[Map[String, String]] =
    +  "(" ~> repsep(pair, ",") <~ ")" ^^ { case s: Seq[(String, String)] => s.toMap }
    +
    +  protected lazy val className: Parser[String] = repsep(ident, ".") ^^ { case s => s.mkString(".")}
    +
    +  protected lazy val pair: Parser[(String, String)] = ident ~ stringLit ^^ { case k ~ v => (k,v) }
    +}
    +
    +private[sql] case class CreateTableUsing(
    +    tableName: String,
    +    provider: String,
    +    options: Map[String, String]) extends RunnableCommand {
    +
    +  def run(sqlContext: SQLContext) = {
    +    val loader = Utils.getContextOrSparkClassLoader
    --- End diff --
    
    `loader` is not used below.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r19647038
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/commands.scala ---
    @@ -21,10 +21,12 @@ import org.apache.spark.Logging
     import org.apache.spark.annotation.DeveloperApi
     import org.apache.spark.rdd.RDD
     import org.apache.spark.sql.catalyst.errors.TreeNodeException
    -import org.apache.spark.sql.catalyst.expressions.Attribute
    +import org.apache.spark.sql.catalyst.expressions.{Row, Attribute}
    +import org.apache.spark.sql.catalyst.plans.logical
     import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan
    -import org.apache.spark.sql.{Row, SQLConf, SQLContext}
    +import org.apache.spark.sql.{SQLConf, SQLContext}
     
    +// TODO: DELETE ME...
    --- End diff --
    
    This is part of an internal refactor that is started below.  RunnableCommands will prevent the need for having two classes for every single command as well as a special handler in the query planner for each.
    
    Its pretty big though so I decided to not finish it in this PR.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by mateiz <gi...@git.apache.org>.
Github user mateiz commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r19650167
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/sources/package.scala ---
    @@ -0,0 +1,95 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.spark.sql
    +
    +import org.apache.spark.annotation.DeveloperApi
    +import org.apache.spark.rdd.RDD
    +import org.apache.spark.sql.catalyst.expressions.{Expression, Attribute}
    +
    +/**
    + * A set of APIs for adding data sources to Spark SQL.
    + */
    +package object sources {
    +
    +  /**
    +   * Implemented by objects that produce relations for a specific kind of data source.  When
    +   * Spark SQL is given a DDL operation with a USING clause specified, this interface is used to
    +   * pass in the parameters specified by a user.
    +   *
    +   * Users may specify the fully qualified class name of a given data source.  When that class is
    +   * not found Spark SQL will append the class name `DefaultSource` to the path, allowing for
    +   * less verbose invocation.  For example, 'org.apache.spark.sql.json' would resolve to the
    +   * data source 'org.apache.spark.sql.json.DefaultSource'
    +   */
    +  trait RelationProvider {
    +    /** Returns a new base relation with the given parameters. */
    +    def createRelation(sqlContext: SQLContext, parameters: Map[String, String]): BaseRelation
    +  }
    +
    +  /**
    +   * Represents a collection of tuples with a known schema.  Classes that extend BaseRelation must
    +   * be able to produce the schema of their data in the form of a [[StructType]]  In order to be
    +   * executed, a BaseRelation must also mix in at least one of the Scan traits.
    +   *
    +   * BaseRelations must also define a equality function that only returns true when the two
    +   * instances will return the same data.  This equality function is used when determining when
    +   * it is safe to substitute cached results for a given relation.
    +   */
    +  @DeveloperApi
    +  abstract class BaseRelation {
    +    def sqlContext: SQLContext
    +    def schema: StructType
    +  }
    +
    +  /**
    +   * Mixed into a BaseRelation that can produce all of its tuples as an RDD of Row objects.
    +   */
    +  @DeveloperApi
    +  trait TableScan {
    +    self: BaseRelation =>
    --- End diff --
    
    Yeah, it's a bit annoying, but I think keeping compatibility with the people who build against the simpler APIs is quite useful. They can just link to a new Spark version. And if they're looking to use new features, they can anyway do the minimal refactoring to switch to a new version of the API (e.g. instead of extending PrunedScan they can extend PrunedScan2 or whatever).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-61392395
  
      [Test build #22731 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/22731/consoleFull) for   PR 2475 at commit [`1ed6010`](https://github.com/apache/spark/commit/1ed60106b05bd39d5a29fa0e1ac8f98c19d2412a).
     * This patch merges cleanly.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by mateiz <gi...@git.apache.org>.
Github user mateiz commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r19701683
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/sources/filters.scala ---
    @@ -0,0 +1,22 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +
    +package org.apache.spark.sql.sources
    +
    +abstract sealed class Filter
    --- End diff --
    
    If you make it sealed, will this lead to compatibility issues across Spark versions (if someone has code compiled when there was only one subclass of this, but they link to some version of Spark with 2 of them)? At one point I thought sealed classes got their own numeric ID to avoid isInstanceOf checks, but I'm not sure that really happens. @heathermiller do you know how this works?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-61173758
  
      [Test build #22561 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/22561/consoleFull) for   PR 2475 at commit [`360cb30`](https://github.com/apache/spark/commit/360cb30728f2b01acced3fc8b696f1f8ebcb5891).
     * This patch **passes all tests**.
     * This patch merges cleanly.
     * This patch adds the following public classes _(experimental)_:
      * `case class StructField(name: String, dataType: DataType, nullable: Boolean = true) `
      * `trait RunnableCommand extends logical.Command `
      * `case class ExecutedCommand(cmd: RunnableCommand) extends SparkPlan `
      * `  protected case class Keyword(str: String)`
      * `            sys.error(s"Failed to load class for data source: $provider")`
      * `  trait RelationProvider `
      * `  abstract class BaseRelation `
      * `  trait TableScan `
      * `  trait PrunedScan `
      * `  trait FilteredScan `



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-61166399
  
      [Test build #22561 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/22561/consoleFull) for   PR 2475 at commit [`360cb30`](https://github.com/apache/spark/commit/360cb30728f2b01acced3fc8b696f1f8ebcb5891).
     * This patch merges cleanly.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r19647025
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/sources/package.scala ---
    @@ -0,0 +1,95 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.spark.sql
    +
    +import org.apache.spark.annotation.DeveloperApi
    +import org.apache.spark.rdd.RDD
    +import org.apache.spark.sql.catalyst.expressions.{Expression, Attribute}
    +
    +/**
    + * A set of APIs for adding data sources to Spark SQL.
    --- End diff --
    
    would be helfpul to link to an implementation of this, e.g. the json api.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-61387677
  
      [Test build #22711 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/22711/consoleFull) for   PR 2475 at commit [`1d41bb5`](https://github.com/apache/spark/commit/1d41bb50e8f5bc889b9019716462db8358be78f8).
     * This patch **fails Spark unit tests**.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-61337655
  
      [Test build #22646 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/22646/consoleFull) for   PR 2475 at commit [`a70d602`](https://github.com/apache/spark/commit/a70d602c445d8df21cdfba9f4499d6df36d48ada).
     * This patch merges cleanly.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [WIP][SPARK-3247][SQL] An API for adding forei...

Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r18122131
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/avro.scala ---
    @@ -0,0 +1,110 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.spark.sql
    +
    +import org.apache.avro.Schema
    +import org.apache.avro.file.DataFileReader
    +import org.apache.avro.generic.{GenericRecord, GenericDatumReader}
    +import org.apache.avro.mapred.FsInput
    +
    +import org.apache.hadoop.fs.Path
    +
    +import org.apache.spark.sql.foreign._
    +import org.apache.spark.sql.test.TestSQLContext
    +
    +import scala.collection.JavaConversions._
    +
    +/**
    + * An example of a Spark SQL foreign data source for reading data stored in Avro.  Currently this
    + * is only for illustrative purposes and much of the data type conversion logic still needs to be
    + * written.
    + */
    +package object avro extends DataSource {
    --- End diff --
    
    i think it is slightly confusing to make this a package object that extends DataSource for a few reasons
    
    1. it is less common for a package object to implement a lot of stuff
    2. based on the fact that you have an implicit class AvroContext, I'm assuming users would usually import avro._. in this case, createRelation is exposed as well


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by liancheng <gi...@git.apache.org>.
Github user liancheng commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r19662385
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/sources/ddl.scala ---
    @@ -0,0 +1,110 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +
    +package org.apache.spark.sql.sources
    +
    +import org.apache.spark.Logging
    +import org.apache.spark.sql.SQLContext
    +import org.apache.spark.sql.execution.RunnableCommand
    +import org.apache.spark.util.Utils
    +
    +import scala.language.implicitConversions
    +import scala.util.parsing.combinator.lexical.StdLexical
    +import scala.util.parsing.combinator.syntactical.StandardTokenParsers
    +import scala.util.parsing.combinator.PackratParsers
    +
    +import org.apache.spark.sql.catalyst.plans.logical._
    +import org.apache.spark.sql.catalyst.SqlLexical
    +
    +/**
    + * A parser for foreign DDL commands.
    + */
    +private[sql] class DDLParser extends StandardTokenParsers with PackratParsers with Logging {
    +
    +  def apply(input: String): Option[LogicalPlan] = {
    +    phrase(ddl)(new lexical.Scanner(input)) match {
    +      case Success(r, x) => Some(r)
    +      case x =>
    +        logDebug(s"Not recognized as DDL: $x")
    +        None
    +    }
    +  }
    +
    +  protected case class Keyword(str: String)
    +
    +  protected implicit def asParser(k: Keyword): Parser[String] =
    +    lexical.allCaseVersions(k.str).map(x => x : Parser[String]).reduce(_ | _)
    +
    +  protected val CREATE = Keyword("CREATE")
    +  protected val TEMPORARY = Keyword("TEMPORARY")
    +  protected val TABLE = Keyword("TABLE")
    +  protected val USING = Keyword("USING")
    +  protected val OPTIONS = Keyword("OPTIONS")
    +
    +  // Use reflection to find the reserved words defined in this class.
    +  protected val reservedWords =
    +    this.getClass
    +      .getMethods
    +      .filter(_.getReturnType == classOf[Keyword])
    +      .map(_.invoke(this).asInstanceOf[Keyword].str)
    +
    +  override val lexical = new SqlLexical(reservedWords)
    +
    +  protected lazy val ddl: Parser[LogicalPlan] = createTable
    +
    +  /**
    +   * CREATE FOREIGN TEMPORARY TABLE avroTable
    +   * USING org.apache.spark.sql.avro
    +   * OPTIONS (path "../hive/src/test/resources/data/files/episodes.avro")
    +   */
    +  protected lazy val createTable: Parser[LogicalPlan] =
    +    CREATE ~> TEMPORARY ~> TABLE ~> ident ~
    +    USING ~ className ~
    +    OPTIONS ~ options ^^ {
    +      case tableName ~ _ ~ provider ~ _ ~ opts =>
    +        CreateTableUsing(tableName, provider, opts)
    +    }
    --- End diff --
    
    Nit: can be simplified a bit to
    
    ```scala
      protected lazy val createTable: Parser[LogicalPlan] =
        CREATE ~ TEMPORARY ~ TABLE ~> ident ~ (USING ~> className) ~ (OPTIONS ~> options) ^^ {
          case tableName ~ provider ~ opts =>
            CreateTableUsing(tableName, provider, opts)
        }
    ```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by scwf <gi...@git.apache.org>.
Github user scwf commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r19654727
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/json/JSONRelation.scala ---
    @@ -0,0 +1,49 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +
    +package org.apache.spark.sql.json
    +
    +import org.apache.spark.sql.{SQLConf, SQLContext}
    --- End diff --
    
    ```SQLConf``` no need here


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-61342067
  
      [Test build #22646 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/22646/consoleFull) for   PR 2475 at commit [`a70d602`](https://github.com/apache/spark/commit/a70d602c445d8df21cdfba9f4499d6df36d48ada).
     * This patch **passes all tests**.
     * This patch merges cleanly.
     * This patch adds the following public classes _(experimental)_:
      * `abstract class GenericStrategy[PhysicalPlan <: TreeNode[PhysicalPlan]] extends Logging `
      * `case class StructField(name: String, dataType: DataType, nullable: Boolean = true) `
      * `trait RunnableCommand extends logical.Command `
      * `case class ExecutedCommand(cmd: RunnableCommand) extends SparkPlan `
      * `  protected case class Keyword(str: String)`
      * `            sys.error(s"Failed to load class for data source: $provider")`
      * `case class EqualTo(attribute: String, value: Any) extends Filter`
      * `trait RelationProvider `
      * `abstract class BaseRelation `
      * `abstract class TableScan extends BaseRelation `
      * `abstract class PrunedScan extends BaseRelation `
      * `abstract class PrunedFilteredScan extends BaseRelation `



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-61389564
  
      [Test build #22713 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/22713/consoleFull) for   PR 2475 at commit [`ab2c31f`](https://github.com/apache/spark/commit/ab2c31f86352580b8fce64a397d4f07c1b41df59).
     * This patch **passes all tests**.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r19708531
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/sources/filters.scala ---
    @@ -0,0 +1,22 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +
    +package org.apache.spark.sql.sources
    +
    +abstract sealed class Filter
    --- End diff --
    
    Removed `sealed` to be safe.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-61422214
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22765/
    Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by liancheng <gi...@git.apache.org>.
Github user liancheng commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r19659626
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/sources/interfaces.scala ---
    @@ -0,0 +1,86 @@
    +/*
    +* Licensed to the Apache Software Foundation (ASF) under one or more
    +* contributor license agreements.  See the NOTICE file distributed with
    +* this work for additional information regarding copyright ownership.
    +* The ASF licenses this file to You under the Apache License, Version 2.0
    +* (the "License"); you may not use this file except in compliance with
    +* the License.  You may obtain a copy of the License at
    +*
    +*    http://www.apache.org/licenses/LICENSE-2.0
    +*
    +* Unless required by applicable law or agreed to in writing, software
    +* distributed under the License is distributed on an "AS IS" BASIS,
    +* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    +* See the License for the specific language governing permissions and
    +* limitations under the License.
    +*/
    +package org.apache.spark.sql.sources
    +
    +import org.apache.spark.annotation.DeveloperApi
    +import org.apache.spark.rdd.RDD
    +import org.apache.spark.sql.catalyst.expressions.{Expression, Attribute}
    +
    +/**
    + * Implemented by objects that produce relations for a specific kind of data source.  When
    + * Spark SQL is given a DDL operation with a USING clause specified, this interface is used to
    + * pass in the parameters specified by a user.
    + *
    + * Users may specify the fully qualified class name of a given data source.  When that class is
    + * not found Spark SQL will append the class name `DefaultSource` to the path, allowing for
    + * less verbose invocation.  For example, 'org.apache.spark.sql.json' would resolve to the
    + * data source 'org.apache.spark.sql.json.DefaultSource'
    + *
    + * A new instance of this class with be instantiated each time a DDL call is made.
    + */
    +@DeveloperApi
    +trait RelationProvider {
    +  /** Returns a new base relation with the given parameters. */
    +  def createRelation(sqlContext: SQLContext, parameters: Map[String, String]): BaseRelation
    +}
    +
    +/**
    + * Represents a collection of tuples with a known schema.  Classes that extend BaseRelation must
    + * be able to produce the schema of their data in the form of a [[StructType]]  In order to be
    + * executed, a BaseRelation must also mix in at least one of the Scan traits.
    + *
    + * BaseRelations must also define a equality function that only returns true when the two
    + * instances will return the same data.  This equality function is used when determining when
    + * it is safe to substitute cached results for a given relation.
    + */
    +@DeveloperApi
    +abstract class BaseRelation {
    +  def sqlContext: SQLContext
    +  def schema: StructType
    +}
    +
    +/**
    + * Mixed into a BaseRelation that can produce all of its tuples as an RDD of Row objects.
    + */
    +@DeveloperApi
    +abstract class TableScan extends BaseRelation {
    +  def buildScan(): RDD[Row]
    +}
    +
    +/**
    + * Mixed into a BaseRelation that can eliminate unneeded columns before producing an RDD
    + * containing all of its tuples as Row objects.
    + */
    +@DeveloperApi
    +abstract class PrunedScan extends BaseRelation {
    +  def buildScan(requiredColumns: Seq[Attribute]): RDD[Row]
    +}
    +
    +/**
    + * Mixed into a BaseRelation that can eliminate unneeded columns and filter using selected
    + * predicates before producing an RDD containing all matching tuples as Row objects.
    + *
    + * The pushed down filters are currently purely an optimization as they will all be evaluated
    + * again.  This means it is safe to use them with methods that produce false positives such
    + * as filtering partitions based on a bloom filter.
    + */
    +@DeveloperApi
    +abstract class FilteredScan extends BaseRelation {
    --- End diff --
    
    `FilteredPrunedScan` should be a better name for this class. Also need a comment to tell developers if we want to implement foreign data sources that only support filter pushdown without column pruning, we should inherit this class and just output all input columns internally.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [WIP][SPARK-3247][SQL] An API for adding forei...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-56285912
  
      [QA tests have started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/20615/consoleFull) for   PR 2475 at commit [`47d542c`](https://github.com/apache/spark/commit/47d542cc0238fba04b6c4e4456393d812d559c4e).
     * This patch merges cleanly.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r19646942
  
    --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/types/dataTypes.scala ---
    @@ -389,7 +389,7 @@ case class ArrayType(elementType: DataType, containsNull: Boolean) extends DataT
      * @param dataType The data type of this field.
      * @param nullable Indicates if values of this field can be `null` values.
      */
    -case class StructField(name: String, dataType: DataType, nullable: Boolean) {
    +case class StructField(name: String, dataType: DataType, nullable: Boolean = true) {
    --- End diff --
    
    do we need the default value for nullable? seems to me you are setting nullable explicitly everywhere


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-61334958
  
    Test FAILed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22641/
    Test FAILed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-61198599
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22577/
    Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by liancheng <gi...@git.apache.org>.
Github user liancheng commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r19660051
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/sources/interfaces.scala ---
    @@ -0,0 +1,86 @@
    +/*
    +* Licensed to the Apache Software Foundation (ASF) under one or more
    +* contributor license agreements.  See the NOTICE file distributed with
    +* this work for additional information regarding copyright ownership.
    +* The ASF licenses this file to You under the Apache License, Version 2.0
    +* (the "License"); you may not use this file except in compliance with
    +* the License.  You may obtain a copy of the License at
    +*
    +*    http://www.apache.org/licenses/LICENSE-2.0
    +*
    +* Unless required by applicable law or agreed to in writing, software
    +* distributed under the License is distributed on an "AS IS" BASIS,
    +* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    +* See the License for the specific language governing permissions and
    +* limitations under the License.
    +*/
    +package org.apache.spark.sql.sources
    +
    +import org.apache.spark.annotation.DeveloperApi
    +import org.apache.spark.rdd.RDD
    +import org.apache.spark.sql.catalyst.expressions.{Expression, Attribute}
    +
    +/**
    + * Implemented by objects that produce relations for a specific kind of data source.  When
    + * Spark SQL is given a DDL operation with a USING clause specified, this interface is used to
    + * pass in the parameters specified by a user.
    + *
    + * Users may specify the fully qualified class name of a given data source.  When that class is
    + * not found Spark SQL will append the class name `DefaultSource` to the path, allowing for
    + * less verbose invocation.  For example, 'org.apache.spark.sql.json' would resolve to the
    + * data source 'org.apache.spark.sql.json.DefaultSource'
    + *
    + * A new instance of this class with be instantiated each time a DDL call is made.
    + */
    +@DeveloperApi
    +trait RelationProvider {
    +  /** Returns a new base relation with the given parameters. */
    +  def createRelation(sqlContext: SQLContext, parameters: Map[String, String]): BaseRelation
    +}
    +
    +/**
    + * Represents a collection of tuples with a known schema.  Classes that extend BaseRelation must
    + * be able to produce the schema of their data in the form of a [[StructType]]  In order to be
    + * executed, a BaseRelation must also mix in at least one of the Scan traits.
    + *
    + * BaseRelations must also define a equality function that only returns true when the two
    + * instances will return the same data.  This equality function is used when determining when
    + * it is safe to substitute cached results for a given relation.
    + */
    +@DeveloperApi
    +abstract class BaseRelation {
    +  def sqlContext: SQLContext
    +  def schema: StructType
    +}
    +
    +/**
    + * Mixed into a BaseRelation that can produce all of its tuples as an RDD of Row objects.
    + */
    +@DeveloperApi
    +abstract class TableScan extends BaseRelation {
    +  def buildScan(): RDD[Row]
    +}
    +
    +/**
    + * Mixed into a BaseRelation that can eliminate unneeded columns before producing an RDD
    + * containing all of its tuples as Row objects.
    + */
    +@DeveloperApi
    +abstract class PrunedScan extends BaseRelation {
    +  def buildScan(requiredColumns: Seq[Attribute]): RDD[Row]
    +}
    +
    +/**
    + * Mixed into a BaseRelation that can eliminate unneeded columns and filter using selected
    + * predicates before producing an RDD containing all matching tuples as Row objects.
    + *
    + * The pushed down filters are currently purely an optimization as they will all be evaluated
    + * again.  This means it is safe to use them with methods that produce false positives such
    + * as filtering partitions based on a bloom filter.
    + */
    +@DeveloperApi
    +abstract class FilteredScan extends BaseRelation {
    --- End diff --
    
    Conceptually filter pushdown and column pruning are orthogonal. I guess you merged both optimizations into `FilteredScan` and converted all scan traits to abstract classes to simplify the implementation of `DataSources` strategy (e.g. to avoid verbose pattern matching)?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-61343409
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22653/
    Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [WIP][SPARK-3247][SQL] An API for adding forei...

Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r18122141
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/foreign/DDLParser.scala ---
    @@ -0,0 +1,109 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +
    +package org.apache.spark.sql.catalyst
    --- End diff --
    
    why is this in catalyst package?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-61387652
  
    Test FAILed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22709/
    Test FAILed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r19708526
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/sources/filters.scala ---
    @@ -0,0 +1,22 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +
    +package org.apache.spark.sql.sources
    +
    +abstract sealed class Filter
    +
    +case class EqualTo(attribute: String, value: Any) extends Filter
    --- End diff --
    
    Yeah, I wanted to sketch it out first to make sure everyone was onboard.  I've added more types now.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by liancheng <gi...@git.apache.org>.
Github user liancheng commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-61228389
  
    Encountered compilation error while compiling locally (with a clean checkout `./sbt/sbt -Phive,hadoop-2.3 compil`):
    
    ```
    [error] /Users/lian/local/src/spark/master/sql/core/src/main/scala/org/apache/spark/sql/sources/interfaces.scala:85: not found: type Row
    [error]                  filters: Seq[Expression]): RDD[Row]
    [error]                                                 ^
    [error] /Users/lian/local/src/spark/master/sql/core/src/main/scala/org/apache/spark/sql/sources/interfaces.scala:53: not found: type StructType
    [error]   def schema: StructType
    [error]               ^
    [error] /Users/lian/local/src/spark/master/sql/core/src/main/scala/org/apache/spark/sql/sources/interfaces.scala:70: not found: type Row
    [error]   def buildScan(requiredColumns: Seq[Attribute]): RDD[Row]
    [error]                                                       ^
    [error] /Users/lian/local/src/spark/master/sql/core/src/main/scala/org/apache/spark/sql/sources/interfaces.scala:61: not found: type Row
    [error]   def buildScan(): RDD[Row]
    [error]                        ^
    [error] /Users/lian/local/src/spark/master/sql/core/src/main/scala/org/apache/spark/sql/sources/interfaces.scala:38: not found: type SQLContext
    [error]   def createRelation(sqlContext: SQLContext, parameters: Map[String, String]): BaseRelation
    [error]                                  ^
    [error] /Users/lian/local/src/spark/master/sql/core/src/main/scala/org/apache/spark/sql/sources/interfaces.scala:52: not found: type SQLContext
    [error]   def sqlContext: SQLContext
    [error]                   ^
    [warn] one warning found
    [error] 6 errors found
    ```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [WIP][SPARK-3247][SQL] An API for adding forei...

Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r18122137
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/commands.scala ---
    @@ -44,6 +46,34 @@ trait Command {
       override def execute(): RDD[Row] = sqlContext.sparkContext.parallelize(sideEffectResult, 1)
     }
     
    +trait RunnableCommand extends logical.Command {
    +  self: Product =>
    +
    +  def output: Seq[Attribute]
    +  def run(sqlContext: SQLContext): Seq[Row]
    +}
    +
    +case class ExecutedCommand(cmd: RunnableCommand) extends SparkPlan {
    +  /**
    +   * A concrete command should override this lazy field to wrap up any side effects caused by the
    +   * command or any other computation that should be evaluated exactly once. The value of this field
    +   * can be used as the contents of the corresponding RDD generated from the physical plan of this
    +   * command.
    +   *
    +   * The `execute()` method of all the physical command classes should reference `sideEffectResult`
    +   * so that the command can be executed eagerly right after the command query is created.
    +   */
    +  protected[sql] lazy val sideEffectResult: Seq[Row] = cmd.run(sqlContext)
    +
    +  override def output = cmd.output
    +
    +  def children = Nil
    --- End diff --
    
    override?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r19648230
  
    --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/types/dataTypes.scala ---
    @@ -389,7 +389,7 @@ case class ArrayType(elementType: DataType, containsNull: Boolean) extends DataT
      * @param dataType The data type of this field.
      * @param nullable Indicates if values of this field can be `null` values.
      */
    -case class StructField(name: String, dataType: DataType, nullable: Boolean) {
    +case class StructField(name: String, dataType: DataType, nullable: Boolean = true) {
    --- End diff --
    
    We don't _need_ to, but `nullablility` is purely an optimization (that we don't even take advantage of for the most part).  Other parts of the API already default nullable to true.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-61393233
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22731/
    Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-61339108
  
      [Test build #22653 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/22653/consoleFull) for   PR 2475 at commit [`e3e690e`](https://github.com/apache/spark/commit/e3e690e28c31e1cf4852f7d484fb9fcdb7fa0ea1).
     * This patch merges cleanly.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-61394068
  
    LGTM. You'd want to update the PR description before merging it.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r19646962
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/commands.scala ---
    @@ -21,10 +21,12 @@ import org.apache.spark.Logging
     import org.apache.spark.annotation.DeveloperApi
     import org.apache.spark.rdd.RDD
     import org.apache.spark.sql.catalyst.errors.TreeNodeException
    -import org.apache.spark.sql.catalyst.expressions.Attribute
    +import org.apache.spark.sql.catalyst.expressions.{Row, Attribute}
    +import org.apache.spark.sql.catalyst.plans.logical
     import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan
    -import org.apache.spark.sql.{Row, SQLConf, SQLContext}
    +import org.apache.spark.sql.{SQLConf, SQLContext}
     
    +// TODO: DELETE ME...
    --- End diff --
    
    hm?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [WIP][SPARK-3247][SQL] An API for adding forei...

Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r18122136
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/avro.scala ---
    @@ -0,0 +1,110 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.spark.sql
    +
    +import org.apache.avro.Schema
    +import org.apache.avro.file.DataFileReader
    +import org.apache.avro.generic.{GenericRecord, GenericDatumReader}
    +import org.apache.avro.mapred.FsInput
    +
    +import org.apache.hadoop.fs.Path
    +
    +import org.apache.spark.sql.foreign._
    +import org.apache.spark.sql.test.TestSQLContext
    +
    +import scala.collection.JavaConversions._
    +
    +/**
    + * An example of a Spark SQL foreign data source for reading data stored in Avro.  Currently this
    + * is only for illustrative purposes and much of the data type conversion logic still needs to be
    + * written.
    + */
    +package object avro extends DataSource {
    +
    +  /**
    +   * Creates a new relation for data store in avro given a `path` as a parameter.
    +   */
    +  def createRelation(sqlContext: SQLContext, parameters: Map[String, String]) = {
    +    AvroRelation(parameters("path"))(sqlContext)
    +  }
    +
    +  /**
    +   * Adds a method, `avroFile`, to SQLContext that allows reading data stored in Avro.
    +   */
    +  implicit class AvroContext(sqlContext: SQLContext) {
    +    def avroFile(filePath: String) = AvroRelation(filePath)(sqlContext)
    +  }
    +
    +  case class AvroRelation(location: String)(val sqlContext: SQLContext)
    +    extends BaseRelation with TableScan {
    +
    +    val schema = {
    --- End diff --
    
    declare the type explicitly for schema and buildScan. I think that's much more important here because this is going to be an example where people learn to use this ...


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [WIP][SPARK-3247][SQL] An API for adding forei...

Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r18122110
  
    --- Diff: project/SparkBuild.scala ---
    @@ -209,6 +209,7 @@ object Catalyst {
     
     object SQL {
       lazy val settings = Seq(
    +    libraryDependencies += "org.apache.avro" % "avro" % "1.7.7",
    --- End diff --
    
    we should check whether this conflicts with anything we already have ...


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r19645289
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/sources/DataSource.scala ---
    @@ -0,0 +1,21 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.spark.sql.sources
    +
    +import org.apache.spark.sql.SQLContext
    --- End diff --
    
    empty file


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by yhuai <gi...@git.apache.org>.
Github user yhuai commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r19681071
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/SQLContext.scala ---
    @@ -295,6 +306,7 @@ class SQLContext(@transient val sparkContext: SparkContext)
     
         val strategies: Seq[Strategy] =
           CommandStrategy(self) ::
    +      DataSources ::
    --- End diff --
    
    Seems `HiveContext` is missing this strategy.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [WIP][SPARK-3247][SQL] An API for adding forei...

Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r18122133
  
    --- Diff: project/SparkBuild.scala ---
    @@ -209,6 +209,7 @@ object Catalyst {
     
     object SQL {
       lazy val settings = Seq(
    +    libraryDependencies += "org.apache.avro" % "avro" % "1.7.7",
    --- End diff --
    
    This is only here as an illustration.  This library will be its own package likely and I'll remove this before merging.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-61422212
  
      [Test build #22765 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/22765/consoleFull) for   PR 2475 at commit [`1ed6010`](https://github.com/apache/spark/commit/1ed60106b05bd39d5a29fa0e1ac8f98c19d2412a).
     * This patch **passes all tests**.
     * This patch merges cleanly.
     * This patch adds the following public classes _(experimental)_:
      * `abstract class GenericStrategy[PhysicalPlan <: TreeNode[PhysicalPlan]] extends Logging `
      * `trait RunnableCommand extends logical.Command `
      * `case class ExecutedCommand(cmd: RunnableCommand) extends SparkPlan `
      * `  protected case class Keyword(str: String)`
      * `            sys.error(s"Failed to load class for data source: $provider")`
      * `case class EqualTo(attribute: String, value: Any) extends Filter`
      * `case class GreaterThan(attribute: String, value: Any) extends Filter`
      * `case class GreaterThanOrEqual(attribute: String, value: Any) extends Filter`
      * `case class LessThan(attribute: String, value: Any) extends Filter`
      * `case class LessThanOrEqual(attribute: String, value: Any) extends Filter`
      * `trait RelationProvider `
      * `abstract class BaseRelation `
      * `abstract class TableScan extends BaseRelation `
      * `abstract class PrunedScan extends BaseRelation `
      * `abstract class PrunedFilteredScan extends BaseRelation `



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r19648832
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/sources/package.scala ---
    @@ -0,0 +1,95 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.spark.sql
    +
    +import org.apache.spark.annotation.DeveloperApi
    +import org.apache.spark.rdd.RDD
    +import org.apache.spark.sql.catalyst.expressions.{Expression, Attribute}
    +
    +/**
    + * A set of APIs for adding data sources to Spark SQL.
    + */
    +package object sources {
    +
    +  /**
    +   * Implemented by objects that produce relations for a specific kind of data source.  When
    +   * Spark SQL is given a DDL operation with a USING clause specified, this interface is used to
    +   * pass in the parameters specified by a user.
    +   *
    +   * Users may specify the fully qualified class name of a given data source.  When that class is
    +   * not found Spark SQL will append the class name `DefaultSource` to the path, allowing for
    +   * less verbose invocation.  For example, 'org.apache.spark.sql.json' would resolve to the
    +   * data source 'org.apache.spark.sql.json.DefaultSource'
    +   */
    +  trait RelationProvider {
    +    /** Returns a new base relation with the given parameters. */
    +    def createRelation(sqlContext: SQLContext, parameters: Map[String, String]): BaseRelation
    +  }
    +
    +  /**
    +   * Represents a collection of tuples with a known schema.  Classes that extend BaseRelation must
    +   * be able to produce the schema of their data in the form of a [[StructType]]  In order to be
    +   * executed, a BaseRelation must also mix in at least one of the Scan traits.
    +   *
    +   * BaseRelations must also define a equality function that only returns true when the two
    +   * instances will return the same data.  This equality function is used when determining when
    +   * it is safe to substitute cached results for a given relation.
    +   */
    +  @DeveloperApi
    +  abstract class BaseRelation {
    +    def sqlContext: SQLContext
    +    def schema: StructType
    +  }
    +
    +  /**
    +   * Mixed into a BaseRelation that can produce all of its tuples as an RDD of Row objects.
    +   */
    +  @DeveloperApi
    +  trait TableScan {
    +    self: BaseRelation =>
    --- End diff --
    
    @rxin agrees with you, and after thinking about the possible ways to extend the filter API down the road by adding methods, I'm convinced to use abstract classes instead.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by mateiz <gi...@git.apache.org>.
Github user mateiz commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r19645814
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/sources/package.scala ---
    @@ -0,0 +1,95 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.spark.sql
    +
    +import org.apache.spark.annotation.DeveloperApi
    +import org.apache.spark.rdd.RDD
    +import org.apache.spark.sql.catalyst.expressions.{Expression, Attribute}
    +
    +/**
    + * A set of APIs for adding data sources to Spark SQL.
    + */
    +package object sources {
    +
    +  /**
    +   * Implemented by objects that produce relations for a specific kind of data source.  When
    +   * Spark SQL is given a DDL operation with a USING clause specified, this interface is used to
    +   * pass in the parameters specified by a user.
    +   *
    +   * Users may specify the fully qualified class name of a given data source.  When that class is
    +   * not found Spark SQL will append the class name `DefaultSource` to the path, allowing for
    +   * less verbose invocation.  For example, 'org.apache.spark.sql.json' would resolve to the
    +   * data source 'org.apache.spark.sql.json.DefaultSource'
    +   */
    +  trait RelationProvider {
    +    /** Returns a new base relation with the given parameters. */
    +    def createRelation(sqlContext: SQLContext, parameters: Map[String, String]): BaseRelation
    +  }
    +
    +  /**
    +   * Represents a collection of tuples with a known schema.  Classes that extend BaseRelation must
    +   * be able to produce the schema of their data in the form of a [[StructType]]  In order to be
    +   * executed, a BaseRelation must also mix in at least one of the Scan traits.
    +   *
    +   * BaseRelations must also define a equality function that only returns true when the two
    +   * instances will return the same data.  This equality function is used when determining when
    +   * it is safe to substitute cached results for a given relation.
    +   */
    +  @DeveloperApi
    +  abstract class BaseRelation {
    +    def sqlContext: SQLContext
    +    def schema: StructType
    +  }
    +
    +  /**
    +   * Mixed into a BaseRelation that can produce all of its tuples as an RDD of Row objects.
    +   */
    +  @DeveloperApi
    +  trait TableScan {
    +    self: BaseRelation =>
    --- End diff --
    
    Any thought on making subclasses of BaseRelation that are also abstract classes instead of using traits with self types? It seems like it would be more binary-compatibility-friendly and less likely to change across Scala releases.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [WIP][SPARK-3247][SQL] An API for adding forei...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-58130999
  
    Test FAILed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/21361/Test FAILed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-61342072
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22646/
    Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by yhuai <gi...@git.apache.org>.
Github user yhuai commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r19687528
  
    --- Diff: sql/core/src/test/scala/org/apache/spark/sql/sources/PrunedScanSuite.scala ---
    @@ -0,0 +1,102 @@
    +/*
    +* Licensed to the Apache Software Foundation (ASF) under one or more
    +* contributor license agreements.  See the NOTICE file distributed with
    +* this work for additional information regarding copyright ownership.
    +* The ASF licenses this file to You under the Apache License, Version 2.0
    +* (the "License"); you may not use this file except in compliance with
    +* the License.  You may obtain a copy of the License at
    +*
    +*    http://www.apache.org/licenses/LICENSE-2.0
    +*
    +* Unless required by applicable law or agreed to in writing, software
    +* distributed under the License is distributed on an "AS IS" BASIS,
    +* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    +* See the License for the specific language governing permissions and
    +* limitations under the License.
    +*/
    +
    +package org.apache.spark.sql.sources
    +
    +import org.apache.spark.sql.catalyst.expressions.{Row => _, _}
    +import org.apache.spark.sql._
    +
    +class PrunedScanSource extends RelationProvider {
    +  override def createRelation(
    +      sqlContext: SQLContext,
    +      parameters: Map[String, String]): BaseRelation = {
    +    SimplePrunedScan(parameters("from").toInt, parameters("to").toInt)(sqlContext)
    +  }
    +}
    +
    +case class SimplePrunedScan(from: Int, to: Int)(@transient val sqlContext: SQLContext)
    +  extends PrunedScan {
    +
    +  override def schema =
    +    StructType(
    +      StructField("a", IntegerType, nullable = false) ::
    +      StructField("b", IntegerType, nullable = false) :: Nil)
    +
    +  override def buildScan(requiredColumns: Seq[Attribute]) = {
    +    val rowBuilders = requiredColumns.map(_.name).map {
    +      case "a" => (i: Int) => Seq(i)
    +      case "b" => (i: Int) => Seq(i * 2)
    +    }
    +
    +    sqlContext.sparkContext.parallelize(from to to).map(i =>
    +      Row.fromSeq(rowBuilders.map(_(i)).reduceOption(_ ++ _).getOrElse(Seq.empty)))
    +  }
    +}
    +
    +class PrunedScanSuite extends DataSourceTest {
    +  import caseInsensisitiveContext._
    +
    +  before {
    +    sql(
    +      """
    +        |CREATE TEMPORARY TABLE oneToTenPruned
    +        |USING org.apache.spark.sql.sources.PrunedScanSource
    +        |OPTIONS (
    +        |  from '1',
    +        |  to '10'
    +        |)
    +      """.stripMargin)
    +  }
    +
    +  sqlTest(
    +    "SELECT * FROM oneToTenPruned",
    +    (1 to 10).map(i => Row(i, i * 2)).toSeq)
    +
    +  sqlTest(
    +    "SELECT a, b FROM oneToTenPruned",
    +    (1 to 10).map(i => Row(i, i * 2)).toSeq)
    +
    +  sqlTest(
    +    "SELECT b, a FROM oneToTenPruned",
    +    (1 to 10).map(i => Row(i * 2, i)).toSeq)
    +
    +  sqlTest(
    +    "SELECT a FROM oneToTenPruned",
    --- End diff --
    
    Is the purpose of this test to check if we can correctly prune columns? If so, I guess we also need to check the `output` of the `PhysicalRDD`.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-61387678
  
    Test FAILed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22711/
    Test FAILed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r19645425
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/json/JSONRelation.scala ---
    @@ -0,0 +1,52 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +
    +package org.apache.spark.sql.json
    +
    +import org.apache.spark.sql.{SQLConf, SQLContext}
    +import org.apache.spark.sql.sources._
    +
    +private[sql] class DefaultSource extends RelationProvider {
    +  /** Returns a new base relation with the given parameters. */
    +  override def createRelation(
    +      sqlContext: SQLContext,
    +      parameters: Map[String, String]): BaseRelation = {
    +    val fileName =
    +      parameters.getOrElse("fileName", sys.error(s"Option 'fileName' not specified"))
    --- End diff --
    
    I think this fits on one line and you don't need to line wrap ... also you can remove "s" since there is no interpolation


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by mateiz <gi...@git.apache.org>.
Github user mateiz commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r19645907
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/sources/package.scala ---
    @@ -0,0 +1,95 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.spark.sql
    +
    +import org.apache.spark.annotation.DeveloperApi
    +import org.apache.spark.rdd.RDD
    +import org.apache.spark.sql.catalyst.expressions.{Expression, Attribute}
    +
    +/**
    + * A set of APIs for adding data sources to Spark SQL.
    + */
    +package object sources {
    +
    +  /**
    +   * Implemented by objects that produce relations for a specific kind of data source.  When
    +   * Spark SQL is given a DDL operation with a USING clause specified, this interface is used to
    +   * pass in the parameters specified by a user.
    +   *
    +   * Users may specify the fully qualified class name of a given data source.  When that class is
    +   * not found Spark SQL will append the class name `DefaultSource` to the path, allowing for
    +   * less verbose invocation.  For example, 'org.apache.spark.sql.json' would resolve to the
    +   * data source 'org.apache.spark.sql.json.DefaultSource'
    +   */
    +  trait RelationProvider {
    +    /** Returns a new base relation with the given parameters. */
    +    def createRelation(sqlContext: SQLContext, parameters: Map[String, String]): BaseRelation
    +  }
    +
    +  /**
    +   * Represents a collection of tuples with a known schema.  Classes that extend BaseRelation must
    +   * be able to produce the schema of their data in the form of a [[StructType]]  In order to be
    +   * executed, a BaseRelation must also mix in at least one of the Scan traits.
    +   *
    +   * BaseRelations must also define a equality function that only returns true when the two
    +   * instances will return the same data.  This equality function is used when determining when
    +   * it is safe to substitute cached results for a given relation.
    +   */
    +  @DeveloperApi
    +  abstract class BaseRelation {
    +    def sqlContext: SQLContext
    +    def schema: StructType
    +  }
    +
    +  /**
    +   * Mixed into a BaseRelation that can produce all of its tuples as an RDD of Row objects.
    +   */
    +  @DeveloperApi
    +  trait TableScan {
    +    self: BaseRelation =>
    +
    +    def buildScan(): RDD[Row]
    +  }
    +
    +  /**
    +   * Mixed into a BaseRelation that can eliminate unneeded columns before producing an RDD
    +   * containing all of its tuples as Row objects.
    +   */
    +  @DeveloperApi
    +  trait PrunedScan {
    +    self: BaseRelation =>
    +
    +    def buildScan(requiredColumns: Seq[Attribute]): RDD[Row]
    --- End diff --
    
    Should this take AttributeReferences or field names instead? I'm not sure what the semantics here really is. Or can this also be used for nested ones (e.g. `select user.address.city`)?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by mateiz <gi...@git.apache.org>.
Github user mateiz commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r19646688
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/sources/package.scala ---
    @@ -0,0 +1,95 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.spark.sql
    +
    +import org.apache.spark.annotation.DeveloperApi
    +import org.apache.spark.rdd.RDD
    +import org.apache.spark.sql.catalyst.expressions.{Expression, Attribute}
    +
    +/**
    + * A set of APIs for adding data sources to Spark SQL.
    + */
    +package object sources {
    +
    +  /**
    +   * Implemented by objects that produce relations for a specific kind of data source.  When
    +   * Spark SQL is given a DDL operation with a USING clause specified, this interface is used to
    +   * pass in the parameters specified by a user.
    +   *
    +   * Users may specify the fully qualified class name of a given data source.  When that class is
    +   * not found Spark SQL will append the class name `DefaultSource` to the path, allowing for
    +   * less verbose invocation.  For example, 'org.apache.spark.sql.json' would resolve to the
    +   * data source 'org.apache.spark.sql.json.DefaultSource'
    +   */
    +  trait RelationProvider {
    +    /** Returns a new base relation with the given parameters. */
    +    def createRelation(sqlContext: SQLContext, parameters: Map[String, String]): BaseRelation
    +  }
    +
    +  /**
    +   * Represents a collection of tuples with a known schema.  Classes that extend BaseRelation must
    +   * be able to produce the schema of their data in the form of a [[StructType]]  In order to be
    +   * executed, a BaseRelation must also mix in at least one of the Scan traits.
    +   *
    +   * BaseRelations must also define a equality function that only returns true when the two
    +   * instances will return the same data.  This equality function is used when determining when
    +   * it is safe to substitute cached results for a given relation.
    +   */
    +  @DeveloperApi
    +  abstract class BaseRelation {
    +    def sqlContext: SQLContext
    +    def schema: StructType
    +  }
    +
    +  /**
    +   * Mixed into a BaseRelation that can produce all of its tuples as an RDD of Row objects.
    +   */
    +  @DeveloperApi
    +  trait TableScan {
    +    self: BaseRelation =>
    +
    +    def buildScan(): RDD[Row]
    +  }
    +
    +  /**
    +   * Mixed into a BaseRelation that can eliminate unneeded columns before producing an RDD
    +   * containing all of its tuples as Row objects.
    +   */
    +  @DeveloperApi
    +  trait PrunedScan {
    +    self: BaseRelation =>
    +
    +    def buildScan(requiredColumns: Seq[Attribute]): RDD[Row]
    --- End diff --
    
    I think it's better to resolve them here if we can (and we do have the schema for these to do it). I'm just worried about keeping this narrow in the case where we add new subclasses of Attribute in the future -- we don't want implementors of this to suddenly have to deal with them.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-61338760
  
    Okay, thanks for all the comments. 
    
    I've done the following:
     - Changed the various base relations into `abstract class` instead of `trait`.
     - Removed `Expression`, `Attribute`, `Seq` from all function signatures instead using `Filter`, `String` and `Array` (a simple case class that is used only in this package) respectively.  This should make binary compatibility / java interop easier, and also takes care of some things like `a = 1` vs `1 = a` for the user.  Right now this only supports equality, but that will be easy to extend.  This comes at the cost of expressivity but its probably worth it for long term compatibility.
     - Added test cases that verify that pushdown of filters and column pruning are happening as expected.
     - Pulled `Strategy` out of the Planner so it is now possible to integrate very tightly if so desired.  This interface is appropriately commented as being unstable and only for experimentation.
     - Addressed a bunch of other small comments


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-61388270
  
      [Test build #22713 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/22713/consoleFull) for   PR 2475 at commit [`ab2c31f`](https://github.com/apache/spark/commit/ab2c31f86352580b8fce64a397d4f07c1b41df59).
     * This patch merges cleanly.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-61361324
  
    I like the new API a lot. Java support. Long term binary compatibility! 
    



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by mateiz <gi...@git.apache.org>.
Github user mateiz commented on the pull request:

    https://github.com/apache/spark/pull/2475#issuecomment-61358186
  
    The new API for sources looks good to me, thanks for making the changes. It will be easy to plug in a lot of neat data sources here.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r19686971
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/sources/interfaces.scala ---
    @@ -0,0 +1,86 @@
    +/*
    +* Licensed to the Apache Software Foundation (ASF) under one or more
    +* contributor license agreements.  See the NOTICE file distributed with
    +* this work for additional information regarding copyright ownership.
    +* The ASF licenses this file to You under the Apache License, Version 2.0
    +* (the "License"); you may not use this file except in compliance with
    +* the License.  You may obtain a copy of the License at
    +*
    +*    http://www.apache.org/licenses/LICENSE-2.0
    +*
    +* Unless required by applicable law or agreed to in writing, software
    +* distributed under the License is distributed on an "AS IS" BASIS,
    +* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    +* See the License for the specific language governing permissions and
    +* limitations under the License.
    +*/
    +package org.apache.spark.sql.sources
    +
    +import org.apache.spark.annotation.DeveloperApi
    +import org.apache.spark.rdd.RDD
    +import org.apache.spark.sql.catalyst.expressions.{Expression, Attribute}
    +
    +/**
    + * Implemented by objects that produce relations for a specific kind of data source.  When
    + * Spark SQL is given a DDL operation with a USING clause specified, this interface is used to
    + * pass in the parameters specified by a user.
    + *
    + * Users may specify the fully qualified class name of a given data source.  When that class is
    + * not found Spark SQL will append the class name `DefaultSource` to the path, allowing for
    + * less verbose invocation.  For example, 'org.apache.spark.sql.json' would resolve to the
    + * data source 'org.apache.spark.sql.json.DefaultSource'
    + *
    + * A new instance of this class with be instantiated each time a DDL call is made.
    + */
    +@DeveloperApi
    +trait RelationProvider {
    +  /** Returns a new base relation with the given parameters. */
    +  def createRelation(sqlContext: SQLContext, parameters: Map[String, String]): BaseRelation
    +}
    +
    +/**
    + * Represents a collection of tuples with a known schema.  Classes that extend BaseRelation must
    + * be able to produce the schema of their data in the form of a [[StructType]]  In order to be
    + * executed, a BaseRelation must also mix in at least one of the Scan traits.
    + *
    + * BaseRelations must also define a equality function that only returns true when the two
    + * instances will return the same data.  This equality function is used when determining when
    + * it is safe to substitute cached results for a given relation.
    --- End diff --
    
    BaseRelation is not a LogicalPlan so it just needs to have a reasonable equality function.  The same result function is defined in LogicalRelation.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r19651082
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/sources/package.scala ---
    @@ -0,0 +1,95 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.spark.sql
    +
    +import org.apache.spark.annotation.DeveloperApi
    +import org.apache.spark.rdd.RDD
    +import org.apache.spark.sql.catalyst.expressions.{Expression, Attribute}
    +
    +/**
    + * A set of APIs for adding data sources to Spark SQL.
    + */
    +package object sources {
    +
    +  /**
    +   * Implemented by objects that produce relations for a specific kind of data source.  When
    +   * Spark SQL is given a DDL operation with a USING clause specified, this interface is used to
    +   * pass in the parameters specified by a user.
    +   *
    +   * Users may specify the fully qualified class name of a given data source.  When that class is
    +   * not found Spark SQL will append the class name `DefaultSource` to the path, allowing for
    +   * less verbose invocation.  For example, 'org.apache.spark.sql.json' would resolve to the
    +   * data source 'org.apache.spark.sql.json.DefaultSource'
    +   */
    +  trait RelationProvider {
    +    /** Returns a new base relation with the given parameters. */
    +    def createRelation(sqlContext: SQLContext, parameters: Map[String, String]): BaseRelation
    +  }
    +
    +  /**
    +   * Represents a collection of tuples with a known schema.  Classes that extend BaseRelation must
    +   * be able to produce the schema of their data in the form of a [[StructType]]  In order to be
    +   * executed, a BaseRelation must also mix in at least one of the Scan traits.
    +   *
    +   * BaseRelations must also define a equality function that only returns true when the two
    +   * instances will return the same data.  This equality function is used when determining when
    +   * it is safe to substitute cached results for a given relation.
    +   */
    +  @DeveloperApi
    +  abstract class BaseRelation {
    +    def sqlContext: SQLContext
    +    def schema: StructType
    +  }
    +
    +  /**
    +   * Mixed into a BaseRelation that can produce all of its tuples as an RDD of Row objects.
    +   */
    +  @DeveloperApi
    +  trait TableScan {
    +    self: BaseRelation =>
    +
    +    def buildScan(): RDD[Row]
    +  }
    +
    +  /**
    +   * Mixed into a BaseRelation that can eliminate unneeded columns before producing an RDD
    +   * containing all of its tuples as Row objects.
    +   */
    +  @DeveloperApi
    +  trait PrunedScan {
    +    self: BaseRelation =>
    +
    +    def buildScan(requiredColumns: Seq[Attribute]): RDD[Row]
    --- End diff --
    
    To be clear they will always be resolved here (or the analyzer would have thrown an exception).  I just usually don't use AttributeReference in type signatures so as to hide their implementation details.
    
    You raise some good points though, and are making me reconsider an earlier design where we don't expose `Attribute` or `Expression`.  Instead we have an interface like this:
    
    Attributes are `String`. This can just be `attributeName`.  We can possibly also support `attributeName.fieldName` (or  maybe even `attributeName[0]`?).  The planner will default to just pushing down full attribute projections, but the developer can override methods to turn on the more advanced projections.
    
    For filters we define a simple set initially that we can add to as we go:
    
    ```scala
    case class EqualTo(attribute: String, value: Any)
    ```
    
    Pros: Easier to make binary compatibility promises.  Users don't have to handle `a = 1` and `1 = a` any more.  Does not expose catalyst.
    Cons: Users have to deal with string parsing for complex projections.  Expressions pushed down are now are limited to those we decide to write duplicate code for.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request: [SPARK-3247][SQL] An API for adding data sourc...

Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2475#discussion_r19652398
  
    --- Diff: sql/core/src/main/scala/org/apache/spark/sql/sources/package.scala ---
    @@ -0,0 +1,92 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.spark.sql
    +
    +import org.apache.spark.annotation.DeveloperApi
    +import org.apache.spark.rdd.RDD
    +import org.apache.spark.sql.catalyst.expressions.{Expression, Attribute}
    +
    +/**
    + * A set of APIs for adding data sources to Spark SQL.
    + */
    +package object sources {
    --- End diff --
    
    I did a test on the generated class name for classes embedded in a package object. The classname is actually something like "package$RelationProvider". 
    
    We should really just make these top level classes rather than putting them in the package object. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org