This is an automated email from the ASF dual-hosted git repository.
github-merge-queue[bot] pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/texera.git
The following commit(s) were added to refs/heads/main by this push:
new fcfd112fbc test(workflow-operator): pin the filter predicate's
comparison arms (#7883)
fcfd112fbc is described below
commit fcfd112fbc8fd8c3aa59d77003debc881da0392f
Author: Xinyuan Lin <[email protected]>
AuthorDate: Sun Aug 23 08:07:40 2026 +0000
test(workflow-operator): pin the filter predicate's comparison arms (#7883)
### What changes were proposed in this PR?
Two operator-evaluation files whose gaps were entirely branch-arm
partials on already-executed lines. 33 tests → 44.
Measured with a full-module `WorkflowOperator/jacoco`, one fresh sbt JVM
per run, `rm -rf` on the jacoco dir between them, and the same
suite-name exclusion on both sides — `FileScanSourceOpExecSpec` aborts
in this worktree on a pre-existing Windows file lock, and since
sbt-jacoco runs unforked a failing test task yields *no report directory
at all*, not an all-zero one.
| File | Codecov | Branch arms |
|---|---|---|
| `DictionaryMatcherOpExec.scala` | 44/50 = 88.0% → **49/50 = 98.0%** |
8 missed → **1** |
| `FilterPredicate.java` | 54/60 = 90.0% → **56/60 = 93.3%** | 6 missed
→ **3** |
| **bundle** | 98/110 = 89.1% → **105/110 = 95.5%** | +10 arms |
JaCoCo line-hit was already 100% on the Scala file and 59/60 on the Java
one — every gap was a partial arm on a line that already executed, which
is exactly the case Codecov penalises and line-hit hides.
**This is +7 lines, and I would rather say so than dress it up.** What
earns the PR is the behaviour underneath.
### The behaviour was much less pinned than the percentages suggested
Two findings that are corrections to my own first draft, both material:
- **`FilterPredicate`'s comparison switch was barely constrained.** The
first draft called the file "essentially exhausted" — true of coverage,
false of behaviour. Four of its six comparison arms could be swapped for
a neighbouring arm and stay green against all 2314 module tests. Those
are now pinned.
- **`URL_STOP_WORDS_SET` had no proof it did anything.** The first draft
claimed its new test was "the first proof anywhere in the repo that that
list does anything at all". That was true of the single entry `"www"`
and nothing else — truncating the whole list to `List("www")` survived.
The test is now a loop that pins every entry.
### Verification
26 mutations re-derived from scratch, **25 killed by a named test with
its exact failure message, 1 equivalent mutant.**
The first draft reported no survivors. **Eleven semantic mutants were
alive on it** — five on `FilterPredicate`'s comparison switch, five on
`DictionaryMatcherOpExec` (comma delimiter, field case-folding, null
handling), and one that had slipped past both stop-word tests. All
eleven now die, each re-credited to the test that kills it.
The one mutant left alive drops a field from `hashCode`; killing it
would require asserting a specific hash value, so it is recorded as
equivalent rather than counted as a survivor.
### Corrections, including to a reviewer
- A reviewer suggested an operand-swap variant
(`Objects.equals(attribute, that.value)`) as a survivor for these
fixtures. **Wrong** — I ran it, and it is killed at HEAD by two
pre-existing tests, because the attribute name and the compared value
are distinguishable in those fixtures.
- The first draft's fallback estimate ("drop that one test and the
bundle falls to +6 lines / +8 arms") was off by one arm; the real
contribution of that test is 1 line and 3 arms.
- Its module-wide branch figure was off by one: the baseline is 1214 →
1203, of which −10 comes from these two files and −1 from an unrelated
file drifting.
- Its claim that two lines "report as fully covered despite
never-executed instructions" was true of one line, at baseline only.
- A judgement call flagged in the first draft no longer applies: the
test no longer pins `intercept[MatchError]`, so it does not cement
scalac's exception choice as the misconfiguration contract.
### A landmine first recorded, then removed after review
The first draft *documented* an order-dependence in a NOTE instead of
fixing it, and a review comment was right that removing it was the
better call. `5974ffa` removes it.
`DictionaryMatcherOpExecSpec`'s `opExec` is a shared `var`, and the
pre-existing "close properly" test asserted `tokenizedDictionaryEntries
== null` on whatever the *previous* test left behind. That held only
because the preceding test used `SUBSTRING`: `close()` clears
`tokenizedDictionaryEntries` but never nulls it — only
`dictionaryEntries` and `luceneAnalyzer` are nulled. So inserting any
`CONJUNCTION_INDEXBASED` test above it broke it, and three of the new
tests are `CONJUNCTION`.
`close properly` now builds its own `SCANBASED` executor. One deviation
from the review suggestion: it does not assert "null **or** empty",
because a disjunction over the two outcomes pins neither. Each arm of
`close()`'s guard is instead pinned exactly, by a test owning its own
executor:
| Arm | `tokenizedDictionaryEntries` after `close()` | Pinned by |
|---|---|---|
| `SCANBASED` — buffer never allocated | `null`, this path's own
contract | `close properly` |
| `CONJUNCTION_INDEXBASED` — buffer allocated | cleared, **not** nulled
| "empty the tokenized dictionary on close" (added here) |
`close properly` also closes twice, keeping the idempotence the old
ordering exercised by accident. The suite is now order-independent and
the NOTE is gone.
Verified in both directions rather than asserted: inserting a
`CONJUNCTION_INDEXBASED` test directly above `close properly` — the edit
the NOTE forbade — fails at `abd61dc` with `ListBuffer() did not equal
null`, and passes at `5974ffa`.
Coverage is unchanged by that commit: `DictionaryMatcherOpExec` stays at
49/50 lines and 1 missed arm of 26, measured with the same scoped
`WorkflowOperator/jacoco` on both commits, so the table above still
holds. Test counts are unchanged too — it modifies a test rather than
adding one.
### Deliberately not included
`DictionaryMatcherOpExec` line 62 keeps one structurally unreachable
arm, so 49/50 is its ceiling.
`FilterPredicate` lines 66, 80, 130 and 144 stay open and all four are
dead: 66 and 80 would need reflection into the private static
`evaluateFilter` or a production seam, and 130/144 are null ternaries
the caller already guards. Widening any of them would be a production
edit — refused.
No production file is touched. Both files are md5-identical to the
pre-mutation snapshots, and no stray `test_large_binary.txt` was left
behind.
### Any related issues, documentation, discussions?
Closes #7882
### How was this PR tested?
```
sbt "WorkflowOperator/testOnly
org.apache.texera.amber.operator.dictionary.DictionaryMatcherOpExecSpec
org.apache.texera.amber.operator.filter.FilterPredicateSpec"
```
```
[info] Total number of tests run: 44
[info] Tests: succeeded 44, failed 0, canceled 0, ignored 0, pending 0
```
Test counts read from the JUnit XML `tests=` attribute rather than
counted by eye: `DictionaryMatcherOpExecSpec` 11 → 20,
`FilterPredicateSpec` 22 → 24. `Test/scalafmtCheck` passes.
### Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 5)
---
.../dictionary/DictionaryMatcherOpExecSpec.scala | 143 +++++++++++++++++++++
.../operator/filter/FilterPredicateSpec.scala | 44 +++++++
2 files changed, 187 insertions(+)
diff --git
a/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/dictionary/DictionaryMatcherOpExecSpec.scala
b/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/dictionary/DictionaryMatcherOpExecSpec.scala
index 20a10c9ef5..8f71c54a36 100644
---
a/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/dictionary/DictionaryMatcherOpExecSpec.scala
+++
b/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/dictionary/DictionaryMatcherOpExecSpec.scala
@@ -195,9 +195,152 @@ class DictionaryMatcherOpExecSpec extends AnyFlatSpec
with BeforeAndAfter {
}
it should "close properly" in {
+ opDesc.matchingType = MatchingType.SCANBASED
+ opExec = new
DictionaryMatcherOpExec(objectMapper.writeValueAsString(opDesc))
+ opExec.open()
opExec.close()
assert(opExec.dictionaryEntries == null)
+ // SCANBASED never allocates the tokenized buffer, so null here is this
path's own
+ // contract rather than a leftover from whichever test ran last. close()'s
other
+ // arm -- the buffer exists and is cleared, not nulled -- is pinned by
"empty the
+ // tokenized dictionary on close" below.
assert(opExec.tokenizedDictionaryEntries == null)
assert(opExec.luceneAnalyzer == null)
+ // Idempotent: a second close() re-enters with all three fields already at
their
+ // post-close values and must not throw.
+ opExec.close()
+ assert(opExec.dictionaryEntries == null)
+ }
+
+ private def tupleWith(field1: String): Tuple =
+ Tuple
+ .builder(tupleSchema)
+ .add(new Attribute("field1", AttributeType.STRING), field1)
+ .add(new Attribute("field2", AttributeType.INTEGER), 1)
+ .add(new Attribute("field3", AttributeType.BOOLEAN), true)
+ .build()
+
+ private def isMatched(inputTuple: Tuple): Boolean =
+ opExec
+ .processTuple(inputTuple, 0)
+ .next()
+ .asInstanceOf[SchemaEnforceable]
+ .enforceSchema(outputSchema)
+ .getField[Boolean]("matched")
+
+ it should "not label an empty field as matched even though every entry
contains the empty string" in {
+ opDesc.dictionary = dictionaryScan
+ opDesc.matchingType = MatchingType.SUBSTRING
+ opExec = new
DictionaryMatcherOpExec(objectMapper.writeValueAsString(opDesc))
+ opExec.open()
+ // Without the empty-text guard, SUBSTRING would ask whether "nice a a
person"
+ // contains "" -- which every string does -- and report a match.
+ assert(!isMatched(tupleWith("")))
+ opExec.close()
+ }
+
+ it should "not match under CONJUNCTION_INDEXBASED when the field tokenizes
to nothing" in {
+ opDesc.dictionary = "the"
+ opDesc.matchingType = MatchingType.CONJUNCTION_INDEXBASED
+ opExec = new
DictionaryMatcherOpExec(objectMapper.writeValueAsString(opDesc))
+ opExec.open()
+ // "the" is an English stop word, so both the entry and the field tokenize
to
+ // the empty set, and the empty set is trivially a subset of itself. Only
the
+ // explicit non-emptiness check keeps this from being reported as a match.
+ assert(!isMatched(tupleWith("the")))
+ opExec.close()
+ }
+
+ it should "ignore dictionary tokens that stem into an English stop word" in {
+ opDesc.dictionary = "nice willing person"
+ opDesc.matchingType = MatchingType.CONJUNCTION_INDEXBASED
+ opExec = new
DictionaryMatcherOpExec(objectMapper.writeValueAsString(opDesc))
+ opExec.open()
+ // The analyzer stems "willing" to "will", which is an English stop word,
so the
+ // entry reduces to {nice, person} and is still a subset of the field's
+ // {person, nice}. The field deliberately reverses the two words so that
plain
+ // substring containment ("nice willing person" contains "person nice") is
+ // false: only the tokenized conjunction path can satisfy this assertion.
+ assert(isMatched(tupleWith("person nice")))
+ opExec.close()
+ }
+
+ it should "ignore dictionary tokens that are URL stop words" in {
+ // Each of these survives the English analyzer unchanged and is not an
English
+ // stop word, so each is dropped only by the operator's own URL list.
"https"
+ // is deliberately absent: the Porter stemmer rewrites it to "http", so
that
+ // entry can never fire (recorded as dead data, not pinned). As above, the
+ // field reverses the word order so substring containment cannot satisfy
it.
+ for (urlWord <- List("http", "org", "net", "com", "store", "www", "html"))
{
+ opDesc.dictionary = s"nice $urlWord person"
+ opDesc.matchingType = MatchingType.CONJUNCTION_INDEXBASED
+ opExec = new
DictionaryMatcherOpExec(objectMapper.writeValueAsString(opDesc))
+ opExec.open()
+ withClue(s"URL stop word $urlWord: ")(assert(isMatched(tupleWith("person
nice"))))
+ opExec.close()
+ }
+ }
+
+ it should "split the dictionary on commas" in {
+ opDesc.dictionary = "cat,dog"
+ opDesc.matchingType = MatchingType.SCANBASED
+ opExec = new
DictionaryMatcherOpExec(objectMapper.writeValueAsString(opDesc))
+ opExec.open()
+ // Two entries, not one: "dog" is an entry on its own and the raw text is
not.
+ assert(isMatched(tupleWith("dog")))
+ assert(!isMatched(tupleWith("cat,dog")))
+ opExec.close()
+ }
+
+ it should "lower-case the tuple field before comparing it to the dictionary"
in {
+ opDesc.dictionary = dictionaryScan
+ opDesc.matchingType = MatchingType.SCANBASED
+ opExec = new
DictionaryMatcherOpExec(objectMapper.writeValueAsString(opDesc))
+ opExec.open()
+ // Dictionary entries are lower-cased when they are split, so the field
has to
+ // be folded too or an upper-case field could never match a lower-case
entry.
+ assert(isMatched(tupleWith(dictionaryScan.toUpperCase)))
+ opExec.close()
+ }
+
+ it should "report a null field as unmatched instead of failing" in {
+ opDesc.dictionary = dictionaryScan
+ opDesc.matchingType = MatchingType.SCANBASED
+ opExec = new
DictionaryMatcherOpExec(objectMapper.writeValueAsString(opDesc))
+ opExec.open()
+ // The dictionary lookup casts the field to String and lower-cases it, so
a null
+ // field has to be filtered out before the lookup is reached.
+ assert(!isMatched(tupleWith(null)))
+ opExec.close()
+ }
+
+ it should "empty the tokenized dictionary on close" in {
+ opDesc.dictionary = "nice person"
+ opDesc.matchingType = MatchingType.CONJUNCTION_INDEXBASED
+ opExec = new
DictionaryMatcherOpExec(objectMapper.writeValueAsString(opDesc))
+ opExec.open()
+ assert(opExec.tokenizedDictionaryEntries.nonEmpty)
+ opExec.close()
+ // Asserts emptiness only, never null: close() nulls the other two fields
but
+ // merely clears this one, and this test takes no side on that asymmetry.
+ assert(opExec.tokenizedDictionaryEntries.isEmpty)
+ }
+
+ it should "fail loudly rather than silently report no match when no matching
type is configured" in {
+ opDesc.dictionary = dictionaryScan
+ opDesc.matchingType = null
+ opExec = new
DictionaryMatcherOpExec(objectMapper.writeValueAsString(opDesc))
+ opExec.open()
+ // A descriptor whose "Matching type" is absent from the JSON deserializes
to a
+ // null matchingType -- Jackson does not enforce `required` on read -- so
this is
+ // a reachable runtime state. What is pinned is only that the operator
refuses
+ // it instead of labelling every tuple "not matched". The exception's
identity
+ // is scalac's business (today a MatchError from the non-exhaustive match)
and
+ // is deliberately not named, so replacing it with an explicit, clearer
throw
+ // would not break this test.
+ intercept[RuntimeException] {
+ opExec.processTuple(tuple, 0).next()
+ }
+ opExec.close()
}
}
diff --git
a/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/filter/FilterPredicateSpec.scala
b/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/filter/FilterPredicateSpec.scala
index e683c7b4b5..0bb9a3a475 100644
---
a/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/filter/FilterPredicateSpec.scala
+++
b/common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/filter/FilterPredicateSpec.scala
@@ -210,4 +210,48 @@ class FilterPredicateSpec extends AnyFlatSpec with
Matchers {
singleFieldTuple(AttributeType.TIMESTAMP,
Timestamp.valueOf("2020-01-01 00:00:00"))
) shouldBe true
}
+
+ "FilterPredicate.evaluate" should "pin every ordering operator on both sides
of its boundary" in {
+ // Every other assertion in this spec places its operands on only one side
of
+ // the operator's boundary: the two `_OR_EQUAL_TO` arms are only ever asked
+ // about EQUAL operands (where `== 0` answers the same) and the two strict
arms
+ // only about strictly-ordered ones (where the non-strict twin answers the
+ // same). Each operator below is therefore asked all three questions --
field
+ // less than, equal to, and greater than the value -- so that no arm of the
+ // comparison switch can be swapped for another and stay green.
+ def evaluates(condition: ComparisonType, age: Int): Boolean =
+ new FilterPredicate("age", condition, "18").evaluate(ageTuple(age))
+
+ evaluates(ComparisonType.EQUAL_TO, 10) shouldBe false
+ evaluates(ComparisonType.EQUAL_TO, 18) shouldBe true
+ evaluates(ComparisonType.EQUAL_TO, 30) shouldBe false
+
+ evaluates(ComparisonType.NOT_EQUAL_TO, 10) shouldBe true
+ evaluates(ComparisonType.NOT_EQUAL_TO, 18) shouldBe false
+ evaluates(ComparisonType.NOT_EQUAL_TO, 30) shouldBe true
+
+ evaluates(ComparisonType.GREATER_THAN, 10) shouldBe false
+ evaluates(ComparisonType.GREATER_THAN, 18) shouldBe false
+ evaluates(ComparisonType.GREATER_THAN, 30) shouldBe true
+
+ evaluates(ComparisonType.GREATER_THAN_OR_EQUAL_TO, 10) shouldBe false
+ evaluates(ComparisonType.GREATER_THAN_OR_EQUAL_TO, 18) shouldBe true
+ evaluates(ComparisonType.GREATER_THAN_OR_EQUAL_TO, 30) shouldBe true
+
+ evaluates(ComparisonType.LESS_THAN, 10) shouldBe true
+ evaluates(ComparisonType.LESS_THAN, 18) shouldBe false
+ evaluates(ComparisonType.LESS_THAN, 30) shouldBe false
+
+ evaluates(ComparisonType.LESS_THAN_OR_EQUAL_TO, 10) shouldBe true
+ evaluates(ComparisonType.LESS_THAN_OR_EQUAL_TO, 18) shouldBe true
+ evaluates(ComparisonType.LESS_THAN_OR_EQUAL_TO, 30) shouldBe false
+ }
+
+ "FilterPredicate.equals" should "distinguish predicates that differ only in
attribute, or only in condition" in {
+ val base = new FilterPredicate("age", ComparisonType.EQUAL_TO, "1")
+ val otherAttribute = new FilterPredicate("name", ComparisonType.EQUAL_TO,
"1")
+ val otherCondition = new FilterPredicate("age",
ComparisonType.GREATER_THAN, "1")
+ base should not be otherAttribute
+ base should not be otherCondition
+ }
}
