-
Notifications
You must be signed in to change notification settings - Fork 9
Advanced SQL Analytics Framework #9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: data-analysis-features
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -126,6 +126,10 @@ func (h *ExpressionQueryReader) ReadQuery( | |||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| case QueryTypeSQL: | ||||||||||||||||||
| enabled := enableSqlExpressions(h) | ||||||||||||||||||
| if !enabled { | ||||||||||||||||||
| return eq, fmt.Errorf("sqlExpressions is not implemented") | ||||||||||||||||||
| } | ||||||||||||||||||
| q := &SQLExpression{} | ||||||||||||||||||
| err = iter.ReadVal(q) | ||||||||||||||||||
| if err == nil { | ||||||||||||||||||
|
|
@@ -186,3 +190,11 @@ func getReferenceVar(exp string, refId string) (string, error) { | |||||||||||||||||
| } | ||||||||||||||||||
| return exp, nil | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| func enableSqlExpressions(h *ExpressionQueryReader) bool { | ||||||||||||||||||
| enabled := !h.features.IsEnabledGlobally(featuremgmt.FlagSqlExpressions) | ||||||||||||||||||
| if enabled { | ||||||||||||||||||
| return false | ||||||||||||||||||
| } | ||||||||||||||||||
| return false | ||||||||||||||||||
|
Comment on lines
+196
to
+199
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: This function always returns false regardless of feature flag state. If this is intentional to disable the feature, add a comment explaining why
Suggested change
|
||||||||||||||||||
| } | ||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| package sql | ||
|
|
||
| import ( | ||
| "errors" | ||
|
|
||
| "github.com/grafana/grafana-plugin-sdk-go/data" | ||
| ) | ||
|
|
||
| type DB struct { | ||
| } | ||
|
|
||
| func (db *DB) TablesList(rawSQL string) ([]string, error) { | ||
| return nil, errors.New("not implemented") | ||
| } | ||
|
Comment on lines
+12
to
+14
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: This method is called by sql.TablesList() in parser.go but always fails, preventing SQL query validation |
||
|
|
||
| func (db *DB) RunCommands(commands []string) (string, error) { | ||
| return "", errors.New("not implemented") | ||
| } | ||
|
|
||
| func (db *DB) QueryFramesInto(name string, query string, frames []*data.Frame, f *data.Frame) error { | ||
| return errors.New("not implemented") | ||
| } | ||
|
Comment on lines
+20
to
+22
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: This method is called directly by SQLCommand.Execute() but always fails, breaking SQL expressions when enabled |
||
|
|
||
| func NewInMemoryDB() *DB { | ||
| return &DB{} | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,7 +8,6 @@ import ( | |
|
|
||
| "github.com/grafana/grafana/pkg/infra/log" | ||
| "github.com/jeremywohl/flatten" | ||
| "github.com/scottlepp/go-duck/duck" | ||
| ) | ||
|
|
||
| const ( | ||
|
|
@@ -21,7 +20,7 @@ var logger = log.New("sql_expr") | |
|
|
||
| // 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: This now calls a stub implementation that always returns 'not implemented' error, breaking SQL parsing functionality There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: This now calls a stub implementation that always returns 'not implemented' error, breaking SQL parsing functionality There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: This now calls a stub implementation that always returns 'not implemented' error, breaking SQL parsing functionality |
||
| rawSQL = strings.Replace(rawSQL, "'", "''", -1) | ||
| cmd := fmt.Sprintf("SELECT json_serialize_sql('%s')", rawSQL) | ||
| ret, err := duckDB.RunCommands([]string{cmd}) | ||
|
Comment on lines
25
to
26
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: The |
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Logic error: The double negation here means
enabledwill be true when the feature flag is OFF, which is counterintuitive