Advanced SQL Analytics Framework #9
Conversation
… vulnerability (#94942) * disable sql expressions remove duckdb ref * Run `make update-workspace` --------- Co-authored-by: Scott Lepper <[email protected]>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Summary
This PR implements an "Advanced SQL Analytics Framework" for Grafana's expression system by replacing an external DuckDB dependency with an internal SQL implementation. The changes remove the github.com/scottlepp/go-duck dependency and several related packages, introducing a new internal SQL engine structure.
The core changes include:
- New SQL DB Interface: Added
pkg/expr/sql/db.gowith aDBstruct that defines methods for SQL operations (TablesList,RunCommands,QueryFramesInto) - SQL Command Integration: Modified
pkg/expr/sql_command.goto use the new internalsql.NewInMemoryDB()instead of the external DuckDB implementation - SQL Parser Updates: Updated
pkg/expr/sql/parser.goto use the new internal DB for SQL analysis - Feature Flag Protection: Added feature flag checking in
pkg/expr/reader.gowithenableSqlExpressions()function - Dependency Cleanup: Removed DuckDB-related dependencies from
go.modand updated workspace dependencies ingo.work.sum
This change represents a strategic shift from relying on external SQL engines to building custom SQL analytics capabilities within Grafana's expression system. The framework is designed to work with Grafana's existing data frame processing pipeline and supports SQL queries over time series data.
Confidence score: 0/5
- This PR will cause immediate runtime failures and completely break SQL expression functionality
- All SQL operations now return "not implemented" errors, making the feature non-functional despite maintaining the API surface
- Critical logic bugs in feature flag implementation and missing return statements will cause additional failures
- Files need immediate attention:
pkg/expr/sql/db.go,pkg/expr/sql_command.go,pkg/expr/reader.go,pkg/expr/sql/parser.go
6 files reviewed, 5 comments
| // TablesList returns a list of tables for the sql statement | ||
| func TablesList(rawSQL string) ([]string, error) { | ||
| duckDB := duck.NewInMemoryDB() | ||
| duckDB := NewInMemoryDB() |
There was a problem hiding this comment.
logic: This now calls a stub implementation that always returns 'not implemented' error, breaking SQL parsing functionality
| // TablesList returns a list of tables for the sql statement | ||
| func TablesList(rawSQL string) ([]string, error) { | ||
| duckDB := duck.NewInMemoryDB() | ||
| duckDB := NewInMemoryDB() |
There was a problem hiding this comment.
logic: This now calls a stub implementation that always returns 'not implemented' error, breaking SQL parsing functionality
| func (db *DB) QueryFramesInto(name string, query string, frames []*data.Frame, f *data.Frame) error { | ||
| return errors.New("not implemented") | ||
| } |
There was a problem hiding this comment.
logic: This method is called directly by SQLCommand.Execute() but always fails, breaking SQL expressions when enabled
| } | ||
|
|
||
| func enableSqlExpressions(h *ExpressionQueryReader) bool { | ||
| enabled := !h.features.IsEnabledGlobally(featuremgmt.FlagSqlExpressions) |
There was a problem hiding this comment.
logic: Logic error: The double negation here means enabled will be true when the feature flag is OFF, which is counterintuitive
| enabled := !h.features.IsEnabledGlobally(featuremgmt.FlagSqlExpressions) | |
| enabled := h.features.IsEnabledGlobally(featuremgmt.FlagSqlExpressions) |
| // TablesList returns a list of tables for the sql statement | ||
| func TablesList(rawSQL string) ([]string, error) { | ||
| duckDB := duck.NewInMemoryDB() | ||
| duckDB := NewInMemoryDB() |
There was a problem hiding this comment.
logic: This now calls a stub implementation that always returns 'not implemented' error, breaking SQL parsing functionality
| if enabled { | ||
| return false | ||
| } | ||
| return false |
There was a problem hiding this comment.
logic: This function always returns false regardless of feature flag state. If this is intentional to disable the feature, add a comment explaining why
| if enabled { | |
| return false | |
| } | |
| return false | |
| if enabled { | |
| return true | |
| } | |
| return false |
| cmd := fmt.Sprintf("SELECT json_serialize_sql('%s')", rawSQL) | ||
| ret, err := duckDB.RunCommands([]string{cmd}) |
There was a problem hiding this comment.
logic: The json_serialize_sql function is specific to DuckDB and won't work with the stub implementation
| func (db *DB) TablesList(rawSQL string) ([]string, error) { | ||
| return nil, errors.New("not implemented") | ||
| } |
There was a problem hiding this comment.
logic: This method is called by sql.TablesList() in parser.go but always fails, preventing SQL query validation
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 2 weeks if no further activity occurs. Please feel free to give a status update or ping for review. Thank you for your contributions! |
|
This pull request has been automatically closed because it has not had any further activity in the last 2 weeks. Thank you for your contributions! |
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
2 participants
Test 9