Automated Test: advanced-sql-analytics #339
Conversation
… vulnerability (#94942) * disable sql expressions remove duckdb ref * Run `make update-workspace` --------- Co-authored-by: Scott Lepper <scott.lepper@gmail.com>
📝 WalkthroughWalkthroughThis PR removes an external DuckDB dependency and replaces it with a local in-memory SQL database implementation. Additionally, a runtime check is introduced to gate SQL expression functionality, currently returning "not implemented" to block the feature. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/expr/sql_command.go (1)
110-118:⚠️ Potential issue | 🟠 MajorDead code:
NoDatabranch is immediately overwritten.When
frame.Rows() == 0, lines 111-113 setrsp.ValuestoNoData, but execution falls through to lines 116-118 which unconditionally overwritersp.ValueswithTableData. TheNoDatacase is never returned. This appears to be a pre-existing bug — likely a missingelseor earlyreturn.Proposed fix
if frame.Rows() == 0 { rsp.Values = mathexp.Values{ mathexp.NoData{Frame: frame}, } - } - - rsp.Values = mathexp.Values{ - mathexp.TableData{Frame: frame}, + } else { + rsp.Values = mathexp.Values{ + mathexp.TableData{Frame: frame}, + } }
🤖 Fix all issues with AI agents
In `@pkg/expr/reader.go`:
- Around line 194-200: The function enableSqlExpressions currently always
returns false and inverts the feature flag; fix it by either (A) making the
intent explicit to permanently block SQL expressions by replacing the body with
a single unambiguous return false, or (B) gating correctly on the feature flag
by returning the direct result of
h.features.IsEnabledGlobally(featuremgmt.FlagSqlExpressions) so
enableSqlExpressions(h *ExpressionQueryReader) reflects the flag; update the
implementation of enableSqlExpressions and remove the inverted logic to avoid
future confusion.
🧹 Nitpick comments (1)
pkg/expr/reader.go (1)
128-138: Gate correctly blocks the SQL path, but error message could be more informative.The gate works as intended since
enableSqlExpressionsalways returnsfalse. Minor suggestion: include a hint that this is behind a feature flag so operators know it's intentionally disabled.- return eq, fmt.Errorf("sqlExpressions is not implemented") + return eq, fmt.Errorf("SQL expressions are not enabled; this feature is not yet implemented")
| func enableSqlExpressions(h *ExpressionQueryReader) bool { | ||
| enabled := !h.features.IsEnabledGlobally(featuremgmt.FlagSqlExpressions) | ||
| if enabled { | ||
| return false | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
enableSqlExpressions always returns false — logic is misleading and likely buggy.
This function unconditionally returns false regardless of the feature flag state:
enabled := !h.features.IsEnabledGlobally(...)— the negation inverts the meaning, soenabledistruewhen the flag is off.- Both the
if enabledbranch and the fallthrough returnfalse.
If the intent is to permanently block SQL expressions for now, simplify to return false to make the intent clear. If the intent is to eventually gate on the feature flag, the correct implementation would be:
func enableSqlExpressions(h *ExpressionQueryReader) bool {
return h.features.IsEnabledGlobally(featuremgmt.FlagSqlExpressions)
}The current code is a maintenance hazard — a future developer removing one of the return false statements would still face the inverted boolean, resulting in the feature being enabled when the flag is off and disabled when it's on.
Proposed fix (if intent is to always block)
func enableSqlExpressions(h *ExpressionQueryReader) bool {
- enabled := !h.features.IsEnabledGlobally(featuremgmt.FlagSqlExpressions)
- if enabled {
- return false
- }
return false
}Proposed fix (if intent is to respect the feature flag)
func enableSqlExpressions(h *ExpressionQueryReader) bool {
- enabled := !h.features.IsEnabledGlobally(featuremgmt.FlagSqlExpressions)
- if enabled {
- return false
- }
- return false
+ return h.features.IsEnabledGlobally(featuremgmt.FlagSqlExpressions)
}🤖 Prompt for AI Agents
In `@pkg/expr/reader.go` around lines 194 - 200, The function enableSqlExpressions
currently always returns false and inverts the feature flag; fix it by either
(A) making the intent explicit to permanently block SQL expressions by replacing
the body with a single unambiguous return false, or (B) gating correctly on the
feature flag by returning the direct result of
h.features.IsEnabledGlobally(featuremgmt.FlagSqlExpressions) so
enableSqlExpressions(h *ExpressionQueryReader) reflects the flag; update the
implementation of enableSqlExpressions and remove the inverted logic to avoid
future confusion.
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit