aglinxinyuan commented on PR #8132: URL: https://github.com/apache/texera/pull/8132#issuecomment-5479745842
Thanks for the review. Both comments are the same suggestion — wrap `exec.close()` in `try`/`finally` — at two sites. I've **refused both** and applied one substitute instead; pushed as `f27a7e16`. **Why refused.** `StableMergeSortOpExec.close()` is exactly `if (sortedBuckets != null) sortedBuckets.clear()` — it clears one private in-memory `ArrayBuffer`. The `OperatorExecutor` trait it implements has a no-op `close()`; there's no file handle, socket, thread or global registry. The exec is a per-test local, freshly constructed in each case, and the spec has no suite-level state and no `beforeEach`/`afterEach`. So "can leak executor resources" and "impact later tests in the same JVM" don't hold for this class — the buffer is collected either way. Separately: the file has **15 bare `exec.close()` sites and zero `try`/`finally`**. A diff-scoped review flagged 2 of the 4 this PR adds and didn't see the 11 identical ones already on `main`. Wrapping just these two would leave 13 bare siblings in the same file and imply to a reader that those are bugs. To be fair to the suggestion: applying it wouldn't have broken anything — coverage and the mutation kills are unaffected either way. This is a refusal on "the stated mechanism doesn't exist here, and it would introduce within-file inconsistency", not on risk. If you'd rather have the idiom, the right shape is all 15 sites in a separate commit. **What I did apply.** Digging into the helper the second comment targets turned up a real failure-path problem that isn't the one flagged: `rejects` is invoked twice from one test case (`AttributeType.ANY` and `LARGE_BINARY`), and ScalaTest's `intercept` failure text names neither the type nor the call site — so a regression in one probe was indistinguishable from the other. Added a `withClue` naming it. One subtlety worth recording: it has to be `attrType.name()`, not the interpolated enum. `AttributeType.toString` delegates to the `@JsonValue` getter, which returns `""` for `ANY`, so `$attrType` would have rendered the clue blank for exactly one of the two probes. Verified by temporarily replacing the `intercept` body so nothing was thrown — the report then read `attributeType=ANY: Expected exception ... but no exception was thrown`. That's one line of scope beyond what was asked, on an already-approved PR; happy to drop it if you'd prefer. It can't affect coverage or a kill — it decorates an unchanged assertion. 73/73 tests pass, `scalafmtCheck` and `scalafix --check` clean, production diff empty. -- 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]
