Skip to content
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 a separate issue for all the different unsupported datatypes for the issue index on complex datatypes in schema report and assessment report #2288

Merged
merged 23 commits into from
Feb 25, 2025

Conversation

ShivanshGahlot
Copy link
Collaborator

@ShivanshGahlot ShivanshGahlot commented Feb 4, 2025

Describe the changes in this pull request

Describe if there are any user-facing changes

Assessment Report will now have separate issues for all the complex datatypes:
image

Schema analysis report will have separate issues for all the complex datatypes:
image

How was this pull request tested?

Manually

Does your PR have changes that can cause upgrade issues?

Component Breaking changes?
MetaDB No
Name registry json No
Data File Descriptor Json No
Export Snapshot Status Json No
Import Data State No
Export Status Json No
Data .sql files of tables No
Export and import data queue No
Schema Dump No
AssessmentDB No
Sizing DB No
Migration Assessment Report Json No
Callhome Json No
YugabyteD Tables No
TargetDB Metadata Tables No

@ShivanshGahlot ShivanshGahlot force-pushed the shivansh/assess-migration-payload branch from fa91bcd to 931918f Compare February 18, 2025 11:03
@@ -1241,6 +1241,59 @@ var includeObjectNameInCallhomePayloadForIssueTypes = []string{
UNSUPPORTED_EXTENSION_ISSUE_TYPE,
}

// analyze issue reasons to modify the reason before sending to callhome as will have sensitive information
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we're using this anywhere...
@sanyamsinghal to confirm.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah seems like a miss during merge from my side. Removed it now

}

// analyze issue reasons to send the object names for to callhome
var reasonsToSendObjectNameToCallhome = []string{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same for this. Not using anywhere. Perhaps a miss during merge?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. Removed it.

}
}
return indexesOnComplexTypesFeature
log.Infof("fetching unsupported features for Index on complex datatypes...")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is just for backwards compatibility (to support yugabyted). RIght?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, and also we are appending the assessment issue in this function getUnsupportedFeaturesFromSchemaAnalysisReport itself, so we need to call this for all various issue types

func NewIndexOnComplexDatatypesIssue(objectType string, objectName string, sqlStatement string, typeName string) QueryIssue {
issue := indexOnComplexDatatypesIssue
issue.Description = fmt.Sprintf(issue.Description, typeName)
func NewIndexOnArrayDatatypeIssue(objectType string, objectName string, sqlStatement string) QueryIssue {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's add tests for each of these types in issues_ddl_test.go to assert that creating an index actually fails on YB.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added tests

INDEX_ON_POLYGON_DATATYPE_ISSUE_SUGGESTION = "Refer to the docs link for the workaround"
INDEX_ON_TXID_SNAPSHOT_DATATYPE_ISSUE_SUGGESTION = "Refer to the docs link for the workaround"
INDEX_ON_ARRAY_DATATYPE_ISSUE_SUGGESTION = "Refer to the docs link for the workaround"
INDEX_ON_USER_DEFINED_DATATYPE_ISSUE_SUGGESTION = "Refer to the docs link for the workaround"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is okay to have a single suggestion constant for this as it is same for all

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

INDEX_ON_POLYGON_DATATYPE_ISSUE_DESCRIPTION = "Indexes on columns with polygon data type are not yet supported in YugabyteDB."
INDEX_ON_TXID_SNAPSHOT_DATATYPE_ISSUE_DESCRIPTION = "Indexes on columns with txid_snapshot data type are not yet supported in YugabyteDB."
INDEX_ON_ARRAY_DATATYPE_ISSUE_DESCRIPTION = "Indexes on columns with array data type are not yet supported in YugabyteDB."
INDEX_ON_USER_DEFINED_DATATYPE_ISSUE_DESCRIPTION = "Indexes on columns with user defined data types are not yet supported in YugabyteDB."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For user-defined type, we can mention the type name in the description as in the description field we can have dynamic data or the schema information in that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey we weren't mentioning the UDT type name before also I guess.

@@ -399,10 +399,10 @@ func (d *IndexIssueDetector) DetectIssues(obj queryparser.DDLObject) ([]QueryIss
if !ok {
continue
}
issues = append(issues, NewIndexOnComplexDatatypesIssue(
fmt.Printf("I was here with %s", typeName)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove comment

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

obj.GetObjectType(),
index.GetObjectName(),
"",
reportTypeName,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can still send this type name to the NewIndexOnUserDefinedTypeIssue and put this detail in the description

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey here we were sending "user_defined_type" as reportTypeName in case of UDT. We were not really sending any details about the UDT.

Copy link
Contributor

@priyanshi-yb priyanshi-yb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
Verify once with the local callhome setup if the issues are coming up properly with specific types and without sensitive information.

}

_, err = conn.Exec(ctx, testCase.sql)
fmt.Println("Query executed: ", testCase.sql)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove print

}
}
return indexesOnComplexTypesFeature
log.Infof("fetching unsupported features for Index on complex datatypes...")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, and also we are appending the assessment issue in this function getUnsupportedFeaturesFromSchemaAnalysisReport itself, so we need to call this for all various issue types

@@ -655,9 +655,9 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are a couple of other schemas as well mgi and rna that are not in GH, so please verify those as well once

@ShivanshGahlot ShivanshGahlot merged commit ec1cb4f into main Feb 25, 2025
66 checks passed
@ShivanshGahlot ShivanshGahlot deleted the shivansh/assess-migration-payload branch February 25, 2025 05:18
shaharuk-yb pushed a commit that referenced this pull request Feb 27, 2025
…s for the issue index on complex datatypes in schema report and assessment report (#2288)

- Earlier indexes on all the complex datatypes were reported under a single issue type: INDEX ON COMPLEX DATATYPES
- Added a separate issue for all such complex datatypes and removed the original issue.
- Reporting the issue in both the analyze schema report and the assessment report.
- Fixed issue: Define separate Issue for each case related to datatype(unsupported datatype, index on complex datatype) #2231
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants