FETCH .. WITH TIES issue detection #2111
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,8 +16,12 @@ limitations under the License. | |
| package queryissue | ||
|
|
||
| import ( | ||
| "fmt" | ||
|
|
||
| mapset "github.com/deckarep/golang-set/v2" | ||
| pg_query "github.com/pganalyze/pg_query_go/v6" | ||
| log "github.com/sirupsen/logrus" | ||
| "google.golang.org/protobuf/proto" | ||
| "google.golang.org/protobuf/reflect/protoreflect" | ||
|
|
||
| "github.com/yugabyte/yb-voyager/yb-voyager/src/query/queryparser" | ||
|
|
@@ -195,3 +199,46 @@ func (d *RangeTableFuncDetector) GetIssues() []QueryIssue { | |
| } | ||
| return issues | ||
| } | ||
|
|
||
| type SelectStmtDetector struct { | ||
| query string | ||
| limitOptionWithTiesDetected bool | ||
| } | ||
|
|
||
| func NewSelectStmtDetector(query string) *SelectStmtDetector { | ||
| return &SelectStmtDetector{ | ||
| query: query, | ||
| } | ||
| } | ||
|
|
||
| func (d *SelectStmtDetector) getSelectStmtFromProto(msg protoreflect.Message) (*pg_query.SelectStmt, error) { | ||
| protoMsg := msg.Interface().(proto.Message) | ||
| selectStmtNode, ok := protoMsg.(*pg_query.SelectStmt) | ||
| if !ok { | ||
| return nil, fmt.Errorf("failed to cast %s to %s", queryparser.PG_QUERY_SELECTSTMT_NODE, queryparser.PG_QUERY_SELECTSTMT_NODE) | ||
| } | ||
| return selectStmtNode, nil | ||
| } | ||
|
There was a problem hiding this comment. Lets add this function in |
||
|
|
||
| func (d *SelectStmtDetector) Detect(msg protoreflect.Message) error { | ||
| if queryparser.GetMsgFullName(msg) == queryparser.PG_QUERY_SELECTSTMT_NODE { | ||
| selectStmtNode, err := d.getSelectStmtFromProto(msg) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| // checks if a SelectStmt node uses a FETCH clause with TIES | ||
| // https://www.postgresql.org/docs/13/sql-select.html#SQL-LIMIT | ||
| if selectStmtNode.LimitOption == pg_query.LimitOption_LIMIT_OPTION_WITH_TIES { | ||
|
There was a problem hiding this comment. define this limit option const There was a problem hiding this comment. Got it. Don't think we can completely avoid this because the return type would be a pg_query.SelectStmt. But we probably don't need to have an explicit dependency. But broadly speaking, agree with not preferring to use pg_query directly here. (as much as possilble). There was a problem hiding this comment. Yes, as much as possible we should avoid using it directly in the queryissue. |
||
| d.limitOptionWithTiesDetected = true | ||
| } | ||
| } | ||
| return nil | ||
| } | ||
|
There was a problem hiding this comment. I am thinking can we stick to one of the approaches (protomsg) as a primary approach, and use other one where its significantly making things easy, other code might become a bit messy. Here in this case i guess something like There was a problem hiding this comment. I see your point about consistency. But why, in general, do you prefer accessing fields of a message via protoreflect.Message rather than structs directly?
I doubt there is significant overhead. It's just type assertion. There was a problem hiding this comment. Merged this for now, since it's been pending for a while and I had to resolve a lot of conflicts! There was a problem hiding this comment.
@makalaaneesh Consistency here is another reason. |
||
|
|
||
| func (d *SelectStmtDetector) GetIssues() []QueryIssue { | ||
| var issues []QueryIssue | ||
| if d.limitOptionWithTiesDetected { | ||
| issues = append(issues, NewFetchWithTiesIssue(DML_QUERY_OBJECT_TYPE, "", d.query)) | ||
| } | ||
| return issues | ||
| } | ||
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.
nit: maybe "FETCH .. WITH TIES Clause"?