Skip to content

Conversation

@carylee
Copy link
Contributor

@carylee carylee commented Aug 31, 2021

What changes were proposed in this pull request?

Update both DataFrame.approxQuantile and DataFrameStatFunctions.approxQuantile to support overloaded definitions when multiple columns are supplied.

Why are the changes needed?

The current type hints don't support the multi-column signature, a form that was added in Spark 2.2 (see the approxQuantile docs.) This change was also introduced to pyspark-stubs (zero323/pyspark-stubs#552). @zero323 asked me to open a PR for the upstream change.

Does this PR introduce any user-facing change?

This change only affects type hints - it brings the approxQuantile type hints up to date with the actual code.

How was this patch tested?

Ran ./dev/lint-python.

@zero323
Copy link
Member

zero323 commented Aug 31, 2021

ok to test

@zero323
Copy link
Member

zero323 commented Aug 31, 2021

Thanks for your contribution @carylee. Could you check the error and follow the instructions there? Thanks!

@zero323
Copy link
Member

zero323 commented Sep 1, 2021

Could you also add [PYTHON] to the title (it should be [SPARK-36617][PYTHON] ...)?

@carylee
Copy link
Contributor Author

carylee commented Sep 1, 2021

I'm not sure what the problems is. My branch is in sync with upstream/master and Github actions are enabled (see below):

Screen Shot 2021-08-31 at 4 39 36 PM

@carylee carylee changed the title [SPARK-36617] Fix type hints for approxQuantile to support multi-column version [SPARK-36617][PYTHON] Fix type hints for approxQuantile to support multi-column version Sep 1, 2021
@zero323
Copy link
Member

zero323 commented Sep 1, 2021

Could you check your actions tab (https://github.com/carylee/spark/actions) directly (you might see something like described in this comment)?

@carylee carylee requested a review from zero323 September 1, 2021 19:50
@zero323
Copy link
Member

zero323 commented Sep 1, 2021

ok to test

Copy link
Member

@zero323 zero323 left a comment

Choose a reason for hiding this comment

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

LGTM, subject to passing Jenkins tests.

@zero323
Copy link
Member

zero323 commented Sep 1, 2021

test this please

@carylee
Copy link
Contributor Author

carylee commented Sep 2, 2021

Copy link
Member

@HyukjinKwon HyukjinKwon left a comment

Choose a reason for hiding this comment

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

LGTM2. I will leave it to @zero323.

@HyukjinKwon
Copy link
Member

ok to test

@HyukjinKwon
Copy link
Member

I think it's good to go and merge since GitHub Actions passed. This change shouldn't break any of tests anyway.

@zero323 zero323 closed this in 37f5ab0 Sep 2, 2021
zero323 pushed a commit that referenced this pull request Sep 2, 2021
…multi-column version

### What changes were proposed in this pull request?
Update both `DataFrame.approxQuantile` and `DataFrameStatFunctions.approxQuantile` to support overloaded definitions when multiple columns are supplied.

### Why are the changes needed?
The current type hints don't support the multi-column signature, a form that was added in Spark 2.2 (see [the approxQuantile docs](https://spark.apache.org/docs/latest/api/python/reference/api/pyspark.sql.DataFrame.approxQuantile.html).) This change was also introduced to pyspark-stubs (zero323/pyspark-stubs#552). zero323 asked me to open a PR for the upstream change.

### Does this PR introduce _any_ user-facing change?
This change only affects type hints - it brings the `approxQuantile` type hints up to date with the actual code.

### How was this patch tested?
Ran `./dev/lint-python`.

Closes #33880 from carylee/master.

Authored-by: Cary Lee <[email protected]>
Signed-off-by: zero323 <[email protected]>
(cherry picked from commit 37f5ab0)
Signed-off-by: zero323 <[email protected]>
zero323 pushed a commit that referenced this pull request Sep 2, 2021
…multi-column version

### What changes were proposed in this pull request?
Update both `DataFrame.approxQuantile` and `DataFrameStatFunctions.approxQuantile` to support overloaded definitions when multiple columns are supplied.

### Why are the changes needed?
The current type hints don't support the multi-column signature, a form that was added in Spark 2.2 (see [the approxQuantile docs](https://spark.apache.org/docs/latest/api/python/reference/api/pyspark.sql.DataFrame.approxQuantile.html).) This change was also introduced to pyspark-stubs (zero323/pyspark-stubs#552). zero323 asked me to open a PR for the upstream change.

### Does this PR introduce _any_ user-facing change?
This change only affects type hints - it brings the `approxQuantile` type hints up to date with the actual code.

### How was this patch tested?
Ran `./dev/lint-python`.

Closes #33880 from carylee/master.

Authored-by: Cary Lee <[email protected]>
Signed-off-by: zero323 <[email protected]>
(cherry picked from commit 37f5ab0)
Signed-off-by: zero323 <[email protected]>
@zero323
Copy link
Member

zero323 commented Sep 2, 2021

Merged to master, branch-3.2 and branch-3.1.

@zero323
Copy link
Member

zero323 commented Sep 2, 2021

Thanks everyone!

flyrain pushed a commit to flyrain/spark that referenced this pull request Sep 21, 2021
…multi-column version

### What changes were proposed in this pull request?
Update both `DataFrame.approxQuantile` and `DataFrameStatFunctions.approxQuantile` to support overloaded definitions when multiple columns are supplied.

### Why are the changes needed?
The current type hints don't support the multi-column signature, a form that was added in Spark 2.2 (see [the approxQuantile docs](https://spark.apache.org/docs/latest/api/python/reference/api/pyspark.sql.DataFrame.approxQuantile.html).) This change was also introduced to pyspark-stubs (zero323/pyspark-stubs#552). zero323 asked me to open a PR for the upstream change.

### Does this PR introduce _any_ user-facing change?
This change only affects type hints - it brings the `approxQuantile` type hints up to date with the actual code.

### How was this patch tested?
Ran `./dev/lint-python`.

Closes apache#33880 from carylee/master.

Authored-by: Cary Lee <[email protected]>
Signed-off-by: zero323 <[email protected]>
(cherry picked from commit 37f5ab0)
Signed-off-by: zero323 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants