CASSANDRA-20297: Fix null field accounting #167
Conversation
ifesdjeen
changed the title
| else | ||
| return setChanged(field, flags); | ||
| } | ||
| VAL l = convert.apply(lo); | ||
|
|
||
| if (l == r) return flags; // no change |
There was a problem hiding this 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.
There was a problem hiding this comment.
This works for me, although I might slightly prefer that we precomputed some flags and saved them in SaveStatus and just did |= saveStatus.commandChangeFlags. This would permit us to clear other registers too (e.g. DepsErased, ExecuteAtErased etc), and do it in a simple declarative way.
|
@belliottsmith great idea! Pushed the implementation and test for this. |
There was a problem hiding this comment.
Only one suggestion: there's an edge case with Vestigial for which - for distributed inference - we don't declare that any of the values are Erased, even though we have erased them. Perhaps we could reify this in the SaveStatus somehow, but for now at least it would be most robust to special-case this SaveStatus and set all of the null/changed flags for Vestigial.
ifesdjeen
force-pushed
the
CASSANDRA-20297
branch
2 times, most recently
from
758a2bc to
1f1e5b1
Compare
Patch by Alex Petrov; reviewed by Benedict Elliott Smith for CASSANDRA-20297
ifesdjeen
force-pushed
the
CASSANDRA-20297
branch
from
1f1e5b1 to
a2ac02b
Compare
No description provided.