belliottsmith commented on code in PR #167:
URL: https://github.com/apache/cassandra-accord/pull/167#discussion_r1944802822
##########
accord-core/src/main/java/accord/impl/CommandChange.java:
##########
@@ -541,10 +541,16 @@ private static <OBJ, VAL> int collectFlags(OBJ lo, OBJ
ro, Function<OBJ, VAL> co
private static <OBJ, VAL> int collectFlags(OBJ lo, OBJ ro, Function<OBJ,
VAL> convert, BiPredicate<VAL, VAL> equals, boolean allowClassMismatch, Field
field, int flags)
{
- VAL l = null;
VAL r = null;
- if (lo != null) l = convert.apply(lo);
if (ro != null) r = convert.apply(ro);
+ if (lo == null)
+ {
+ if (r == null)
+ return setFieldIsNullAndChanged(field, flags);
+ else
+ return setChanged(field, flags);
+ }
+ VAL l = convert.apply(lo);
if (l == r) return flags; // no change
Review Comment:
This could be handled here in these return flags, but could you perhaps
explain more (also in a comment) why this is necessary? I think it might
introduce bugs with compaction, as we seem to be _setting_ to null any field
that is null in both input and output, but for compaction of partial results we
can expect fields to just not participate in the compaction (i.e. be null in
both sides) but be *not null* in any constructed object, and this could cause
them to be shadowed and nulled out, no?
Is this to handle the `TruncatedApply` case, and erasing `Result`? If so, I
think it is better to special-case truncation, and to produce a null field mask
directly.
--
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]
