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

Added enhancement for Gin Index not supported queries in analyze-sche… #499

Merged
merged 3 commits into from
Oct 26, 2022

Conversation

priyanshi-yb
Copy link
Contributor

@priyanshi-yb priyanshi-yb commented Oct 20, 2022

…ma (#428)

Added conditions for Unsupported Gin index queries (Support ticket on DB side : yugabyte/yugabyte-db#7850) -

  1. Gin index on Multicolumn
  2. Gin index on column with ASC/DESC/HASH clause.

Added check for btree_gin extension which can be created which means it exists in the Database but is not supported with YB gin indexes (Support ticket on DB side: yugabyte/yugabyte-db#9958).

@@ -197,6 +198,30 @@ func reportSummary() {
reportStruct.Summary.DBObjects = append(reportStruct.Summary.DBObjects, dbObject)
}
}
// Checks Whether there is a GIN index
Copy link
Contributor

Choose a reason for hiding this comment

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

In the comment, can you show examples (with full SQL DDL stmt) of all the scenarios reported by this function?

// Checks Whether there is a GIN index
func checkGin(sqlInfoArr []sqlInfo, fpath string) {
for _, sqlInfo := range sqlInfoArr {
idx := ginRegex.FindStringSubmatch(sqlInfo.stmt)
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 a general practice to name the output of regex methods as match.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I maintained the consistency as every where in the file, the output of regex methods is idx, I can change that for now only here and will take care of other places in the refactoring.

Comment on lines 206 to 208
columnsFromGin := strings.Trim(idx[4], `()`)
columnList := strings.Split(columnsFromGin, ",")
if len(columnList) > 1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it enough to check for the presence of a comma to conclude that it is an index on multiple columns?

}
}
if strings.Contains(strings.ToLower(sqlInfo.stmt), "extension btree_gin") {
reportCase(fpath, "Schema contains btree_gin extension which is not supported for YB",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you define a local anonymous function to factor out common values:

reportFn := function(msg string, ghIssueID int, name string) {
    reportCase(fpath, msg, fmt.Sprintf(""https://github.com/yugabyte/yugabyte-db/issues/%d", ghIssueID), "INDEX", name, sqlInfo.formattedStmtStr)
}

Copy link
Contributor Author

@priyanshi-yb priyanshi-yb Oct 26, 2022

Choose a reason for hiding this comment

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

@amit-yb now this should be considered in the refactoring of analyzeSchema later?

@priyanshi-yb priyanshi-yb merged commit fc8e09f into main Oct 26, 2022
@priyanshi-yb priyanshi-yb deleted the priyanshi/analyzeSchema_enhancement branch October 26, 2022 10:52
chetank-yb pushed a commit that referenced this pull request Nov 11, 2022
#499)

* Added enhancement for Gin Index not supported queries in analyze-schema (#428)

* added condition for btree_gin extension which is unsupported

* changes after review
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.

2 participants