HeartSaVioR commented on code in PR #58191:
URL: https://github.com/apache/spark/pull/58191#discussion_r3847705791
##########
python/pyspark/sql/datasource_internal.py:
##########
@@ -162,11 +162,10 @@ def getCache(self, start: dict, end: dict) ->
Optional[Iterator[Tuple]]:
it = chain(*entries)
return it
- def read(
- self,
- input_partition: SimpleInputPartition, # type: ignore[override]
- ) -> Iterator[Tuple]:
- return self.simple_reader.readBetweenOffsets(input_partition.start,
input_partition.end)
+ def read(self, partition: InputPartition) -> Iterator[Tuple]:
+ # partitions() only yields SimpleInputPartition. Cast keeps the ABC
override valid.
+ simple_partition = cast(SimpleInputPartition, partition)
Review Comment:
I see there was a thread about whether having explicit runtime check or not.
(isinstance)
Let's say we don't pass SimpleInputPartition in any reason - cast will raise
an error, do I understand correctly? Does cast give the error message (as an
internal error) clearer than isinstance, so that we can indicate and address
the runtime error properly?
Also, if the prior code is able to give the static type check with
SimpleInputPartition, that is a trade-off worth thinking. I agree the code is
somehow breaking ABC but python's abstraction isn't as strong as strongly typed
language, which is what I called as trade-off.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
