Skip to content

Conversation

@rxin
Copy link
Contributor

@rxin rxin commented Aug 9, 2015

There are a few changes in this pull request:

  1. Moved all data sources to execution.datasources, except the public JDBC APIs.
  2. In order to maintain backward compatibility from 1, added a backward compatibility translation map in data source resolution.
  3. Moved ui and metric package into execution.
  4. Added more documentation on some internal classes.
  5. Renamed DataSourceRegister.format -> shortName.
  6. Added "override" modifier on shortName.
  7. Removed IntSQLMetric.

@SparkQA
Copy link

SparkQA commented Aug 9, 2015

Test build #40259 has finished for PR 8056 at commit 122864a.

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

@rxin
Copy link
Contributor Author

rxin commented Aug 9, 2015

cc @zsxwing for review.

@zsxwing
Copy link
Member

zsxwing commented Aug 9, 2015

@rxin could you help remove @VisibleForTesting from SQLListener.scala because of the issue mentioned by @shivaram in #7774 (comment) ?

@SparkQA
Copy link

SparkQA commented Aug 9, 2015

Test build #40263 has finished for PR 8056 at commit 8f4dc20.

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

@zsxwing
Copy link
Member

zsxwing commented Aug 9, 2015

Although the SQL UI only displays the execution info now, moving the ui package into the execution package looks not a good idea. I guess we may add other information to the SQL tab later, then we may need to move the ui package back.

@rxin
Copy link
Contributor Author

rxin commented Aug 9, 2015

@zsxwing removed visible for testing tag.

We can move UI package around later -- it's also OK if it contains non-execution stuff. Not that big of a deal...

@SparkQA
Copy link

SparkQA commented Aug 9, 2015

Test build #40275 has finished for PR 8056 at commit c3a4ba4.

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

@SparkQA
Copy link

SparkQA commented Aug 10, 2015

Test build #40280 has finished for PR 8056 at commit 9d83ba2.

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

@zsxwing
Copy link
Member

zsxwing commented Aug 10, 2015

The failure is because ParquetIOSuite has a hard-code name.

@zsxwing
Copy link
Member

zsxwing commented Aug 10, 2015

Is it intentional to keep DDLSourceLoadSuite.scala and ResolvedDataSourceSuite.scala under org.apache.spark.sql.sources?

@SparkQA
Copy link

SparkQA commented Aug 10, 2015

Test build #40283 has finished for PR 8056 at commit 3dfc06c.

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

@SparkQA
Copy link

SparkQA commented Aug 10, 2015

Test build #1419 has finished for PR 8056 at commit 3dfc06c.

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

@rxin
Copy link
Contributor Author

rxin commented Aug 10, 2015

Not intentional -- but not that big of a deal for them to be there since I care more about the public API visibility here.

@SparkQA
Copy link

SparkQA commented Aug 10, 2015

Test build #40306 has finished for PR 8056 at commit 9df4801.

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

@rxin
Copy link
Contributor Author

rxin commented Aug 10, 2015

OK I'm going to merge this since it's from a flaky test.

@rxin
Copy link
Contributor Author

rxin commented Aug 10, 2015

cc @liancheng for the change also.

asfgit pushed a commit that referenced this pull request Aug 10, 2015
There are a few changes in this pull request:

1. Moved all data sources to execution.datasources, except the public JDBC APIs.
2. In order to maintain backward compatibility from 1, added a backward compatibility translation map in data source resolution.
3. Moved ui and metric package into execution.
4. Added more documentation on some internal classes.
5. Renamed DataSourceRegister.format -> shortName.
6. Added "override" modifier on shortName.
7. Removed IntSQLMetric.

Author: Reynold Xin <[email protected]>

Closes #8056 from rxin/SPARK-9763 and squashes the following commits:

9df4801 [Reynold Xin] Removed hardcoded name in test cases.
d9babc6 [Reynold Xin] Shorten.
e484419 [Reynold Xin] Removed VisibleForTesting.
171b812 [Reynold Xin] MimaExcludes.
2041389 [Reynold Xin] Compile ...
79dda42 [Reynold Xin] Compile.
0818ba3 [Reynold Xin] Removed IntSQLMetric.
c46884f [Reynold Xin] Two more fixes.
f9aa88d [Reynold Xin] [SPARK-9763][SQL] Minimize exposure of internal SQL classes.

(cherry picked from commit 40ed2af)
Signed-off-by: Reynold Xin <[email protected]>
@asfgit asfgit closed this in 40ed2af Aug 10, 2015
@rama-mullapudi
Copy link

jdbcutils scala code has a typo error in schemaString function, decimal type has extra closing braces } which is causing create table with decimal fail as create statement sent to db looks as below

CREATE TABLE foo (TKT_GID DECIMAL(10},0}) NOT NULL)

Below is the code

def schemaString(df: DataFrame, url: String): String = {
.....
case t: DecimalType => s"DECIMAL(${t.precision}},${t.scale}})"
....
}

@rxin
Copy link
Contributor Author

rxin commented Aug 15, 2015

@rama-mullapudi thanks. can you submit a patch to fix those?

I think I only moved the stuff around.

CodingCat pushed a commit to CodingCat/spark that referenced this pull request Aug 17, 2015
There are a few changes in this pull request:

1. Moved all data sources to execution.datasources, except the public JDBC APIs.
2. In order to maintain backward compatibility from 1, added a backward compatibility translation map in data source resolution.
3. Moved ui and metric package into execution.
4. Added more documentation on some internal classes.
5. Renamed DataSourceRegister.format -> shortName.
6. Added "override" modifier on shortName.
7. Removed IntSQLMetric.

Author: Reynold Xin <[email protected]>

Closes apache#8056 from rxin/SPARK-9763 and squashes the following commits:

9df4801 [Reynold Xin] Removed hardcoded name in test cases.
d9babc6 [Reynold Xin] Shorten.
e484419 [Reynold Xin] Removed VisibleForTesting.
171b812 [Reynold Xin] MimaExcludes.
2041389 [Reynold Xin] Compile ...
79dda42 [Reynold Xin] Compile.
0818ba3 [Reynold Xin] Removed IntSQLMetric.
c46884f [Reynold Xin] Two more fixes.
f9aa88d [Reynold Xin] [SPARK-9763][SQL] Minimize exposure of internal SQL classes.
}

val dataSchema =
StructType(schema.filterNot(f => partitionColumns.contains(f.name))).asNullable
Copy link
Contributor

Choose a reason for hiding this comment

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

@rxin Following the discussion on SPARK-9763, I am actually wondering why we convert the StructType with "asNullable" which set all the contained StructField to be Nullable. This will cause problem when one StructFiled is not allowed to be nullable, but the HadoopFsRelationProvider automatically sets it to be nullable. Is it because that all the fields in HadoopFsRelationProvider have to be nullable? Thanks!

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.

5 participants