michaeljmarshall commented on code in PR #4353:
URL: https://github.com/apache/cassandra/pull/4353#discussion_r2688032919
##########
src/java/org/apache/cassandra/index/sai/memory/MemtableIndexManager.java:
##########
@@ -92,6 +102,7 @@ public long update(DecoratedKey key, Row oldRow, Row newRow,
Memtable memtable)
return index(key, newRow, memtable);
}
+ // TODO when do we hit this case? It isn't covered by tests.
Review Comment:
I would expect that we're guaranteed to not hit this case by the design of
the update execution path but I don't know enough about the calling code to say
for sure. However, with this change, we will have incorrect validation
semantics if that assumption isn't true, so I think it might be better to add
an assertion here that the value isn't `null`.
I agree that the branching isn't ideal, but note that we don't actually
update anything in the non-vector case, we just insert but in the vector case,
we're looking to replace the vector in the index.
Maybe it'd be helpful to get confirmation that the system is designed the
way I think it is.
--
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]
