[GitHub] [spark] ByronHsu commented on a change in pull request #34411: [SPARK-37137][PYTHON] Inline type hints for python/pyspark/conf.py

[GitBox <gi...@apache.org>]

ByronHsu commented on a change in pull request #34411:
URL: https://github.com/apache/spark/pull/34411#discussion_r738138944



##########
File path: python/pyspark/conf.py
##########
@@ -178,9 +191,18 @@ def setAll(self, pairs):
             self.set(k, v)
         return self
 
-    def get(self, key, defaultValue=None):
+    @overload

Review comment:
       I add overload because If I only set the return value as Optional, https://github.com/apache/spark/blob/master/python/pyspark/sql/session.py#L613 will show error.

##########
File path: python/pyspark/conf.py
##########
@@ -195,21 +217,21 @@ def get(self, key, defaultValue=None):
             else:
                 return self._conf.get(key, defaultValue)
 
-    def getAll(self):
+    def getAll(self) -> Union[List[Tuple[str, str]], ItemsView[Any, Any]]:

Review comment:
       Is there a more clever way rather than specifying `ItemsView[Any, Any]`?

##########
File path: python/pyspark/conf.py
##########
@@ -133,28 +137,37 @@ def set(self, key, value):
             self._conf[key] = str(value)
         return self
 
-    def setIfMissing(self, key, value):
+    def setIfMissing(self, key: str, value: str) -> "SparkConf":
         """Set a configuration property, if not already set."""
         if self.get(key) is None:
             self.set(key, value)
         return self
 
-    def setMaster(self, value):
+    def setMaster(self, value: str) -> "SparkConf":
         """Set master URL to connect to."""
         self.set("spark.master", value)
         return self
 
-    def setAppName(self, value):
+    def setAppName(self, value: str) -> "SparkConf":
         """Set application name."""
         self.set("spark.app.name", value)
         return self
 
-    def setSparkHome(self, value):
+    def setSparkHome(self, value: str) -> "SparkConf":
         """Set path where Spark is installed on worker nodes."""
         self.set("spark.home", value)
         return self
 
-    def setExecutorEnv(self, key=None, value=None, pairs=None):
+    @overload
+    def setExecutorEnv(self, key: str, value: str) -> "SparkConf":
+        ...
+
+    @overload
+    def setExecutorEnv(self, *, pairs: List[Tuple[str, str]]) -> "SparkConf":
+        ...
+
+    # TODO
+    def setExecutorEnv(self, key=None, value=None, pairs=None):  # type: ignore[no-untyped-def]

Review comment:
       @xinrong-databricks I am not sure how to union the two overload functions.

##########
File path: python/pyspark/conf.py
##########
@@ -122,9 +126,9 @@ def __init__(self, loadDefaults=True, _jvm=None, _jconf=None):
             else:
                 # JVM is not created, so store data in self._conf first
                 self._jconf = None
-                self._conf = {}
+                self._conf = {}  # type: ignore[var-annotated]

Review comment:
       Should I ignore it?




-- 
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