You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by aa8y <gi...@git.apache.org> on 2016/04/22 01:47:41 UTC
[GitHub] spark pull request: [SPARK-11962] (Not merge ready) Added function...
GitHub user aa8y opened a pull request:
https://github.com/apache/spark/pull/12587
[SPARK-11962] (Not merge ready) Added functions `attempt` and `getOption`.
## What changes were proposed in this pull request?
Add methods to get `Try` and `Option` types from a `Row`. The code is *not merge ready*. But I was not sure how I could get feedback without a PR. I've added two major methods to Row, `attempt`
and `getOption`. The former returns a `Try` while the latter returns an `Option`.
* `attempt` was added after a comment was made in PR #10247, where it was suggested I not return None when certain exceptions are thrown. But in my opinion, throwing exceptions from a function which returns an `Option` is not a good use case.
* I am not in love with the name `attempt`. Would welcome suggestions. I wanted to use `try` but it's a keyword.
* In `RowTest`, the tests testing for `ClassCastException` fail. I am not sure what I am doing wrong in the code here. Would appreciate feedback.
## How was this patch tested?
Tests have been added to `RowTest` to test the newly added methods.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/aa8y/spark master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/12587.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 #12587
----
commit 4348492a2d8627869ca7a88fbca0d0e7ee835921
Author: Arun Allamsetty <ar...@instructure.com>
Date: 2016-04-21T19:14:33Z
[SPARK-11962] Add optional get methods to Row.
commit 2731613cd6b58051b80d8d035e57282b2ce0257c
Author: Arun Allamsetty <ar...@instructure.com>
Date: 2016-04-21T23:34:00Z
[SPARK-11962] (Not merge ready, see commit message) Added functions
`attempt` and `getOption`.
The code is not merge ready. But I was not sure how I could get
feedback without a PR. I've added two major methods to Row, `attempt`
and `getOption`. The former returns a Try while the latter returns an
Option.
- `attempt` was added after a comment was made in PR #10247, where it
was suggested I not return None when certain exceptions are thrown. But
in my opinion, throwing exceptions from a function which returns an
Option is not a good use case.
- I am not in love with the name `attempt`. Would welcome suggestions. I
wanted to use `try` but it's a keyword.
- In `RowTest`, the tests testing for `ClassCastException` fail. I am
not sure what I am doing wrong in the code here. Would appreciate
feedback.
----
---
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-11962] WIP: Added functions `attempt` a...
Posted by aa8y <gi...@git.apache.org>.
Github user aa8y commented on the pull request:
https://github.com/apache/spark/pull/12587#issuecomment-214869241
PR being closed as it was pointing to my `master`. The new PR is #12708.
---
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-11962] WIP: Added functions `attempt` a...
Posted by aa8y <gi...@git.apache.org>.
Github user aa8y commented on the pull request:
https://github.com/apache/spark/pull/12587#issuecomment-214869110
PR being closed as it was pointing to my `master`. The new PR is #12708.
---
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-11962] (Not merge ready) Added function...
Posted by aa8y <gi...@git.apache.org>.
Github user aa8y commented on the pull request:
https://github.com/apache/spark/pull/12587#issuecomment-213164874
@holdenk: I will keep in mind your suggestions in the future. The change in the .gitignore was that I sorted the contents. I can replace it with what's on master in the next commit. However please ignore the .gitignore file for now. If you could give some feedback on the other stuff, that would be great. Thanks!
---
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-11962] (Not merge ready) Added function...
Posted by aa8y <gi...@git.apache.org>.
Github user aa8y commented on the pull request:
https://github.com/apache/spark/pull/12587#issuecomment-214365652
@srowen I would really appreciate if you could review this WIP PR as well. I'll close it of course once I get all the feedback. On @holdenk's suggestion I did send an email to `dev@spark` but haven't received any responses. Might have been a weekend thing though :)
---
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-11962] (Not merge ready) Added function...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/12587#issuecomment-213163254
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 pull request: [SPARK-11962] (Not merge ready) Added function...
Posted by hvanhovell <gi...@git.apache.org>.
Github user hvanhovell commented on the pull request:
https://github.com/apache/spark/pull/12587#issuecomment-213947623
@aa8y what is the use case for 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: [SPARK-11962] (Not merge ready) Added function...
Posted by holdenk <gi...@git.apache.org>.
Github user holdenk commented on the pull request:
https://github.com/apache/spark/pull/12587#issuecomment-213163383
@aa8y So generally [WIP] is how people will tag a PR as something which is a work in progress but not ready to be merged. You can also ask for feedback on the dev@ list or by pinging individuals in your JIRA who might have opnionins.
I haven't had a chance to look through the code - but it seems like there is a really big change to the gitignore file which is unrellated and you likely don't need for the intended purpose of 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-11962] WIP: Added functions `attempt` a...
Posted by aa8y <gi...@git.apache.org>.
Github user aa8y closed the pull request at:
https://github.com/apache/spark/pull/12587
---
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-11962] (Not merge ready) Added function...
Posted by aa8y <gi...@git.apache.org>.
Github user aa8y commented on the pull request:
https://github.com/apache/spark/pull/12587#issuecomment-213983438
@hvanhovell The end use case for both `attempt` and `getOption` is to get values out of a row object without throwing an exception. However, in one of my previous PRs, #10247 (which didn't have an `attempt` method), it was pointed out by @marmbrus that the `getOption` method caught exceptions and converted them to `None`s. His PoV was that exceptions should not be caught as that hid important information. But since the whole reason of `getOption` was to *not* throw exceptions, I introduced the `attempt` method which returns `Success` and `Failure` objects, which do not lose information and `getOption`, which does and returns a `Some` or `None`.
---
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