Skip to content

Conversation

@gatorsmile
Copy link
Member

What changes were proposed in this pull request?

Address the comments by @yhuai in the original PR: #14207

First, issue an exception instead of logging a warning when users specify the partitioning columns without a given schema.

Second, refactor the codes a little.

How was this patch tested?

Fixed the test cases.

@SparkQA
Copy link

SparkQA commented Aug 10, 2016

Test build #63479 has finished for PR 14572 at commit b7c0994.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Aug 10, 2016

Test build #63489 has finished for PR 14572 at commit 35cbf9d.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Aug 10, 2016

Test build #63553 has finished for PR 14572 at commit d22dcf3.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class CachedData(plan: LogicalPlan, cachedRepresentation: InMemoryRelation)
    • class CacheManager extends Logging
    • trait DataSourceScanExec extends LeafExecNode with CodegenSupport
    • case class RowDataSourceScanExec(
    • case class FileSourceScanExec(
    • case class ExternalRDD[T](
    • case class ExternalRDDScanExec[T](
    • case class LogicalRDD(
    • case class RDDScanExec(
    • trait FileRelation
    • case class LocalTableScanExec(
    • abstract class RowIterator
    • trait LeafExecNode extends SparkPlan
    • trait UnaryExecNode extends SparkPlan
    • trait BinaryExecNode extends SparkPlan
    • case class PlanLater(plan: LogicalPlan) extends LeafExecNode
    • abstract class SparkStrategies extends QueryPlanner[SparkPlan]
    • class UnsafeRowSerializer(
    • class TypedSumDouble[IN](val f: IN => Double) extends Aggregator[IN, Double, Double]
    • class TypedSumLong[IN](val f: IN => Long) extends Aggregator[IN, Long, Long]
    • class TypedCount[IN](val f: IN => Any) extends Aggregator[IN, Long, Long]
    • class TypedAverage[IN](val f: IN => Double) extends Aggregator[IN, (Double, Long), Double]
    • case class ScalaUDAF(
    • case class InMemoryRelation(
    • case class InMemoryTableScanExec(
    • trait RunnableCommand extends LogicalPlan with logical.Command
    • case class ExecutedCommandExec(cmd: RunnableCommand) extends SparkPlan
    • case class AlterTableRecoverPartitionsCommand(
    • case class DataSourceAnalysis(conf: CatalystConf) extends Rule[LogicalPlan]
    • class FindDataSourceTable(sparkSession: SparkSession) extends Rule[LogicalPlan]
    • case class InsertIntoDataSourceCommand(
    • case class InsertIntoHadoopFsRelationCommand(
    • case class PartitionDirectory(values: InternalRow, path: Path)
    • case class PartitionSpec(
    • case class JDBCPartition(whereClause: String, idx: Int) extends Partition
    • class ResolveDataSource(sparkSession: SparkSession) extends Rule[LogicalPlan]
    • case class PreprocessTableInsertion(conf: SQLConf) extends Rule[LogicalPlan]
    • case class PreWriteCheck(conf: SQLConf, catalog: SessionCatalog)
    • case class DebugExec(child: SparkPlan) extends UnaryExecNode with CodegenSupport
    • class ExchangeCoordinator(
    • case class MapPartitionsRWrapper(
    • class IncrementalExecution(
    • class ExecutionPage(parent: SQLTab) extends WebUIPage(\"execution\") with Logging
    • class SQLHistoryListenerFactory extends SparkHistoryListenerFactory
    • class SQLListener(conf: SparkConf) extends SparkListener with Logging
    • class SQLHistoryListener(conf: SparkConf, sparkUI: SparkUI)
    • class SQLTab(val listener: SQLListener, sparkUI: SparkUI)
    • case class SparkPlanGraph(

@gatorsmile
Copy link
Member Author

retest this please

@SparkQA
Copy link

SparkQA commented Aug 22, 2016

Test build #64175 has finished for PR 14572 at commit d22dcf3.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@yhuai
Copy link
Contributor

yhuai commented Aug 22, 2016

sorry. I missed this PR. Can you update?

@gatorsmile
Copy link
Member Author

No problem. The conflicts are resolved. Thanks! @yhuai

failAnalysis("Cannot specify partition information if the table schema is not specified " +
"when creating and will be inferred at runtime")
}
c
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because of this change, I think we need to create a new jira.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A new JIRA is created and the PR title is updated. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about It is not allowed to specify partition columns when the table schema is not defined. When the table schema is not provided, schema and partition columns will be inferred.?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds better to me. Thanks! Let me change it now.

@gatorsmile gatorsmile changed the title [SPARK-16552] [FOLLOW-UP] [SQL] Store the Inferred Schemas into External Catalog Tables when Creating Tables [SPARK-17192] [SQL] Store the Inferred Schemas into External Catalog Tables when Creating Tables Aug 22, 2016
@SparkQA
Copy link

SparkQA commented Aug 22, 2016

Test build #64225 has finished for PR 14572 at commit a9c94ea.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@gatorsmile gatorsmile changed the title [SPARK-17192] [SQL] Store the Inferred Schemas into External Catalog Tables when Creating Tables [SPARK-17192] [SQL] Issue Exception when Users Specify the Partitioning Columns without a Given Schema Aug 22, 2016
@yhuai
Copy link
Contributor

yhuai commented Aug 22, 2016

LGTM. I left one comment about the error message

@SparkQA
Copy link

SparkQA commented Aug 23, 2016

Test build #64244 has finished for PR 14572 at commit d3a79c8.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@gatorsmile
Copy link
Member Author

retest this please

@SparkQA
Copy link

SparkQA commented Aug 26, 2016

Test build #64452 has finished for PR 14572 at commit d3a79c8.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@gatorsmile
Copy link
Member Author

ping @yhuai : )

@yhuai
Copy link
Contributor

yhuai commented Aug 26, 2016

LGTM. Thanks. Merging to master.

@asfgit asfgit closed this in fd4ba3f Aug 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants