Conversation
|
@codex Please review this PR |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c321135670
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if exp < 0 { | ||
| return 0 |
There was a problem hiding this comment.
Returning 0 for every negative exponent in intPow changes valid TraceQL results for cases that have exact integer answers (for example, 1 ^ -3 should stay 1, and -1 ^ -3 should stay -1). This is a behavioral regression introduced by this patch because negative literals are parseable in scalar expressions, so integer-power queries with negative exponents now produce incorrect values instead of the prior behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Negative powers were always broken AFAICT 🤔 maybe a different thing we may want to open a bug ticket for?
Proximyst
requested review from
carles-grafana,
electron0zero,
ie-pham,
javiermolinar,
mapno,
mattdurham,
mdisibio,
oleg-kozlyuk-grafana,
ruslan-mikhailov,
stoewer,
yvrhdn and
zalegrala
as code owners
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c321135670
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if exp < 0 { | ||
| return 0 |
There was a problem hiding this comment.
Returning 0 for all negative exponents changes query results in reachable paths, not just unsupported a^-b inputs. Fresh evidence: both integer OpPower call sites still invoke intPow(rhsN, lhsN) (pkg/traceql/ast_execute.go:480 and :729), so a negative left operand (for example (-2) ^ 3) now hits this branch and collapses to 0 for every positive RHS, whereas this patch is only intended to address hangs. That introduces incorrect filter/evaluation behavior for integer-power expressions over negative values.
Useful? React with 👍 / 👎.
|
Example query: Interesting, I'm not sure the rationale for the naive implementation, perhaps the for loop is faster than math.Pow on common (i.e. smallish) inputs. However, unless we know this is a bottleneck for certain queries, I think a straightforward Although it does overflow resulting in a negative, for that query, but it handles negative exponents correctly, i.e. 10^-1 should be integer 0, but this loop returns 1. |
|
I shyed away from Just for my own amusement, I'll try out some more AI, so will convert this to a draft while I do that :) |
|
@codex Can you update the implementation of the exponentiation function in this PR to use |
Summary
Testing
Also completed follow-up workflow actions:
|
|
@mdisibio I think this looks good. Impressed that Codex managed the tests :o |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 47428eac43
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| return result | ||
| func intPow(base, exp int) int { | ||
| return int(math.Pow(float64(base), float64(exp))) |
There was a problem hiding this comment.
Converting operands through math.Pow(float64(...)) makes integer exponentiation lose precision for many in-range results, because float64 cannot represent all integers above 2^53 exactly. For example, 2147483647^2 should be 4611686014132420609 but this path returns 4611686014132420608, so integer OpPower expressions can now silently produce wrong query/filter results in both scalar execution paths that call intPow.
Useful? React with 👍 / 👎.
|
Thanks, changelog and then can merge. |
|
Oh sorry, this also now has conflicts because I merged the other PR. |
|
@mdisibio I've updated the changelog but I keep running into flaky tests :S edit: as I comment this, of course the run passes 😂 |
There was a problem hiding this comment.
Pull request overview
Fixes a TraceQL performance bug where integer exponentiation (^) could hang by iterating O(exp) times for large exponents.
Changes:
- Replace the naive integer power loop with a
math.Pow-based implementation. - Add unit tests covering
intPow, including large-exponent cases intended to ensure the function completes quickly. - Add a changelog entry for the TraceQL bugfix.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/traceql/ast_execute.go | Reworks intPow implementation used by the integer ^ operator. |
| pkg/traceql/ast_execute_test.go | Adds a dedicated TestIntPow with small/edge/large exponent cases. |
| CHANGELOG.md | Records the bugfix in the changelog. |
| } | ||
| return result | ||
| func intPow(base, exp int) int { | ||
| return int(math.Pow(float64(base), float64(exp))) |
There was a problem hiding this comment.
Using math.Pow(float64(...)) for integer exponentiation changes semantics and can return incorrect results due to float64 rounding (e.g. (2^31-1)^2 cannot be represented exactly) and makes overflow behavior implementation-dependent when converting +Inf/NaN/out-of-range float64 back to int. Consider implementing an O(log exp) integer exponentiation-by-squaring version that keeps deterministic int wraparound behavior, and handle exp<0 explicitly (return 0 except base==±1, or return an error at the operator level). Also, current call sites pass intPow(rhsN, lhsN) (ast_execute.go:486/741), which is easy to misread with the new base/exp parameter names—worth aligning the argument order or renaming to avoid confusion.
| return int(math.Pow(float64(base), float64(exp))) | |
| if exp < 0 { | |
| switch base { | |
| case 1: | |
| return 1 | |
| case -1: | |
| if exp%2 == 0 { | |
| return 1 | |
| } | |
| return -1 | |
| default: | |
| return 0 | |
| } | |
| } | |
| result := 1 | |
| for exp > 0 { | |
| if exp&1 == 1 { | |
| result *= base | |
| } | |
| exp >>= 1 | |
| if exp > 0 { | |
| base *= base | |
| } | |
| } | |
| return result |
| {"large exponent", 2, 62, 1 << 62}, | ||
| // These cases would hang with a naive O(n) loop. | ||
| // They must complete near-instantly or go test -timeout will kill them. | ||
| {"100 ^ 100", 100, 100, math.MinInt64}, | ||
| {"maxint32 ^ 2", math.MaxInt32, 2, 4611686014132420608}, | ||
| {"2 ^ maxint32", 2, math.MaxInt32, math.MinInt64}, | ||
| {"maxint32 ^ maxint32", math.MaxInt32, math.MaxInt32, math.MinInt64}, | ||
| {"maxint64 ^ 2", math.MaxInt64, 2, math.MinInt64}, | ||
| {"2 ^ maxint64", 2, math.MaxInt64, math.MinInt64}, | ||
| {"maxint64 ^ maxint64", math.MaxInt64, math.MaxInt64, math.MinInt64}, |
There was a problem hiding this comment.
Several expectations here rely on float64 overflow / float64->int conversion behavior (e.g. expecting math.MinInt64 for huge results). That conversion is explicitly implementation-dependent in Go when the float64 value is out of int range or is ±Inf/NaN, so these test cases can be flaky across architectures / Go versions. Additionally, maxint32 ^ 2 is mathematically 4611686014132420609, but the test expects 4611686014132420608 (suggesting the implementation is rounding, not doing true integer pow). Could these “large exponent” cases instead assert that the call completes quickly, and/or compute a deterministic expected value based on the intended integer semantics (e.g., wraparound modulo 2^word using big.Int with bits.UintSize)?
What this PR does:
We currently have a naive implementation of integer powers. This hangs CPUs if given very large numbers. As there is no context propagation, that also does not stop when the requester gives up.
Note: the bug and fix were both LLM-driven.
Which issue(s) this PR fixes:
No issue was filed here.
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]