[GitHub] flink pull request: FLINK-3657: Change access of DataSetUtils.coun...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/flink/pull/1829


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: FLINK-3657: Change access of DataSetUtils.coun...

[fhueske]

Github user fhueske commented on the pull request:

https://github.com/apache/flink/pull/1829#issuecomment-210647072
  
Thanks for the update @smarthi. Will merge 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: FLINK-3657: Change access of DataSetUtils.coun...

[hsaputra]

Github user hsaputra commented on the pull request:

https://github.com/apache/flink/pull/1829#issuecomment-210080951
  
I think it will go to 1.1.x release. Need some clean up to isolate the 
changes needed.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: FLINK-3657: Change access of DataSetUtils.coun...

[andrewpalumbo]

Github user andrewpalumbo commented on the pull request:

https://github.com/apache/flink/pull/1829#issuecomment-210057521
  
Is it possible to get this merged into the 1,0.2 release?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: FLINK-3657: Change access of DataSetUtils.coun...

[smarthi]

GitHub user smarthi reopened a pull request:

https://github.com/apache/flink/pull/1829

FLINK-3657: Change access of DataSetUtils.countElements() to 'public'



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

$ git pull https://github.com/smarthi/flink Flink-3657

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

https://github.com/apache/flink/pull/1829.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 #1829


commit a3e33aa7bc8fad09a122490a13d294210df5e186
Author: smarthi 
Date:   2016-03-23T06:28:28Z

FLINK-3657: Change access of DataSetUtils.countElements() to 'public'




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: FLINK-3657: Change access of DataSetUtils.coun...

[hsaputra]

Github user hsaputra commented on the pull request:

https://github.com/apache/flink/pull/1829#issuecomment-205001401
  
+1 for merging to master. 

But would love to isolate the changes to exclude refactoring changes if 
possible


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: FLINK-3657: Change access of DataSetUtils.coun...

[uce]

Github user uce commented on the pull request:

https://github.com/apache/flink/pull/1829#issuecomment-204931011
  
@smarthi I think it's not too specific at all since its part of 
`DataSetUtils` and not `DataSet`. Let's merge the changes to master for `1.1`.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: FLINK-3657: Change access of DataSetUtils.coun...

[smarthi]

Github user smarthi commented on the pull request:

https://github.com/apache/flink/pull/1829#issuecomment-204762444
  
Closing this PR without merging as its "way too specific", thanks for all 
the 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: FLINK-3657: Change access of DataSetUtils.coun...

[smarthi]

Github user smarthi closed the pull request at:

https://github.com/apache/flink/pull/1829


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: FLINK-3657: Change access of DataSetUtils.coun...

[hsaputra]

Github user hsaputra commented on the pull request:

https://github.com/apache/flink/pull/1829#issuecomment-204614791
  
@mbalassi : There is already request from @zentol and me to clean up the 
PR, so would love to get this merge excluding the refactor parts. Thanks.

I think we should merge this to master and 1.1.0, the question is whether 
this should go to 1.0.1 release, which @fhueske and @uce think should not.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: FLINK-3657: Change access of DataSetUtils.coun...

[mbalassi]

Github user mbalassi commented on the pull request:

https://github.com/apache/flink/pull/1829#issuecomment-204587443
  
I think @smarthi has a point that the functionality is a nice fit for 
`DataSetUtils` and he has properly added it to the scala API and covered the 
tests.
It is true that it has some minor refactoring obfuscating the core PR, but 
besides that LGTM. If noone thinks otherwise I would merge this Tuesday morning 
with optionally excluding the refactor parts if it is considered that 
bothersome.
Unfortunately for the Mahout release I think the policy is as @fhueske 
suggests that we should reserve new features for the next, 1.1.0 release line.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: FLINK-3657: Change access of DataSetUtils.coun...

[hsaputra]

Github user hsaputra commented on the pull request:

https://github.com/apache/flink/pull/1829#issuecomment-201568146
  
@smarthi Could you remove some of the refactoring or clean up changes 
included with this PR?

It can be added as separate PR please. 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: FLINK-3657: Change access of DataSetUtils.coun...

[hsaputra]

Github user hsaputra commented on the pull request:

https://github.com/apache/flink/pull/1829#issuecomment-201567738
  
If it is on the Dataset or any extendible public APIs, I would be worried. 
The change was to expose private to public without modifying existing behavior. 
Would be ok from my side.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: FLINK-3657: Change access of DataSetUtils.coun...

[andrewpalumbo]

Github user andrewpalumbo commented on the pull request:

https://github.com/apache/flink/pull/1829#issuecomment-201507914
  
I've actually come across several use cases for this when Implementing a 
blockified matrix multiplication operator.  Exposing this method as public 
would be very helpful. It would be great to have in your 1.0.1 release.  
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: FLINK-3657: Change access of DataSetUtils.coun...

[smarthi]

Github user smarthi commented on the pull request:

https://github.com/apache/flink/pull/1829#issuecomment-200848523
  
@uce @fhueske to add more context to this PR, we r in the final stretch of 
a planned 0.12.0 Mahout release that adds Flink as a distributed engine for 
Samsara linear algebra. I will be demoing the Mahout-Flink integration at my 
talk in ApacheCon, Vancouver, May 9-11. If this can't make it in 1.0.1, I guess 
we need to go with a redundant clone in Mahout until this becomes available in 
a future Flink release and want to avoid that.   


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: FLINK-3657: Change access of DataSetUtils.coun...

[fhueske]

Github user fhueske commented on the pull request:

https://github.com/apache/flink/pull/1829#issuecomment-200804264
  
@uce, API stability is not an issue. The method was `private` before and 
part of the accessible API. IMO, the question is rather whether we want to add 
new features in a bugfix release.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: FLINK-3657: Change access of DataSetUtils.coun...

[uce]

Github user uce commented on the pull request:

https://github.com/apache/flink/pull/1829#issuecomment-200803091
  
As a side note (I didn't look at the code changes): We can't rename the 
method name for `1.0.1` as `DataSetUtils` is annotated with `PublicEvolving`, 
meaning that we are only allowed to change between minor versions, that is for 
`1.1.0`.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: FLINK-3657: Change access of DataSetUtils.coun...

[smarthi]

Github user smarthi commented on the pull request:

https://github.com/apache/flink/pull/1829#issuecomment-200551422
  
Not sure as to why u think this is "way to specific", this is a very 
convenient feature to have when dealing with Distributed Row Matrix (DRM) 
blocks for distributed Machine Learning computations. We found ourselves 3 
different variants doing the same thing and Flink happened to have one that 
directly did it for Flink's datasets. Hence this PR.

@fhueske thanks for your feedback, it would be great to have this in the 
planned 1.0.1 bug release. 


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: FLINK-3657: Change access of DataSetUtils.coun...

[zentol]

Github user zentol commented on the pull request:

https://github.com/apache/flink/pull/1829#issuecomment-200418755
  
Please stop force pushing for squashed changes, it obfuscates which 
comments you addressed, forcing us to through the _entire _PR again.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: FLINK-3657: Change access of DataSetUtils.coun...

[andrewpalumbo]

Github user andrewpalumbo commented on the pull request:

https://github.com/apache/flink/pull/1829#issuecomment-200413224
  
This method would be very useful to us, e.g., when assigning ordinal 
indices to the elements of a `DataSet` from an external mapping.  I would not 
think that this is too specific a use 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: FLINK-3657: Change access of DataSetUtils.coun...

[fhueske]

Github user fhueske commented on the pull request:

https://github.com/apache/flink/pull/1829#issuecomment-200244139
  
I'm not sure if it is too specific. The fact that somebody reimplemented 
the functionality hints that it might be useful. 
However, the name would need to be changed to something like 
`countElementsPerPartition()`, IMO.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: FLINK-3657: Change access of DataSetUtils.coun...

[zentol]

Github user zentol commented on the pull request:

https://github.com/apache/flink/pull/1829#issuecomment-200215310
  
I think this method's use-case is **way** too specific to offer it as part 
of the API.

You also touch 3 files purely for formatting.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] flink pull request: FLINK-3657: Change access of DataSetUtils.coun...

[smarthi]

GitHub user smarthi opened a pull request:

https://github.com/apache/flink/pull/1829

FLINK-3657: Change access of DataSetUtils.countElements() to 'public'



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

$ git pull https://github.com/smarthi/flink Flink-3657

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

https://github.com/apache/flink/pull/1829.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 #1829


commit d5ba6d90d3cdd6b93f962f2d9226e443810b08b0
Author: smarthi 
Date:   2016-03-23T06:28:28Z

FLINK-3657: Change access of DataSetUtils.countElements() to 'public'




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---