Reporting jsonb subscripting issue in assessment and analyze #2129
Conversation
priyanshi-yb
changed the title
|
|
||
| func (p *ParserIssueDetector) getJsonbColumns() []string { | ||
| var jsonColumns []string | ||
| for _, mp := range p.columnsWithUnsupportedIndexDatatypes { |
There was a problem hiding this comment.
let's not re-use columnsWithUnsupportedIndexDatatypes to get jsonb columns here. If in the future, jsonb index gets supported, we'll have to remove it from that list, but we'll need it for detecting jsonb-related issues.
Can we use a different variable here?
Also, this means that we need to have a single instance of parserIssueDetector for both analyzeSchema as well as assess-migration AND that the unsupportedqueryconstructs are fetched AFTER the schema issues have been detected, correct? Can you verify that this is indeed happening?
There was a problem hiding this comment.
Also, this means that we need to have a single instance of parserIssueDetector for both analyzeSchema as well as assess-migration AND that the unsupportedqueryconstructs are fetched AFTER the schema issues have been detected, correct? Can you verify that this is indeed happening?
yes correct, we would need that because we rely on exported schema details for other things, e.g. datatypes (UDT, array etc..) so it is still the case that parserIssueDetector needs to be the same for analyze and assessment and run any other detection in assessment after exported schema is Parsed (ParseRequiredDDL()) .
There was a problem hiding this comment.
But if required all this can be abstracted out if we run the ParseRequiredDDL separately on exported schema for assessment.
There was a problem hiding this comment.
I think it's okay for now, it's a fair assumption to make, I suppose. just add a comment in code at the point where we're calling parserIssueDetector to get unsupported query constructs.
| func (mv *FunctionProcessor) Process(parseTree *pg_query.ParseResult) (DDLObject, error) { | ||
| funcNode, ok := getCreateFuncStmtNode(parseTree) | ||
| if !ok { | ||
| return nil, fmt.Errorf("not a CREATE VIEW statement") |
There was a problem hiding this comment.
not a CREATE FUNCTION
| SchemaName string | ||
| FuncName string | ||
| ReturnType string | ||
| IsReturnPercType bool |
There was a problem hiding this comment.
what is this field? doesn't seem to be used?
There was a problem hiding this comment.
yeah, not using it currently, will remove it.
| return aIndirection, ok | ||
| } | ||
|
|
||
| func GetGenericNode(msg protoreflect.Message) (*pg_query.Node, bool) { |
There was a problem hiding this comment.
not being used? remove?
| return nil | ||
| } | ||
|
|
||
| arg := aIndirectionNode.GetArg() |
There was a problem hiding this comment.
can you add a comment here explaining the indirection node and the arg, indices fields at a high level. Explain that we're essentially checking that the arg is a jsonb type
| type_name:{names:{string:{sval:"jsonb"}} typemod:-1 location:306} location:304}} | ||
| */ | ||
| typeCast := node.GetTypeCast() | ||
| typeName, _ := GetTypeNameAndSchema(typeCast.GetTypeName().GetNames()) |
There was a problem hiding this comment.
GetTypeNameAndSchema need not be exported anymore because you moved the entire function to queryparser? Same probably for a few other functions that you decided to export. Pls check.
| @@ -264,3 +268,69 @@ func IsDDL(parseTree *pg_query.ParseResult) (bool, error) { | |||
| //Not Full-proof as we don't have all DDL types but atleast we will skip all the types we know currently | |||
| return !ok, nil | |||
| } | |||
|
|
|||
| func IsJsonbType(node *pg_query.Node, jsonbColumns []string, jsonbFunctions []string) bool { | |||
There was a problem hiding this comment.
how about DoesNodeEvaluateToJsonB or something similar? add some doc to this function. Explain that we are checking if the particular node, when evaluated, results in a jsonb value.. could be a column, a typecase, a function call, an expression, etc,etc.
priyanshi-yb
force-pushed
the
priyanshi/report-json-subscripting
branch
from
f5de10d to
a0954f1
Compare
| `SELECT ('{"key": "value1"}'::jsonb || '{"key": "value2"}'::jsonb)['key'] AS object_in_array;`, | ||
| } | ||
|
|
||
| for _, sql := range withoutIssueSqls { |
There was a problem hiding this comment.
thanks for writing negative tests 👍
Describe the changes in this pull request
Reporting the JSONB subscription in the DMLs, DDLs and PLPGSQL objects.
The approach to detecting these is not defined, so I am doing my best effort to detect as many as possible in the source schema. There are still edge cases that can't be detected easily.
https://yugabyte.atlassian.net/browse/DB-14545
Describe if there are any user-facing changes
addition of a feature to the report-
Unsupported Feature
Unsupported Query Constructs
Unsupported PL/pgSQL objects
How was this pull request tested?
Unit tests
TODO: add end-to-end tests
Does your PR have changes that can cause upgrade issues?