MINOR: simplify FK-join logic #20605
Conversation
The existing FK join logic is very convoluted due to incremtal changes and bug-fixes, and thus very hard to understand. This PR rewrite the logic from scratch to make it easier to understanding, and fixes a minor bug on the side.
| value -> { | ||
| final String[] tokens = value.split("\\|"); | ||
| return tokens.length == 2 ? tokens[1] : null; | ||
| } |
There was a problem hiding this comment.
Some side cleanup... While playing with the code adding other test (which I removed later, as they were just for myself), this method failed if we did only get one token...
| @Override | ||
| public void process(final Record<KRight, SubscriptionWrapper<KLeft>> record) { | ||
| if (record.key() == null && !SubscriptionWrapper.Instruction.PROPAGATE_NULL_IF_NO_FK_VAL_AVAILABLE.equals(record.value().instruction())) { | ||
| final KRight foreignKey = record.key(); |
There was a problem hiding this comment.
Minor cleanup, to explain that record.key() is the FK -- it's not necessarily obvious form the existing code.
There was a problem hiding this comment.
as a thought, what if we rename the record to something like fkRecord instead?
There was a problem hiding this comment.
Happy to rename, but don't think it does buy us too much, as it clear from the class name and parameter type that it's a subscription record. But renaming does not help much to explain how a subscription record built up internally.
| } | ||
| } | ||
|
|
||
| private void leftJoinInstructions(final Record<KLeft, Change<VLeft>> record) { |
There was a problem hiding this comment.
The diff is unfortunately a little bit messy, but if you compare new and old code, also because I added comments, it should become clear the the new code is much cleaner, especially for left-join case.
| @Override | ||
| public void process(final Record<KRight, SubscriptionWrapper<KLeft>> record) { | ||
| if (record.key() == null && !SubscriptionWrapper.Instruction.PROPAGATE_NULL_IF_NO_FK_VAL_AVAILABLE.equals(record.value().instruction())) { | ||
| final KRight foreignKey = record.key(); |
There was a problem hiding this comment.
as a thought, what if we rename the record to something like fkRecord instead?
| // however, we cannot avoid it as we have no means to know if the old FK joined or not | ||
| forward(record, oldForeignKey, DELETE_KEY_AND_PROPAGATE); | ||
| } | ||
| } else { // valid FK |
There was a problem hiding this comment.
nit: could be else if
There was a problem hiding this comment.
Not sure if I can follow?
if (newForeignKey = null) {
...
} else { // what would we add here? -> `else if (newForeignKey != null)` would be redundant
...
}
| // | ||
| // if FK did change, we need to explicitly delete the old subscription, | ||
| // because the new subscription goes to a different partition | ||
| final boolean foreignKeyChanged = !Arrays.equals(serialize(newForeignKey), serialize(oldForeignKey)); |
There was a problem hiding this comment.
we can check for null here to avoid the unnecessary serialization and comparison
There was a problem hiding this comment.
Don't think it would buy us much? Just a few method calls, as serialize() would do the null checks and returns null, and if either serialize call does return null, equals is also very cheap.
I would rather keep it the way it is to keep fewer if/else checks what makes it easier to read, and I don't think we get any perf benefits if we make the code more complicated.
| // for all cases (insert, update, and delete), we send a new subscription; | ||
| // we need to get a response back for all cases to always produce a left-join result | ||
| // | ||
| // note: for delete, `newForeignKey` is null, what is a "hack" |
There was a problem hiding this comment.
a thought, if we split the logic into if equals null, delete(or pass null expricitly), if not do the normal stuff, will it look as hacky?
There was a problem hiding this comment.
IMHO it's still hacky, as we keep sending a null to the right hand side. The hacky part is not about the variable being null (it contributes a little bit I guess, as it might seem unintuitive that it is a valid case), but about sending null at all (and sending null is also unintuitive by itself IMHO). Thoughts?
The existing FK join logic is very convoluted due to incremental changes
and bug-fixes, and thus very hard to understand.
This PR rewrites the logic from scratch to make it easier to
understanding.