feat: add list ops with array length accessor and any/all/none quantifiers - #6
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds array Changes
Sequence Diagram(s)sequenceDiagram
participant Parser as Parser
participant Validator as Validator
participant Evaluator as Evaluator
participant Context as DataContext
rect rgba(200,200,255,0.5)
Parser->>Parser: Tokenize input, detect ANY/ALL/NONE
Parser->>Parser: parseQuantifierExpression(left) -> BinaryOp(quantifier, left, subExpr)
end
rect rgba(200,255,200,0.5)
Validator->>Validator: Visit BinaryOp(quantifier,...)
Validator->>Validator: validateQuantifierOperation(left) -> ensure identifier/property
end
rect rgba(255,200,200,0.5)
Evaluator->>Context: resolve left operand -> value (expect array)
Evaluator->>Evaluator: if final property == "length" -> return numeric length
alt Quantifier = ANY
Evaluator->>Evaluator: evaluateQuantifierAny() - true on first matching element
else Quantifier = ALL
Evaluator->>Evaluator: evaluateQuantifierAll() - false on first non-matching element
else Quantifier = NONE
Evaluator->>Evaluator: evaluateQuantifierNone() - false on first matching element
end
Evaluator->>Evaluator: return boolean result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
lucasnakano
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@benchmark_optimized_test.go`:
- Around line 155-178: The benchmark comment on
BenchmarkOptimizedEngineQuantifierAny incorrectly claims "for zero allocations"
but the quantifier evaluation incurs 1 allocation (24 B) due to []any boxing;
update the function comment (and analogous quantifier benchmark comments such as
for `all`/`none` if present) to accurately state "for 1 allocation from []any
boxing (24 B)" or similar, so the comment reflects actual allocation behavior;
locate the comment above the `BenchmarkOptimizedEngineQuantifierAny` function
(and matching quantifier benchmarks) and change the wording rather than altering
benchmark logic.
🧹 Nitpick comments (1)
CLAUDE.md (1)
43-57: Add language identifier to fenced code block.Static analysis (markdownlint MD040) flags this code block as missing a language specifier. Since these are rule expressions, you could use a generic identifier or leave as plain text explicitly.
📝 Proposed fix
-``` +```text x eq 10 // equality
8ef3746 to
3d1a63f
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @.github/workflows/test.yml:
- Line 20: The workflow update changes the golangci-lint tool to "version:
v2.9.0" which is a large jump from v2.2.1; run the CI pipeline locally and in CI
to surface any new lint failures, review and fix/whitelist new issues reported
by golangci-lint (adjust config in .golangci.yml or suppress specific checks),
or if you're not ready to address new errors, revert or pin back to "v2.2.1"
temporarily; ensure the chosen approach leaves the pipeline green and update the
workflow line referencing "version: v2.9.0" accordingly.
In `@CLAUDE.md`:
- Around line 43-44: The fenced code block containing the example "x eq 10" is
missing a language tag which triggers markdownlint MD040; update the opening
fence from ``` to include a language tag (e.g., change ``` to ```text) for the
block that contains "x eq 10" so the fenced code block is annotated and the
linter warning is resolved.
- Around line 134-135: The current "Allocation Verification" rule in CLAUDE.md
includes a parenthetical exception "(array operations may have 1 alloc from Go
runtime `[]any` handling)" which weakens the 0 allocs/op requirement; remove
that exception and make the rule strictly "All benchmarks must show 0
allocs/op". Update the "Allocation Verification" header text to omit the
parenthetical, and then locate any corresponding benchmark descriptions/tests
that referenced or relied on the allowed 1 allocation and change the
implementations or assertions so they report 0 allocs/op instead (ensure any
mention of `[]any` handling is removed or replaced with guidance to avoid
allocations). Ensure documentation and tests consistently require and assert 0
allocs/op.
🧹 Nitpick comments (1)
evaluator.go (1)
471-475: Note behavioral difference between quantifiers for non-map elements.
ALLreturnsfalsefor non-map elements (line 474), whileANYandNONEskip them withcontinue. This is intentional and correct: forALL, every element must satisfy the predicate, so a non-evaluable element fails the condition. ForANY/NONE, we're looking for existence/absence of matches among evaluable elements.Worth documenting this semantic distinction if not already covered.
3d1a63f to
a7e3907
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@benchmark_optimized_test.go`:
- Around line 130-295: Add explicit allocation verification to each new
benchmark (BenchmarkOptimizedEngineArrayLength,
BenchmarkOptimizedEngineQuantifierAny, BenchmarkOptimizedEngineQuantifierAll,
BenchmarkOptimizedEngineQuantifierNone, BenchmarkOptimizedEngineComplexBetting,
BenchmarkOptimizedEngineQuantifierAnyShortCircuit): call b.ReportAllocs() and
use testing.AllocsPerRun (with a reasonable iteration count) to assert allocs/op
== 0 for the evaluated rule/context before entering the timed loop (or before
b.ResetTimer()), and fail the benchmark with b.Fatalf if allocs != 0 so the
benchmark enforces the "0 allocs/op" guideline.
heynemann
left a comment
heynemann
left a comment
There was a problem hiding this comment.
Approving to unblock but left comments within coderabbit threads
74aafb4 to
0e3f61f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@benchmark_optimized_test.go`:
- Around line 229-234: The benchmark currently allows allocations by asserting
allocs <= 1; change the allocation check to require zero allocations by
replacing the testing.AllocsPerRun check (the block that calls
testing.AllocsPerRun(1, func() { _, _ = engine.Evaluate(rule, ctx) })) to fail
when allocs != 0 (or allocs > 0) and update the b.Fatalf message accordingly;
apply the same change to the other benchmark blocks that call
testing.AllocsPerRun (the ones exercising engine.Evaluate with rule and ctx) so
all benchmarks enforce 0 allocs/op.
- Around line 103-110: The benchmark currently allows up to 1 allocation by
checking "allocs > 1"; change the assertion to enforce zero allocations by
checking "allocs != 0" or "allocs > 0" and update the failure message to
"expected 0 allocs/op, got %f". Apply this change around the
testing.AllocsPerRun(1, func() { _, _ = engine.Evaluate(rule, ctx) }) block and
keep b.ReportAllocs() and the same call to engine.Evaluate(rule, ctx).
| b.ReportAllocs() | ||
|
|
||
| // 1 alloc (24 B) from []any interface boxing in Go runtime. | ||
| if allocs := testing.AllocsPerRun(1, func() { | ||
| _, _ = engine.Evaluate(rule, ctx) | ||
| }); allocs > 1 { | ||
| b.Fatalf("expected <= 1 allocs/op, got %f", allocs) | ||
| } |
There was a problem hiding this comment.
Enforce 0 allocs/op for the in benchmark.
This currently tolerates allocations, which violates the benchmark requirement.
Proposed fix
- // 1 alloc (24 B) from []any interface boxing in Go runtime.
if allocs := testing.AllocsPerRun(1, func() {
_, _ = engine.Evaluate(rule, ctx)
- }); allocs > 1 {
- b.Fatalf("expected <= 1 allocs/op, got %f", allocs)
+ }); allocs != 0 {
+ b.Fatalf("expected 0 allocs/op, got %f", allocs)
}As per coding guidelines, "Allocation Verification: All benchmarks must show 0 allocs/op".
🤖 Prompt for AI Agents
In `@benchmark_optimized_test.go` around lines 103 - 110, The benchmark currently
allows up to 1 allocation by checking "allocs > 1"; change the assertion to
enforce zero allocations by checking "allocs != 0" or "allocs > 0" and update
the failure message to "expected 0 allocs/op, got %f". Apply this change around
the testing.AllocsPerRun(1, func() { _, _ = engine.Evaluate(rule, ctx) }) block
and keep b.ReportAllocs() and the same call to engine.Evaluate(rule, ctx).
There was a problem hiding this comment.
The in operator benchmark correctly allows 1 allocation (24 B). This is a pre-existing behavior — not introduced by this PR. The allocation comes from Go's runtime boxing of a []any slice header (24 bytes) into an any interface field (EvalResult.OriginalValue).
There was a problem hiding this comment.
@lucasnakano Understood. You're right that the in operator's allocation is pre-existing and the check correctly reflects that. The 24 B allocation from boxing []any into the interface is a Go runtime behavior that applies to both the existing in operator and the new quantifier operations.
Since this is a known limitation and not a regression, the allocs > 1 check is appropriate for this benchmark.
🔥 Failed to add learnings
Learnings cannot be added due to server error. Please try again.
2 participants
Summary
Antes dessa mudança, o rule engine não conseguia operar sobre listas de objetos. O máximo que se podia fazer era verificar se uma lista existia (
selections pr). Não era possível contar elementos, nem verificar condições sobre os objetos dentro da lista.Isso era um bloqueio para casos de uso reais como validação de apostas, onde é necessário responder perguntas como:
O que foi adicionado
.length— Acessor de tamanho em arrays via dot notation:any/all/none— Quantificadores que avaliam uma sub-expressão contra cada elemento de uma lista:Os quantificadores suportam sub-expressões compostas (
and,or,not), aninhamento (lista dentro de lista), e combinação com.lengthe condições externas:Performance Impact
.lengthany(3 elementos)all(3 elementos)none(3 elementos)*A alocação de 24B vem do boxing de
[]anyemanyno Go runtime — mesmo padrão que o operadorinjá existente.Checklist
Summary by CodeRabbit
New Features
Tests
Documentation
Validation / Errors