Skip to content

Ft/conditional prev val#14

Merged
dev-davexoyinbo merged 3 commits into
mainfrom
ft/conditional-prev-val
Apr 13, 2026
Merged

Ft/conditional prev val#14
dev-davexoyinbo merged 3 commits into
mainfrom
ft/conditional-prev-val

Conversation

@dev-davexoyinbo
Copy link
Copy Markdown
Owner

No description provided.

@dev-davexoyinbo dev-davexoyinbo merged commit d2f87b9 into main Apr 13, 2026
1 check passed
@qodo-code-review
Copy link
Copy Markdown

Review Summary by Qodo

Return previous values from conditional counter operations
✨ Enhancement

Grey Divider

Walkthroughs

Description
• Return previous values from conditional counter operations
• Update inc_if and set_if to return (new_value, old_value) tuples
• Update batch operations to return (key, new, old) tuples
• Modify instance-aware counter operations similarly for state tracking
Diagram
flowchart LR
  A["Conditional Counter Methods"] -->|inc_if/set_if| B["Return new_value"]
  A -->|inc_if/set_if| C["Return old_value"]
  B --> D["(new_value, old_value)"]
  C --> D
  E["Batch Operations"] -->|inc_all_if/set_all_if| F["Return key"]
  E -->|inc_all_if/set_all_if| G["Return new_value"]
  E -->|inc_all_if/set_all_if| H["Return old_value"]
  F --> I["(key, new, old)"]
  G --> I
  H --> I
Loading

Grey Divider

File Changes

1. src/counter/counter_trait.rs ✨ Enhancement +27/-41

Update trait signatures to return previous values

src/counter/counter_trait.rs


2. src/counter/lax_counter.rs ✨ Enhancement +26/-16

Implement previous value returns in lax counter

src/counter/lax_counter.rs


3. src/counter/strict_counter.rs ✨ Enhancement +38/-22

Implement previous value returns in strict counter

src/counter/strict_counter.rs


View more (9)
4. src/counter/tests/lax_counter.rs 🧪 Tests +49/-7

Update lax counter tests for new return types

src/counter/tests/lax_counter.rs


5. src/counter/tests/strict_counter.rs 🧪 Tests +9/-6

Update strict counter tests for new return types

src/counter/tests/strict_counter.rs


6. src/icounter/mod.rs ✨ Enhancement +57/-35

Update instance-aware counter trait signatures

src/icounter/mod.rs


7. src/icounter/lax_instance_aware_counter.rs ✨ Enhancement +54/-24

Implement previous state returns in lax instance counter

src/icounter/lax_instance_aware_counter.rs


8. src/icounter/strict_instance_aware_counter.rs ✨ Enhancement +186/-77

Implement previous state returns in strict instance counter

src/icounter/strict_instance_aware_counter.rs


9. src/icounter/tests/lax_instance_aware_counter.rs 🧪 Tests +59/-10

Update lax instance counter tests for new return types

src/icounter/tests/lax_instance_aware_counter.rs


10. src/icounter/tests/strict_instance_aware_counter.rs 🧪 Tests +27/-11

Update strict instance counter tests for new return types

src/icounter/tests/strict_instance_aware_counter.rs


11. README.md 📝 Documentation +11/-5

Update documentation with new return value examples

README.md


12. docs/lib.md 📝 Documentation +3/-3

Update library documentation with new return values

docs/lib.md


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Copy Markdown

qodo-code-review Bot commented Apr 13, 2026

Code Review by Qodo

🐞 Bugs (1)   📘 Rule violations (1)   📎 Requirement gaps (0)   🖥 UI issues (0)   🎨 UX Issues (0)
🐞\ ≡ Correctness (1)
📘\ ≡ Correctness (1)

Grey Divider


Action required

1. INC_LUA returns no key 📘
Description
The INC_LUA script returns only numeric values and does not echo the relevant key string,
preventing safe key-addressed result mapping. This forces callers toward brittle positional
alignment for batch operations.
Code

src/counter/strict_counter.rs[R36-41]

    local current = tonumber(redis.call('HGET', container_key, key)) or 0
    if not compare_values(current, comparator, compare_against) then
-        return current
+        return {current, current}
    end

-    return redis.call('HINCRBY', container_key, key, count)
+    return {tonumber(redis.call('HINCRBY', container_key, key, count)), current}
Evidence
PR Compliance ID 3 requires Redis scripts to include the key string in their return values; the
modified INC_LUA returns {current, current} / {new_total, current} without key.

CLAUDE.md
src/counter/strict_counter.rs[36-41]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`INC_LUA` does not echo the `key` in its return value, violating the key-echo requirement and preventing safe key-based association of results.

## Issue Context
Batch operations should be able to build results keyed by the returned key strings rather than relying on pipeline ordering.

## Fix Focus Areas
- src/counter/strict_counter.rs[35-42]
- src/counter/strict_counter.rs[292-313]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Batch set_on_instance drops duplicates 🐞
Description
StrictInstanceAwareCounter::set_on_instance_if_batch also uses a HashMap keyed by DistkitRedisKey to
store results, so duplicate keys overwrite earlier entries and the returned per-item new/old states
are wrong for earlier duplicates.
Code

src/icounter/strict_instance_aware_counter.rs[R1107-1112]

        let mut conn = self.connection_manager.clone();
-        let mut map: HashMap<DistkitRedisKey, (i64, i64)> = HashMap::with_capacity(updates.len());
+        let mut map: HashMap<DistkitRedisKey, (i64, i64, i64, i64)> =
+            HashMap::with_capacity(updates.len());
        let mut processed = 0;

        while processed < updates.len() {
Evidence
As with set_if_batch, this function inserts each pipeline result into a HashMap and later
reconstructs outputs by looking up by key, which necessarily loses distinct results for duplicate
keys and breaks per-item ordering semantics.

src/icounter/strict_instance_aware_counter.rs[1097-1178]
src/icounter/mod.rs[526-563]
src/common/mod.rs[20-54]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`StrictInstanceAwareCounter::set_on_instance_if_batch` collapses duplicate keys by storing results in a `HashMap` and reconstructing outputs with `map.get(k)`.

### Issue Context
`execute_pipeline_with_script_retry` returns results aligned with the invocations pushed into the pipeline, which are created by iterating the chunk slice.

### Fix
Build the returned `Vec` by zipping each input update with its corresponding pipeline result (per chunk), preserving duplicates and per-item old/new state.

### Fix Focus Areas
- src/icounter/strict_instance_aware_counter.rs[1097-1178]
- src/common/mod.rs[20-54]
- src/icounter/mod.rs[526-563]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

@dev-davexoyinbo dev-davexoyinbo deleted the ft/conditional-prev-val branch April 13, 2026 10:13
Comment on lines 36 to +41
local current = tonumber(redis.call('HGET', container_key, key)) or 0
if not compare_values(current, comparator, compare_against) then
return current
return {current, current}
end

return redis.call('HINCRBY', container_key, key, count)
return {tonumber(redis.call('HINCRBY', container_key, key, count)), current}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

1. inc_lua returns no key 📘 Rule violation ≡ Correctness

The INC_LUA script returns only numeric values and does not echo the relevant key string,
preventing safe key-addressed result mapping. This forces callers toward brittle positional
alignment for batch operations.
Agent Prompt
## Issue description
`INC_LUA` does not echo the `key` in its return value, violating the key-echo requirement and preventing safe key-based association of results.

## Issue Context
Batch operations should be able to build results keyed by the returned key strings rather than relying on pipeline ordering.

## Fix Focus Areas
- src/counter/strict_counter.rs[35-42]
- src/counter/strict_counter.rs[292-313]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Comment on lines 1107 to 1112
let mut conn = self.connection_manager.clone();
let mut map: HashMap<DistkitRedisKey, (i64, i64)> = HashMap::with_capacity(updates.len());
let mut map: HashMap<DistkitRedisKey, (i64, i64, i64, i64)> =
HashMap::with_capacity(updates.len());
let mut processed = 0;

while processed < updates.len() {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

2. Batch set_on_instance drops duplicates 🐞 Bug ≡ Correctness

StrictInstanceAwareCounter::set_on_instance_if_batch also uses a HashMap keyed by DistkitRedisKey to
store results, so duplicate keys overwrite earlier entries and the returned per-item new/old states
are wrong for earlier duplicates.
Agent Prompt
### Issue description
`StrictInstanceAwareCounter::set_on_instance_if_batch` collapses duplicate keys by storing results in a `HashMap` and reconstructing outputs with `map.get(k)`.

### Issue Context
`execute_pipeline_with_script_retry` returns results aligned with the invocations pushed into the pipeline, which are created by iterating the chunk slice.

### Fix
Build the returned `Vec` by zipping each input update with its corresponding pipeline result (per chunk), preserving duplicates and per-item old/new state.

### Fix Focus Areas
- src/icounter/strict_instance_aware_counter.rs[1097-1178]
- src/common/mod.rs[20-54]
- src/icounter/mod.rs[526-563]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant