Copilot commented on code in PR #4873:
URL: https://github.com/apache/solr/pull/4873#discussion_r3933410303
##########
solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/stream/ComplementStream.java:
##########
@@ -201,9 +210,12 @@ public Tuple read() throws IOException {
}
// if a != b && a < b then we know there is no b which a might equal so
return a
- if (!eq.test(a, b) && streamA.getStreamSort().compare(a, b) < 0) {
- streamB.pushBack(b);
- return a;
+ if (!eq.test(a, b)) {
+ eq.assertFieldsPresent(a, b);
Review Comment:
Because this assertion is inside the `!eq.test` branch, it misses absent
fields whenever both lookups produce null; `FieldEqualitor.test` then reports
equality and `complement` silently drops the tuple. Move the presence check
ahead of the equality test so the new fail-loud behavior also covers that case.
##########
solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/eq/StreamEqualitor.java:
##########
@@ -26,4 +29,75 @@ public interface StreamEqualitor extends Equalitor<Tuple>,
Expressible, Serializ
public boolean isDerivedFrom(StreamEqualitor base);
public boolean isDerivedFrom(StreamComparator base);
+
+ /**
+ * Whether this equalitor's left-hand field(s) are exactly the field(s) that
{@code base} - a
+ * single stream's own sort comparator, whose left/right field names are
necessarily identical -
+ * sorts on. Used to validate the stream feeding the left side of a
two-stream equality (e.g.
+ * streamA in complement/intersect), as opposed to {@link
#isDerivedFrom(StreamComparator)} which
+ * matches either side and so cannot validate an asymmetric {@code on=}
clause correctly.
+ */
+ boolean isDerivedFromLeft(StreamComparator base);
+
+ /** Right-hand counterpart of {@link #isDerivedFromLeft(StreamComparator)}.
*/
+ boolean isDerivedFromRight(StreamComparator base);
Review Comment:
Adding these as abstract interface methods breaks source and binary
compatibility for third-party `StreamEqualitor` implementations; an existing
compiled implementation can fail with `AbstractMethodError` when these methods
are invoked. Provide default fallbacks to the existing `isDerivedFrom`
contract, while the built-in asymmetric equalitors continue to override them.
This issue also appears on line 100 of the same file.
##########
solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/stream/IntersectStream.java:
##########
@@ -203,13 +212,14 @@ public Tuple read() throws IOException {
}
// We're not at the end, and they're not equal. We now need to decide
which we can
- // throw away. This is accomplished by checking which is less than the
other. The
- // one that is less (determined by the sort) can be tossed. The other
should
- // be pushed back and the loop continued. We don't have to worry about
an == 0
- // result because we already know tuples a and b are not equal. And
because eq
- // is derived from the sorts of both streamA and streamB we can rest
assured that
- // equality is not a possibility.
- int aComp = streamA.getStreamSort().compare(a, b);
+ // throw away. This is accomplished by checking which is less than the
other, using
+ // crossStreamComparator - a comparator built from eq's (possibly
different) left/right
+ // field names, since streamA's own sort comparator only knows streamA's
field and would
+ // read null off of b. The one that is less can be tossed. The other
should be pushed back
+ // and the loop continued. We don't have to worry about an == 0 result
because we already
+ // know tuples a and b are not equal.
+ eq.assertFieldsPresent(a, b);
Review Comment:
This validation runs only after `eq.test(a, b)` is false.
`FieldEqualitor.test` treats two null lookups as equal, so a missing `on` field
paired with another missing or present-null field returns `a` before this check
and still silently accepts the wiring error. Validate field presence before
testing equality.
--
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]
