aglinxinyuan commented on code in PR #7883:
URL: https://github.com/apache/texera/pull/7883#discussion_r3838001265
##########
common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/dictionary/DictionaryMatcherOpExecSpec.scala:
##########
@@ -200,4 +200,142 @@ class DictionaryMatcherOpExecSpec extends AnyFlatSpec
with BeforeAndAfter {
assert(opExec.tokenizedDictionaryEntries == null)
assert(opExec.luceneAnalyzer == null)
}
+
+ // NOTE: every test below must stay BELOW "close properly". That test asserts
+ // `tokenizedDictionaryEntries == null` on whatever executor the preceding
test
+ // left in the shared `opExec` var, and close() only *clears* that buffer --
it
+ // never nulls it. Inserting a CONJUNCTION_INDEXBASED test above it would
make
+ // it fail.
Review Comment:
Good catch, and the diagnosis is exactly right — fixed in 5974ffa.
Documenting an order-dependence I could just remove was the wrong call.
`close properly` now builds its own SCANBASED executor instead of reading
whatever the previous test left in the shared `opExec` var, and the NOTE is
gone.
One deliberate deviation from the suggestion: I did not assert "either null
or empty", because a disjunction over the two outcomes pins neither. Instead
each arm of `close()`'s guard is pinned exactly, by a test that owns its own
executor:
- **SCANBASED** — `open()` never allocates the buffer, so
`tokenizedDictionaryEntries == null` is that path's own contract rather than a
leftover. That is `close properly`.
- **CONJUNCTION_INDEXBASED** — the buffer exists and is cleared but not
nulled. Already pinned by "empty the tokenized dictionary on close", which this
PR added: `nonEmpty` before, `isEmpty` after, and explicitly no claim about
null.
`close properly` also now closes twice, which keeps the idempotence that the
old ordering was exercising by accident (the preceding test had already closed
that executor).
Verified in both directions rather than just asserting it: inserting a
`CONJUNCTION_INDEXBASED` test directly above `close properly` — the edit the
NOTE forbade — fails at `abd61dc` with `ListBuffer() did not equal null` at
line 208, and passes at `5974ffa`. So the hazard was real, and it is now gone.
`DictionaryMatcherOpExec` coverage is unchanged — 49/50 lines, 1 missed arm
of 26, line 62's structurally unreachable arm being the only partial — measured
with the same scoped `WorkflowOperator/jacoco` on both commits, so the coverage
table above still holds. Test counts are also unchanged at 20 and 24, since
this modifies a test rather than adding one.
--
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]
