Skip to content

[SPARK-36906][PYTHON] Inline type hints for conf.py and observation.py in python/pyspark/sql#34159

Closed
xinrong-meng wants to merge 2 commits intoapache:masterfrom
xinrong-meng:inline_conf_observation
Closed

[SPARK-36906][PYTHON] Inline type hints for conf.py and observation.py in python/pyspark/sql#34159
xinrong-meng wants to merge 2 commits intoapache:masterfrom
xinrong-meng:inline_conf_observation

Conversation

@xinrong-meng
Copy link
Member

@xinrong-meng xinrong-meng commented Sep 30, 2021

What changes were proposed in this pull request?

Inline type hints for conf.py and observation.py in python/pyspark/sql.

Why are the changes needed?

Currently, there is type hint stub files (*.pyi) to show the expected types for functions, but we can also take advantage of static type checking within the functions by inlining the type hints.

Does this PR introduce any user-facing change?

No.

It has a DOC typo fix:
Metrics are aggregation expressions, which are applied to the DataFrame while **is** is being
is changed to
Metrics are aggregation expressions, which are applied to the DataFrame while **it** is being

How was this patch tested?

Existing test.

@SparkQA
Copy link

SparkQA commented Sep 30, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/48284/

@SparkQA
Copy link

SparkQA commented Sep 30, 2021

Test build #143773 has finished for PR 34159 at commit 8c039c0.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Oct 1, 2021

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/48284/

@xinrong-meng
Copy link
Member Author

xinrong-meng commented Oct 1, 2021


@since(2.0)
def set(self, key, value):
def set(self, key: str, value: str) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

Seems like this can be more permissive on value type (Union[str, int, bool]?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

@ueshin ueshin left a comment

Choose a reason for hiding this comment

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

LGTM.

@ueshin
Copy link
Member

ueshin commented Oct 4, 2021

Thanks! merging to master.

@ueshin ueshin closed this in 8181260 Oct 4, 2021
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.

5 participants