This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-7883-6b1a1e8691de7588b93013dbe1df6f46d408a68d in repository https://gitbox.apache.org/repos/asf/texera.git
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 + } }
