cloud-fan commented on code in PR #58877:
URL: https://github.com/apache/spark/pull/58877#discussion_r4407291392
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/python/HybridQueue.scala:
##########
@@ -38,6 +38,14 @@ trait Queue[T] {
def add(item: T): Boolean
def remove(): T
def close(): Unit
+
+ /**
+ * Releases any resources the queue only needs while it is being written to,
after which the
+ * queue accepts no further items but can still be read. Called once a queue
is known to be
Review Comment:
**Nit (P3):** This Scaladoc states "after which the queue accepts no further
items but can still be read" as a guarantee of every `Queue`, but the default
added here is a no-op, and `InMemoryRowQueue` inherits it: its `add()` keeps
returning true after `closeOutputStream()` while the page has room. Only
`DiskRowQueue` enforces the stated behavior, by nulling `out` so that `add()`
returns false. Nothing misbehaves today, because the method is only called on
`DiskRowQueue` instances (from `spill()` and from `DiskRowQueue.remove()`), but
a future caller that uses it to seal an arbitrary queue would be misled.
Consider stating it as a caller obligation and scoping the rejection to
implementations that release write-side state, for example:
```scala
/**
* Signals that nothing more will be added to this queue, so it can
release any resources it
* only needs while being written to. A disk-backed queue then closes its
output stream and
* rejects further items, but can still be read. Called once a queue is
known to be complete,
* so that a disk-backed queue does not hold an output stream and its
buffer open until its
* first read. The default is a no-op, for queues that hold nothing
write-only.
*/
```
**Verification:**
- **Inspection:** Check that the reworded trait documentation is consistent
with both the inherited no-op used by in-memory queues and the disk-backed
override, and that it frames 'no further items' as a caller obligation or an
implementation-specific guarantee rather than an unconditional trait
postcondition.
--
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]
