-
Notifications
You must be signed in to change notification settings - Fork 11
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
Reporting CTE Materialized clause and Sql body in Function #2165
Changes from 13 commits
15b4e98
a9ab725
06108b6
3c0fee2
c3f291d
3058bf7
304b991
e89e77a
9a358df
012cdb9
72b73d8
fc1b227
1eb486e
f52074c
1659133
3ec1447
f252657
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 |
---|---|---|
|
@@ -1070,7 +1070,9 @@ func fetchUnsupportedPGFeaturesFromSchemaReport(schemaAnalysisReport utils.Schem | |
unsupportedFeatures = append(unsupportedFeatures, getUnsupportedFeaturesFromSchemaAnalysisReport(queryissue.JSONB_SUBSCRIPTING_NAME, "", queryissue.JSONB_SUBSCRIPTING, schemaAnalysisReport, false, "")) | ||
unsupportedFeatures = append(unsupportedFeatures, getUnsupportedFeaturesFromSchemaAnalysisReport(queryissue.FOREIGN_KEY_REFERENCES_PARTITIONED_TABLE_NAME, "", queryissue.FOREIGN_KEY_REFERENCES_PARTITIONED_TABLE, schemaAnalysisReport, false, "")) | ||
unsupportedFeatures = append(unsupportedFeatures, getUnsupportedFeaturesFromSchemaAnalysisReport(queryissue.JSON_TYPE_PREDICATE_NAME, "", queryissue.JSON_TYPE_PREDICATE, schemaAnalysisReport, false, "")) | ||
|
||
unsupportedFeatures = append(unsupportedFeatures, getUnsupportedFeaturesFromSchemaAnalysisReport(queryissue.SQL_BODY_IN_FUNCTION_NAME, "", queryissue.SQL_BODY_IN_FUNCTION, schemaAnalysisReport, false, "")) | ||
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. What about MATERIALIZED CTE issue? Can they not show up in DDLs? 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. yeah CTE can be in CREATE VIEW, adding it here as well |
||
unsupportedFeatures = append(unsupportedFeatures, getUnsupportedFeaturesFromSchemaAnalysisReport(queryissue.CTE_WITH_MATERIALIZED_CLAUSE_NAME, "", queryissue.CTE_WITH_MATERIALIZED_CLAUSE, schemaAnalysisReport, false, "")) | ||
|
||
return lo.Filter(unsupportedFeatures, func(f UnsupportedFeature, _ int) bool { | ||
return len(f.Objects) > 0 | ||
}), nil | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -554,3 +554,42 @@ func (j *JsonPredicateExprDetector) GetIssues() []QueryIssue { | |
} | ||
return issues | ||
} | ||
|
||
type CommonTableExpressionDetector struct { | ||
query string | ||
materializedClauseDetected bool | ||
} | ||
|
||
func NewCommonTableExpressionDetector(query string) *CommonTableExpressionDetector { | ||
return &CommonTableExpressionDetector{ | ||
query: query, | ||
} | ||
} | ||
|
||
func (c *CommonTableExpressionDetector) Detect(msg protoreflect.Message) error { | ||
if queryparser.GetMsgFullName(msg) != queryparser.PG_QUERY_CTE_NODE { | ||
return nil | ||
} | ||
/* | ||
with_clause:{ctes:{common_table_expr:{ctename:"cte" ctematerialized:CTEMaterializeNever | ||
ctequery:{select_stmt:{target_list:{res_target:{val:{column_ref:{fields:{a_star:{}} location:939}} location:939}} from_clause:{range_var:{relname:"a" inh:true relpersistence:"p" location:946}} limit_option:LIMIT_OPTION_DEFAULT op:SETOP_NONE}} location:906}} location:901} op:SETOP_NONE}} stmt_location:898 | ||
*/ | ||
cteNode, err := queryparser.ProtoAsCTENode(msg) | ||
if err != nil { | ||
return err | ||
} | ||
if cteNode.Ctematerialized != queryparser.CTE_MATERIALIZED_DEFAULT { | ||
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. Can you add a comment here explaining that both MATERIALIZED, NOT MATERIALIZED flags were not supported.. |
||
//MATERIALIZED / NOT MATERIALIZED clauses in CTE is not supported in YB | ||
c.materializedClauseDetected = true | ||
} | ||
return nil | ||
} | ||
|
||
func (c *CommonTableExpressionDetector) GetIssues() []QueryIssue { | ||
var issues []QueryIssue | ||
if c.materializedClauseDetected { | ||
issues = append(issues, NewCTEWithMaterializedIssue(DML_QUERY_OBJECT_TYPE, "", c.query)) | ||
} | ||
return issues | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -603,6 +603,25 @@ func (v *ViewIssueDetector) DetectIssues(obj queryparser.DDLObject) ([]QueryIssu | |
return issues, nil | ||
} | ||
|
||
// ================FUNCTION ISSUE DETECTOR ================== | ||
|
||
type FunctionIssueDetector struct{} | ||
|
||
func (v *FunctionIssueDetector) DetectIssues(obj queryparser.DDLObject) ([]QueryIssue, error) { | ||
function, ok := obj.(*queryparser.Function) | ||
if !ok { | ||
return nil, fmt.Errorf("invalid object type: expected View") | ||
} | ||
var issues []QueryIssue | ||
|
||
if function.HasSqlBody { | ||
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.
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.
yes BEGIN can also start a function block
No only 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. Right, but will 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. No, this |
||
//https://www.postgresql.org/docs/15/sql-createfunction.html#:~:text=a%20new%20session.-,sql_body,-The%20body%20of | ||
issues = append(issues, NewSqlBodyInFunctionIssue(function.GetObjectType(), function.GetObjectName(), "")) | ||
} | ||
|
||
return issues, nil | ||
} | ||
|
||
// ==============MVIEW ISSUE DETECTOR ====================== | ||
|
||
type MViewIssueDetector struct{} | ||
|
@@ -667,6 +686,8 @@ func (p *ParserIssueDetector) GetDDLDetector(obj queryparser.DDLObject) (DDLIssu | |
return &ViewIssueDetector{}, nil | ||
case *queryparser.MView: | ||
return &MViewIssueDetector{}, nil | ||
case *queryparser.Function: | ||
return &FunctionIssueDetector{}, nil | ||
case *queryparser.Collation: | ||
return &CollationIssueDetector{}, nil | ||
default: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -566,6 +566,20 @@ func NewForeignKeyReferencesPartitionedTableIssue(objectType string, objectName | |
return newQueryIssue(foreignKeyReferencesPartitionedTableIssue, objectType, objectName, SqlStatement, details) | ||
} | ||
|
||
var sqlBodyInFunctionIssue = issue.Issue{ | ||
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. please define a Description for each issue moving forward. |
||
Type: SQL_BODY_IN_FUNCTION, | ||
Name: SQL_BODY_IN_FUNCTION_NAME, | ||
Impact: constants.IMPACT_LEVEL_1, | ||
Description: "SQL Body for sql languages in function statement is not supported in YugabyteDB", | ||
Suggestion: "No workaround available", | ||
GH: "https://github.com/yugabyte/yugabyte-db/issues/25575", | ||
DocsLink: "https://docs.yugabyte.com/preview/yugabyte-voyager/known-issues/postgresql/#postgresql-12-and-later-features", | ||
} | ||
|
||
func NewSqlBodyInFunctionIssue(objectType string, objectName string, SqlStatement string) QueryIssue { | ||
return newQueryIssue(sqlBodyInFunctionIssue, objectType, objectName, SqlStatement, map[string]interface{}{}) | ||
} | ||
|
||
var uniqueNullsNotDistinctIssue = issue.Issue{ | ||
Type: UNIQUE_NULLS_NOT_DISTINCT, | ||
Name: UNIQUE_NULLS_NOT_DISTINCT_NAME, | ||
|
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.
This is also fixed in https://github.com/yugabyte/yb-voyager/pull/2201/files ?
Is that a prerequisite for this PR?
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.
yes otherwise analyze-schema/assessment won't be able to report the case as our parser will not work