Advanced SQL Analytics Framework

[zaibkhan]

Mirrors ai-code-review-evaluation#9 for like-for-like benchmarking.

• Base: data-analysis-features
• Head: advanced-sql-analytics

Original PR excerpt:

Test 9

[codoki-pr-intelligence]

Codoki PR Review

Summary: Fix SQL feature gating, unblock SQL expressions
What’s good: Clean separation for SQL via a dedicated DB type and feature gating scaffolding.
Review Status: ❌ Requires changes
Overall Priority: High

Issues (Critical & High only)

Severity	Issue	Why it matters
High	Correctness — Feature flag check inverted disables SQL expressions	…/expr/reader.go The feature flag logic is inverted and the function always returns false (including the unconditional returns below), which permanently disables SQL expressions and forces the QueryTypeSQL branch to error. Return the flag value directly so enabling the feature actually activates the path.

Showing top 1 issues. Critical: 0, High: 1. See inline suggestions for more.

Key Feedback (click to expand)

• Needs improvement: The feature flag logic for SQL expressions is inverted and always disables the path, returning 'sqlExpressions is not implemented' even when enabled.
• Testing: Add a unit test for enableSqlExpressions to assert behavior when the feature flag is on vs off, and a ReadQuery test that covers the QueryTypeSQL branch for both states.
• Compatibility: Removing go-duck and switching to a new in-memory DB without implementations will break any code path that relies on TablesList/RunCommands/QueryFramesInto once enabled. Ensure rollout plan or guard remains until the replacement is finished.
• Open questions: Should SQL expressions be globally disabled right now, or is the intent to allow them when featuremgmt.FlagSqlExpressions is enabled?

Confidence: 3/5 — Needs work before merge (1 high · status: Requires changes)

React with 👍 or 👎 if you found this review useful.

[codoki-pr-intelligence]

⚠️ High: The feature flag logic is inverted and the function always returns false (including the unconditional returns below), which permanently disables SQL expressions and forces the QueryTypeSQL branch to error. Return the flag value directly so enabling the feature actually activates the path.

Suggested change

	func enableSqlExpressions(h *ExpressionQueryReader) bool {
	func enableSqlExpressions(h *ExpressionQueryReader) bool {
	return h.features.IsEnabledGlobally(featuremgmt.FlagSqlExpressions)
	}