Advanced SQL Analytics Framework

Hi there 👋

🌥️ Tech	🌤️ Feat
1 high, 1 medium, 2 low	2 ok, 2 mitigated

The PR disables SQL expressions to remediate CVE-2024-9264 (RCE/LFI) and removes the DuckDB dependency. The security behavior is correct: SQL expressions are unconditionally disabled in all three gates (reader.go, nodes.go/sql_command.go, and the stub DB layer), and no bypass was found. The removal of the DuckDB import and go.mod entry is complete. However, the implementation introduces several maintainability and security-adjacent concerns that should be cleaned up before merge: the toggle-gating function in reader.go contains dead code with semantically inverted logic that could mislead future contributors into re-enabling the vulnerable path; the stub DB methods lack explanatory comments about why they are disabled; parser.go retains dead DuckDB serialization scaffolding; and there is an unreachable branch in sql_command.go. The documentation checklist item remains unchecked, and the feature toggle is now effectively ignored. These issues are non-blocking for the security fix itself but are recommended for change before final approval.

---

These might need a close look

• 🔴 pkg/expr/reader.go (L195)
  Function enableSqlExpressions is dead code with inverted logic and ignores the feature toggle. It always returns false regardless of IsEnabledGlobally, and the local variable enabled is assigned !IsEnabledGlobally(...), meaning it is true when the flag is off. This is confusing and a latent security landmine: a future developer who 'fixes' the function to if enabled { return true } would re-open the CVE-2024-9264 RCE/LFI path, and would enable SQL exactly when the flag is off (inverted). Replace the body with return false plus a comment stating SQL expressions are permanently disabled for security.

Worth checking

• 🟡 pkg/expr/sql/db.go (L24)
  The new DB type and its methods (TablesList, RunCommands, QueryFramesInto, NewInMemoryDB) all fail with 'not implemented' but carry no explanation of why. A future contributor could reasonably 'complete' these stubs with a real backend (e.g., re-introducing go-duck) and silently resurrect the vulnerability. Add a comment documenting that SQL expression execution is intentionally disabled for CVE-2024-9264 and that a working DuckDB backend must not be wired back in.

Small things (take or leave)

• 🔵 pkg/expr/sql_command.go (L96)
  In the disabled path, db.QueryFramesInto returns 'not implemented' and sets rsp.Error, which is fine as defense-in-depth. However, the if frame.Rows() == 0 { rsp.Values = NoData{...} } block is dead code because rsp.Values is unconditionally overwritten with TableData right after. This is pre-existing but worth a drive-by cleanup to avoid confusion in an otherwise-disabled command.
• 🔵 pkg/expr/sql/parser.go (L23)
  TablesList still contains dead DuckDB serialization scaffolding: it performs strings.Replace, builds SELECT json_serialize_sql('%s'), and calls duckDB.RunCommands, all of which funnel into the stub's 'not implemented' error. The helper functions tablesFromAST/astError/existsInList are unreachable. This is not harmful but makes the code read as though DuckDB parsing still happens. Simplify TablesList to return the 'not implemented' error directly to make it unambiguous that no SQL parsing is performed.

---

✅ Feature-level checklist

Looks good

• ✅ Remove all references to DuckDB from the codebase (go.mod, imports, and code).
  The go-duck dependency is removed from go.mod and imports; only a harmless checksum entry remains in go.work.sum. No remaining duck. usages exist.
• ✅ Disable SQL Expressions to prevent the CVE-2024-9264 RCE/LFI vulnerability.
  SQL expressions are unconditionally disabled in all three independent paths (reader.go, nodes.go/sql_command.go, and the stub DB layer). No bypass was found; the functional outcome is secure.

Partially covered

• ⚠️ If this is a pre-GA feature, it should be behind a feature toggle.
  The FlagSqlExpressions toggle still exists but is now ignored; enableSqlExpressions always returns false regardless of the flag. While this is acceptable for a security disable, the dead code and inverted logic in reader.go misleadingly suggest the toggle is honored, which could lead to accidental re-enablement. Consider simplifying the code to explicitly state the feature is permanently disabled.
• ⚠️ The docs should be updated, and if it is a notable improvement, added to What's New.
  The PR description notes the documentation is not being updated in this PR (other than the existing flag mention). This is acceptable for a security backport, but the checklist item remains unchecked and should be addressed in a follow-up to keep the documentation accurate.