Skip to content

Conversation

@amaliujia
Copy link
Contributor

@amaliujia amaliujia commented Oct 17, 2022

What changes were proposed in this pull request?

The mypy check in master seems to be broken:

Screen Shot 2022-10-17 at 1 09 13 PM

(see it on #38279 and #38275, also reproducible locally).

This PR propose to remove the relevant type: ignore comments.

Why are the changes needed?

Fix python lint check.

Does this PR introduce any user-facing change?

No

How was this patch tested?

UT

@amaliujia amaliujia changed the title [DO NOT MERGE] Test Python Lint [SPARK-40796][BUILD][FOLLOW-UP] Fix unused "type: ignore" comment Oct 17, 2022
@amaliujia amaliujia marked this pull request as ready for review October 17, 2022 20:13
@amaliujia
Copy link
Contributor Author

@HyukjinKwon @zhengruifeng

measure.function.arguments.append( # type: ignore[attr-defined]
cast(Expression, exp).to_plan(session)
)
measure.function.arguments.append(cast(Expression, exp).to_plan(session))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is formatted by the python formatter.

@amaliujia
Copy link
Contributor Author

Screen Shot 2022-10-17 at 3 33 02 PM

Interesting. The ignore comment was actually useful. I am confused now on how to fix that.

@amaliujia
Copy link
Contributor Author

ah I see it is now:

The python code needs to be updated because of the proto change.

@amaliujia amaliujia changed the title [SPARK-40796][BUILD][FOLLOW-UP] Fix unused "type: ignore" comment [SPARK-40796][BUILD][FOLLOW-UP] Fix Mypy check on unused "type: ignore" Oct 17, 2022
@amaliujia
Copy link
Contributor Author

Yes. This issue is fixed. Waiting for tests passing.

@zhengruifeng
Copy link
Contributor

@amaliujia
Copy link
Contributor Author

amaliujia commented Oct 18, 2022

@zhengruifeng

I don't know how this is triggered. If you check this example PR: #38275. After rebasing it started to fail on this issue. That PR only touches two lines in python/pyspark/sql/connect/plan.py which are not relevant to any problematic lines reported by mypy.

Right now I can only suspect that it is triggered when a PR touches python/pyspark/sql/connect/plan.py. Seems that my four affected PR are in such case.

@amaliujia
Copy link
Contributor Author

The python code on Aggregate is stale anyway: they are calling non-existing fields already.

@HyukjinKwon
Copy link
Member

Merged to master.

SandishKumarHN pushed a commit to SandishKumarHN/spark that referenced this pull request Dec 12, 2022
### What changes were proposed in this pull request?

The mypy check in master seems to be broken:

<img width="676" alt="Screen Shot 2022-10-17 at 1 09 13 PM" src="https://user-images.githubusercontent.com/1938382/196273759-64372862-6caa-4a9e-b700-b665c7ff7e6c.png">

(see it on apache#38279 and apache#38275, also reproducible locally).

This PR propose to remove the relevant `type: ignore` comments.

### Why are the changes needed?

Fix python lint check.

### Does this PR introduce _any_ user-facing change?

No

### How was this patch tested?

UT

Closes apache#38287 from amaliujia/test_python_lint.

Authored-by: Rui Wang <[email protected]>
Signed-off-by: Hyukjin Kwon <[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.

3 participants