Fix issue with shift propagation from Table.reverse() on blink tables #3888
Conversation
rcaudy
added
bug
|
Tested with unit test and the provided reproducer. |
There was a problem hiding this comment.
I've verified this fixes the issue. I can't say I understand the nuances involved in ReverseOperation#onUpdate. I'm assuming the else branch is still technically "correct" for empty resultRowSets in the non-blink case; but this will be an improvement for non-blink cases regardless?
Correct. There's no reason to communicate any shifts at all for rows that don't exist (in post-shift space) in the result table. In this case, we can assess that by looking at the result row set once all upstream removes have been removed. Not communicating unnecessary shifts simplifies the downstream update, and potentially saves a lot of work for downstream listeners. |
Should we explicitly disallow unnecessary shifts? Or, have helpers that simplify them away for all cases? |
| if (nextShiftEnd < nextShiftStart) { | ||
| continue; | ||
| // Only compute downstream shifts if there are retained rows to shift | ||
| if (resultRowSet.isEmpty()) { |
There was a problem hiding this comment.
You could also skip this if the prev rowset is empty (since shifts affect rows that exist in both key spaces)
Fixes #3858