-
Notifications
You must be signed in to change notification settings - Fork 18
Update fbcopy.cpp #21
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
base: master
Are you sure you want to change the base?
Conversation
Added default value when A option is set
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.
Pull request overview
This PR adds support for extracting and including default column values in the generated SQL statements when using the -A (ALTER) operation in fbcopy. The changes involve fetching the RDB$DEFAULT_SOURCE field from Firebird system tables and inserting it into the generated DDL output between the column type and NOT NULL constraint.
Key changes:
- Added missing
tr2->Commit()call to properly commit the destination database transaction incopyGeneratorValues() - Modified SQL query to fetch
RDB$DEFAULT_SOURCEfrom system tables - Updated
getDatatype()function to extract and format default values for all column types
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fbcopy/fbcopy.cpp
Outdated
| std::string default_source; | ||
| if (!st1->IsNull(7) && not_nulls) { | ||
| st1->Get(7, default_source); | ||
| } | ||
| else | ||
| default_source = ""; |
Copilot
AI
Nov 28, 2025
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.
[nitpick] The else clause only executes when the default is null OR when not_nulls is false, but only initializes default_source in those cases. Consider initializing default_source to empty string at declaration (line 962) and only populating it when conditions are met. This makes the logic clearer and avoids the unnecessary else clause.
| std::string default_source; | |
| if (!st1->IsNull(7) && not_nulls) { | |
| st1->Get(7, default_source); | |
| } | |
| else | |
| default_source = ""; | |
| std::string default_source = ""; | |
| if (!st1->IsNull(7) && not_nulls) { | |
| st1->Get(7, default_source); | |
| } |
| retval << " " << default_source << " "; | ||
| retval << null_flag; |
Copilot
AI
Nov 28, 2025
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.
Inconsistent indentation: this line uses tabs instead of spaces. The codebase consistently uses spaces for indentation (4 spaces per level). Replace the leading tab with 8 spaces to match surrounding code.
| retval << " " << default_source << " "; | |
| retval << null_flag; | |
| retval << " " << default_source << " "; | |
| retval << null_flag; |
| retval << " " << default_source << " "; | ||
| retval << null_flag; |
Copilot
AI
Nov 28, 2025
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.
Inconsistent indentation: this line uses tabs instead of spaces. The codebase consistently uses spaces for indentation (4 spaces per level). Replace the leading tab with 8 spaces to match surrounding code.
| retval << " " << default_source << " "; | |
| retval << null_flag; | |
| retval << " " << default_source << " "; | |
| retval << null_flag; |
| else | ||
| retval << "Numeric(18,0)"; | ||
|
|
||
| retval << " " << default_source << " "; |
Copilot
AI
Nov 28, 2025
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.
Potential issue: default_source may be empty, resulting in extra spaces in the output (e.g., 'Integer NOT NULL'). Consider conditionally appending default_source only when it's non-empty, or handle the spacing logic more carefully to avoid double spaces.
| retval << " " << default_source << " "; | ||
| retval << null_flag; |
Copilot
AI
Nov 28, 2025
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.
Inconsistent indentation: this line uses tabs instead of spaces. The codebase consistently uses spaces for indentation (4 spaces per level). Replace the leading tabs with 12 spaces to match the expected indentation level.
| retval << " " << default_source << " "; | |
| retval << null_flag; | |
| retval << " " << default_source << " "; | |
| retval << null_flag; |
| retval << "," << -scale << ")"; | ||
|
|
||
| retval << " " << default_source << " "; | ||
| retval << null_flag; |
Copilot
AI
Nov 28, 2025
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.
Inconsistent indentation: this line uses tabs instead of spaces. The codebase consistently uses spaces for indentation (4 spaces per level). Replace the leading tabs with 16 spaces to match the expected indentation level.
| retval << null_flag; | |
| retval << null_flag; |
| retval << "," << -scale << ")" << null_flag; | ||
| retval << "," << -scale << ")"; | ||
|
|
||
| retval << " " << default_source << " "; |
Copilot
AI
Nov 28, 2025
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.
Potential issue: default_source may be empty, resulting in extra spaces in the output (e.g., 'Decimal(18,2) NOT NULL'). Consider conditionally appending default_source only when it's non-empty, or handle the spacing logic more carefully to avoid double spaces.
| if (datatype == 261) // blob | ||
| retval << " sub_type " << subtype; | ||
|
|
||
| retval << " " << default_source << " "; |
Copilot
AI
Nov 28, 2025
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.
Potential issue: default_source may be empty, resulting in extra spaces in the output (e.g., 'Varchar(50) NOT NULL'). Consider conditionally appending default_source only when it's non-empty, or handle the spacing logic more carefully to avoid double spaces. A better approach would be: if (!default_source.empty()) retval << ' ' << default_source;
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Added default value when A option is set