You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "imvtsl (via GitHub)" <gi...@apache.org> on 2024/10/28 08:14:22 UTC
[PR] Add Sample functionality in DataFrame. [spark-connect-go]
imvtsl opened a new pull request, #84:
URL: https://github.com/apache/spark-connect-go/pull/84
Signed-off-by: Vatsal <va...@gmail.com>
### What changes were proposed in this pull request?
This PR adds support for the `sample` DF function.
### Why are the changes needed?
Compatibility
### Does this PR introduce _any_ user-facing change?
new DF functions
### How was this patch tested?
Added e2e tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] Add Sample functionality in DataFrame. [spark-connect-go]
Posted by "grundprinzip (via GitHub)" <gi...@apache.org>.
grundprinzip commented on code in PR #84:
URL: https://github.com/apache/spark-connect-go/pull/84#discussion_r1835714067
##########
spark/sql/dataframe.go:
##########
@@ -148,6 +148,14 @@ type DataFrame interface {
Rollup(ctx context.Context, cols ...column.Convertible) *GroupedData
// SameSemantics returns true if the other DataFrame has the same semantics.
SameSemantics(ctx context.Context, other DataFrame) (bool, error)
+ // Sample samples a data frame without replacement and random seed.
+ Sample(ctx context.Context, fraction float64) (DataFrame, error)
+ // SampleWithReplacement samples a data frame with random seed and with/without replacement.
+ SampleWithReplacement(ctx context.Context, withReplacement bool, fraction float64) (DataFrame, error)
+ // SampleSeed samples a data frame without replacement and given seed.
+ SampleSeed(ctx context.Context, fraction float64, seed int64) (DataFrame, error)
Review Comment:
the parameter `withReplacement` in the other cases already contains the "with" part in the name.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] Add Sample functionality in DataFrame. [spark-connect-go]
Posted by "grundprinzip (via GitHub)" <gi...@apache.org>.
grundprinzip closed pull request #84: Add Sample functionality in DataFrame.
URL: https://github.com/apache/spark-connect-go/pull/84
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] Add Sample functionality in DataFrame. [spark-connect-go]
Posted by "imvtsl (via GitHub)" <gi...@apache.org>.
imvtsl commented on PR #84:
URL: https://github.com/apache/spark-connect-go/pull/84#issuecomment-2440920523
Hi @grundprinzip
I added functionality for the `Sample` function.
While working on this PR, I noticed that existing functions don't currently support optional arguments. For instance, in the [Show](https://github.com/apache/spark-connect-go/blob/a8fccbf4619bb1a59fe87d9a1fcd7361e26142ab/spark/sql/dataframe.go#L420) function, the `numRows` argument is not optional. However, in [PySpark](https://spark.apache.org/docs/latest/api/python/reference/pyspark.sql/api/pyspark.sql.DataFrame.show.html), it is optional. I observed a similar pattern in other functions as well.
In PySpark’s implementation of [Sample](https://spark.apache.org/docs/latest/api/python/_modules/pyspark/sql/dataframe.html#DataFrame.sample), the `withReplacement` and `seed` arguments are optional. Following that model, I used pointers for these arguments in my implementation (so they can be set to nil). Please let me know if this approach is correct or if there's something I might be overlooking. This is my first contribution to this repository, and I'm new to working with Spark.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] Add Sample functionality in DataFrame. [spark-connect-go]
Posted by "grundprinzip (via GitHub)" <gi...@apache.org>.
grundprinzip commented on code in PR #84:
URL: https://github.com/apache/spark-connect-go/pull/84#discussion_r1825931136
##########
spark/sql/dataframe.go:
##########
@@ -148,6 +148,8 @@ type DataFrame interface {
Rollup(ctx context.Context, cols ...column.Convertible) *GroupedData
// SameSemantics returns true if the other DataFrame has the same semantics.
SameSemantics(ctx context.Context, other DataFrame) (bool, error)
+ // Sample samples a data frame
+ Sample(ctx context.Context, withReplacement *bool, fraction float64, seed *int64) (DataFrame, error)
Review Comment:
I believe a more Golang idiomatic approach would be to have multiple methods here
```
Sample(ctx) (DataFrame, error)
SampleWithFraction(ctx, fraction) (DataFrame, error)
SampleWithReplacementFractionSeed(ctx, withReplacement, fraction, seed) (DataFrame, error)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] Add Sample functionality in DataFrame. [spark-connect-go]
Posted by "imvtsl (via GitHub)" <gi...@apache.org>.
imvtsl commented on code in PR #84:
URL: https://github.com/apache/spark-connect-go/pull/84#discussion_r2002169879
##########
spark/sql/dataframe.go:
##########
@@ -148,6 +148,14 @@ type DataFrame interface {
Rollup(ctx context.Context, cols ...column.Convertible) *GroupedData
// SameSemantics returns true if the other DataFrame has the same semantics.
SameSemantics(ctx context.Context, other DataFrame) (bool, error)
+ // Sample samples a data frame without replacement and random seed.
+ Sample(ctx context.Context, fraction float64) (DataFrame, error)
+ // SampleWithReplacement samples a data frame with random seed and with/without replacement.
+ SampleWithReplacement(ctx context.Context, withReplacement bool, fraction float64) (DataFrame, error)
+ // SampleSeed samples a data frame without replacement and given seed.
+ SampleSeed(ctx context.Context, fraction float64, seed int64) (DataFrame, error)
Review Comment:
please refer [below](https://github.com/apache/spark-connect-go/pull/84#issuecomment-2734966459) .
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] Add Sample functionality in DataFrame. [spark-connect-go]
Posted by "grundprinzip (via GitHub)" <gi...@apache.org>.
grundprinzip commented on PR #84:
URL: https://github.com/apache/spark-connect-go/pull/84#issuecomment-2568270079
merging to master, thanks for your contribution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] Add Sample functionality in DataFrame. [spark-connect-go]
Posted by "imvtsl (via GitHub)" <gi...@apache.org>.
imvtsl commented on PR #84:
URL: https://github.com/apache/spark-connect-go/pull/84#issuecomment-2453114766
> While working on this PR, I noticed that existing functions don't currently support optional arguments. For instance, in the [Show](https://github.com/apache/spark-connect-go/blob/a8fccbf4619bb1a59fe87d9a1fcd7361e26142ab/spark/sql/dataframe.go#L420) function, the `numRows` argument is not optional. However, in [PySpark](https://spark.apache.org/docs/latest/api/python/reference/pyspark.sql/api/pyspark.sql.DataFrame.show.html), it is optional. I observed a similar pattern in other functions as well.
@grundprinzip
What are we planning to do about the DataFrame functions that are already implemented?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] Add Sample functionality in DataFrame. [spark-connect-go]
Posted by "grundprinzip (via GitHub)" <gi...@apache.org>.
grundprinzip commented on PR #84:
URL: https://github.com/apache/spark-connect-go/pull/84#issuecomment-2466764558
@imvtsl Golang does not have the concept of default parameters or method overloading. The idiomatic way of dealing with this is to provide different methods. Using an int pointer to indicate optionality is not common.
For example, `Show()` does not have optional parameters in spark-connect-go.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] Add Sample functionality in DataFrame. [spark-connect-go]
Posted by "grundprinzip (via GitHub)" <gi...@apache.org>.
grundprinzip commented on code in PR #84:
URL: https://github.com/apache/spark-connect-go/pull/84#discussion_r1835714005
##########
spark/sql/dataframe.go:
##########
@@ -148,6 +148,14 @@ type DataFrame interface {
Rollup(ctx context.Context, cols ...column.Convertible) *GroupedData
// SameSemantics returns true if the other DataFrame has the same semantics.
SameSemantics(ctx context.Context, other DataFrame) (bool, error)
+ // Sample samples a data frame without replacement and random seed.
+ Sample(ctx context.Context, fraction float64) (DataFrame, error)
+ // SampleWithReplacement samples a data frame with random seed and with/without replacement.
+ SampleWithReplacement(ctx context.Context, withReplacement bool, fraction float64) (DataFrame, error)
+ // SampleSeed samples a data frame without replacement and given seed.
+ SampleSeed(ctx context.Context, fraction float64, seed int64) (DataFrame, error)
Review Comment:
```suggestion
SampleWithSeed(ctx context.Context, fraction float64, seed int64) (DataFrame, error)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] Add Sample functionality in DataFrame. [spark-connect-go]
Posted by "imvtsl (via GitHub)" <gi...@apache.org>.
imvtsl commented on code in PR #84:
URL: https://github.com/apache/spark-connect-go/pull/84#discussion_r1826759752
##########
spark/sql/dataframe.go:
##########
@@ -148,6 +148,8 @@ type DataFrame interface {
Rollup(ctx context.Context, cols ...column.Convertible) *GroupedData
// SameSemantics returns true if the other DataFrame has the same semantics.
SameSemantics(ctx context.Context, other DataFrame) (bool, error)
+ // Sample samples a data frame
+ Sample(ctx context.Context, withReplacement *bool, fraction float64, seed *int64) (DataFrame, error)
Review Comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] Add Sample functionality in DataFrame. [spark-connect-go]
Posted by "imvtsl (via GitHub)" <gi...@apache.org>.
imvtsl commented on PR #84:
URL: https://github.com/apache/spark-connect-go/pull/84#issuecomment-2734966459
Late here, but I wanted to put forth my point:
> the parameter withReplacement in the other cases already contains the "with" part in the name.
I believe the original name SampleSeed better aligns with Go naming conventions and the existing API pattern. In the function SampleWithReplacement, the "With" is included because it directly corresponds to the boolean parameter withReplacement, which defines the sampling behavior.
In contrast, the SampleSeed function takes a seed parameter (not a withSeed), so adding "With" would introduce an unnecessary preposition that doesn't accurately describe the argument.
Go naming conventions generally favor concise, clear names that directly reflect their parameters and purpose. By following this convention, SampleSeed is consistent with both Go idioms and the naming pattern established by SampleWithReplacement.
Here is Google's style guide about naming conventions that you can reference: [https://google.github.io/styleguide/go/best-practices.html#naming](https://google.github.io/styleguide/go/best-practices.html#naming)
> @imvtsl Golang does not have the concept of default parameters or method overloading. The idiomatic way of dealing with this is to provide different methods. Using an int pointer to indicate optionality is not common.
>
> For example, Show() does not have optional parameters in spark-connect-go.
I understand that Go doesn't support default parameters or method overloading. However, I believe the current implementation of `Show()` might not be ideal.
In PySpark, the show() method supports optional parameters:
`n` (int, optional): Number of rows to show.
`truncate` (bool or int, optional): If True, truncates strings longer than 20 chars by default. If set to a positive integer, truncates long strings to that length and aligns cells right.
`vertical` (bool, optional): If True, prints output rows vertically (one line per column value).
However, the current implementation of `Show()` in `spark-connect-go` forces clients to provide all parameters, even if they just want to use default behavior. This isn't correct.
As you mentioned, the idiomatic approach in Go is to provide different methods instead of relying on optional parameters. Just like I did for the Sample function, we could have separate methods for `Show()` to support different use cases without forcing clients to pass all parameters.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] Add Sample functionality in DataFrame. [spark-connect-go]
Posted by "imvtsl (via GitHub)" <gi...@apache.org>.
imvtsl commented on code in PR #84:
URL: https://github.com/apache/spark-connect-go/pull/84#discussion_r2002169879
##########
spark/sql/dataframe.go:
##########
@@ -148,6 +148,14 @@ type DataFrame interface {
Rollup(ctx context.Context, cols ...column.Convertible) *GroupedData
// SameSemantics returns true if the other DataFrame has the same semantics.
SameSemantics(ctx context.Context, other DataFrame) (bool, error)
+ // Sample samples a data frame without replacement and random seed.
+ Sample(ctx context.Context, fraction float64) (DataFrame, error)
+ // SampleWithReplacement samples a data frame with random seed and with/without replacement.
+ SampleWithReplacement(ctx context.Context, withReplacement bool, fraction float64) (DataFrame, error)
+ // SampleSeed samples a data frame without replacement and given seed.
+ SampleSeed(ctx context.Context, fraction float64, seed int64) (DataFrame, error)
Review Comment:
please [refer](https://github.com/apache/spark-connect-go/pull/84#issuecomment-2734966459) below.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org