You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by pfcoperez <gi...@git.apache.org> on 2017/02/23 13:41:55 UTC
[GitHub] spark pull request #17040: CatalogTableType constructor access modifier rend...
GitHub user pfcoperez opened a pull request:
https://github.com/apache/spark/pull/17040
CatalogTableType constructor access modifier renders useless when case class factory is public
## What changes were proposed in this pull request?
`CatalogTableType` seems to implement an union data type enumerating all possible catalog table types.
It is built using a case class with a single value attribute "name". Wisely, it seems to be intended to be set only by the case class private constructor within the companion object with the idea of users not being able to create `CatalogTableType` instances but using those enumerated within that object.
However, making the case class constructor private does't forbid users creating whichever instances they want to create since the case class factory is still public. e.g:
```
scala> case class C private(x: Int)
defined class C
scala> C(1)
res0: C = C(1) // Should users be able to do this?
scala> new C(1) //This is the expected behaviour.
<console>:13: error: constructor C in class C cannot be accessed in object $iw
new C(1)
```
This PR makes `CatalogTableType` a regular class, if matching the name was necessary, its companion object could implement `unapply`. Alternatives ways of solving this could be just using Scala's enumeration or model `CatalogTableType` using algebraic data types based on extending an interface, e.g:
```
object CatalogTableType {
object EXTERNAL extends CatalogTableType { val name = "EXTERNAL" }
object INTERNAL extends CatalogTableType { val name = "INTERNAL" }
object VIEW extends CatalogTableType { val name = "VIEW" }
}
```
## How was this patch tested?
This PR does not add new functionality and has thus been tested by your current test suite.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/pfcoperez/spark catalogtabletpe_private_factory
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/17040.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 #17040
----
commit daae901d331293144af8dd125ea59892f5ec3c3d
Author: Pablo Fco. P�rez Hidalgo <pf...@stratio.com>
Date: 2017-02-23T12:18:06Z
Even though the case class constructor has been made private, the default `apply` factory for the case class remains public so this constraint is useless since anyone can use that factory to create customized instances.
----
---
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 #17040: CatalogTableType constructor access modifier rend...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/17040
---
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 issue #17040: CatalogTableType constructor access modifier renders use...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17040
Can one of the admins verify this patch?
---
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 issue #17040: CatalogTableType constructor access modifier renders use...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/17040
I guess this is meant to be internal API - https://github.com/apache/spark/blob/f041e55eefe1d8a995fed321c66bccbd8b8e5255/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/package.scala#L21-L22
Could you maybe open a JIRA and describe usecase to persuade make this a proper external API maybe?
Also, it'd be great if this PR follows http://spark.apache.org/contributing.html (e.g., [SPARK-XXXX] in the PR title with a JIRA).
---
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