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

Miscellaneous changes in export schema transformations #2378

Merged
merged 3 commits into from
Mar 6, 2025

Conversation

sanyamsinghal
Copy link
Collaborator

@sanyamsinghal sanyamsinghal commented Mar 5, 2025

Describe the changes in this pull request

  • fixed the console output whether recommendations are applied or not.
  • regression: added --skip-recommendations flag back

Added unit test for testing pg parser Deparsing API
Fixing bug https://yugabyte.atlassian.net/browse/DB-15613 is dependent on the upstream release.

Sample console output:
image
image

Describe if there are any user-facing changes

Console output wrt recommendations applied or not.

How was this pull request tested?

Does your PR have changes that can cause upgrade issues?

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

@sanyamsinghal sanyamsinghal self-assigned this Mar 5, 2025
@sanyamsinghal sanyamsinghal changed the title Added more unit test for testing pg parser Deparsing API Added unit test for testing pg parser Deparsing API Mar 5, 2025
- fixed the console output whether recommendations are applied or not.
- regression: added --skip-recommendations flag back
@sanyamsinghal sanyamsinghal changed the title Added unit test for testing pg parser Deparsing API Miscellaneous changes in export schema transformations Mar 5, 2025
@sanyamsinghal
Copy link
Collaborator Author

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 with some nits

Comment on lines 316 to 320
if !skipRecommendations {
transformations = append(transformations, applyShardedTableTransformation) // transform #1
transformations = append(transformations, applyMergeConstraintsTransformation) // transform #2
} else {
transformations = append(transformations, applyMergeConstraintsTransformation) // transform #1
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

if !skipRecommendations {
	transformations = append(transformations, applyShardedTableTransformation)     // transform #1
}
transformations = append(transformations, applyMergeConstraintsTransformation) // transform #1

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

newStmts, err := transformFn(transformedStmts)
if err != nil {
transformFuncName := utils.GetFuncName(transformFn)
log.Infof("applying transformation: %s on %s", filepath.Base(transformFuncName), filePath)
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe filepath.Base is for the filePath arg?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

transformFuncName also comes as fullpath to the function name

@@ -122,6 +122,7 @@ func (t *Transformer) MergeConstraints(stmts []*pg_query.RawStmt) ([]*pg_query.R
Otherwise, add it to the result slice
*/
alterTableCmdType := alterTableCmd.GetSubtype()
log.Infof("alterTableCmdType: %v", *alterTableCmdType.Enum())
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this required as Info log?

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 might be useful.

@@ -146,6 +147,7 @@ func (t *Transformer) MergeConstraints(stmts []*pg_query.RawStmt) ([]*pg_query.R
if !ok {
return nil, fmt.Errorf("CREATE TABLE stmt not found for table %v", objectName)
}
log.Infof("merging constraint %v into CREATE TABLE for object %v", constrType, objectName)
Copy link
Contributor

Choose a reason for hiding this comment

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

can this be debug log?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this might help in debugging so kept it for default log level.

@@ -103,6 +103,7 @@ func ora2pgExtractSchema(source *Source, exportDir string, schemaDir string) {
}
}
}
fmt.Println()
Copy link
Contributor

Choose a reason for hiding this comment

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

remove?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i added it explicitly to print a empty line after the schema export. To make it more readable.

@@ -113,8 +113,8 @@ func (pg *PostgreSQL) Connect() error {
err := pg.db.Ping()
if err == nil {
log.Infof("Already connected to the source database")
log.Infof("Already connected to the source database")
return nil
log.Infof("Already connected to the source database")
Copy link
Contributor

Choose a reason for hiding this comment

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

repeated log

@sanyamsinghal sanyamsinghal merged commit 7d21b3e into main Mar 6, 2025
66 checks passed
@sanyamsinghal sanyamsinghal deleted the sanyam/db-15613 branch March 6, 2025 06:34
sanyamsinghal added a commit that referenced this pull request Mar 6, 2025
sanyamsinghal added a commit that referenced this pull request Mar 6, 2025
* Revert "Miscellaneous changes in export schema transformations (#2378)"
- This reverts commit 7d21b3e.


* Revert "Tech Debt: Merge MergeConstraints and ApplyShardedRecommendations transformation on exported schema (#2366)"
- This reverts commit 2d6ffd4.
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