Negation querying with schema level tracked/untracked keys

[VoletiRam]

Negation querying with schema level tracked/untracked keys. Negation logic: total keys - matching keys = negation result

[allenss-amazon]

Overall, it's definitely advantageous to think of the problem as a state machine: Phase1, Phase2 and Done -- and it should be documented as such. When I read the code, I believe it's true that the phase variable is a redundantly encoded version of the tracked and untracked iterators. In other words:

1. Phase1 <=> tracked_iterator != end.
2. Phase2 <=> tracked_iterator == end && untracked_iterator != end.
3. Done <=> tracked_iterator == end && untracked_iterator == end.

Generally, having redundant state is undesirable because:

1. It increases the cognitive load on the future code reader/maintainer as there are more pieces of state floating around.
2. It increases the complexity of the code as there's extra code to maintain the duplicate state.
3. It's error prone because the duplicate state can get out of sync without careful coding.

One hybrid solution would be to create a few simple boolean functions like IsPhase1() and IsPhase2(), etc.

[allenss-amazon]

This would make more sense, if you setup the positive iterator unconditionally (since it's used in all paths) and then conditionally wrapped it in the negative iterator.

Also, I notice that one path assigns fetch->predicate_, but the other path doesn't. This asymmetry sticks out as a potential issue.

[allenss-amazon]

Isn't the point of this code to align the iterator to the first valid key? Wouldn't it be true that if you setup the correct initialization of iterators/phases you should be able to just call NextKey here, right?

[VoletiRam]

Sentinel logic doesn't help here because with empty sets iterator cannot distinguish between begin(uninitialized) and end(exhausted) but we need to position at first valid key at initialization. Will add initialization variable and we can move this logic to nextKey.

[allenss-amazon]

Why isn't the loop a call to NextKey() ?

[VoletiRam]

Aah yes.

[VoletiRam]

@allenss-amazon Agreed about redundant version of track/untracked state machine. Will follow the suggestion and name the functions as IsTrackedPhase and IsUnTrackedPhase etc instead as it is more self descriptive.

[KarthikSubbarao]

@VoletiRam - Can we update the PR? From the history rewrite change (for fixing the history), the PR has other changes included as well

[KarthikSubbarao]

When I read this code earlier, I saw it was missing handling from the PredicateEvaluator (predicate.cc) from the main thread verification. Can we include this as well? There can be mutations since the time of the background search and the main thread verification + reply stage

[daddaman-amz]

@VoletiRam
Currently ft.info 's num_records in using this function and since we return zero, text is not being accounted for in that

Will your PR be implementing tracked key count or is this something not relevant for text since we have shared structure?