-
Notifications
You must be signed in to change notification settings - Fork 0
Revert 102 revert 98 numeric #6
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
Review SummarySkipped posting 1 drafted comments based on your review threshold. Feel free to update them here. Draft Comments.github/workflows/integration-tests-against-emulator.yaml:26-26Scores:
Reason for filtering: The comment meets the minimum threshold for inclusion Analysis: This is a simple typo fix in a comment that has no impact on production functionality. The fix is extremely clear and specific, providing the exact correction needed. However, since it's just a typo in a comment, it has very low urgency. With a total score of 7, this comment falls below the threshold of 10 required for inclusion under the current filtering rules. |
WalkthroughThis PR enhances integration tests and type mappings for MySQL and PostgreSQL to support the NUMERIC type in Spanner. It upgrades the emulator to version 1.1.1, adds numeric support flags, and modifies tests to ensure accurate data type conversions. README files are updated to clarify type mapping differences, improving compatibility and accuracy. Changes
Entelligence.ai can learn from your feedback. Simply add 👍 / 👎 emojis to teach it your preferences. More shortcuts belowEmoji Descriptions:
Interact with the Bot:
|
|
LGTM 👍 |
| var stringVal string | ||
| iter := client.Single().Read(ctx, "test2", spanner.Key{1}, []string{"a", "b", "c", "d", "e", "f", "g"}) | ||
| defer iter.Stop() | ||
| for { |
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.
Suggestion: Add a check for the iterator.Done condition to gracefully exit the loop when no more rows are available. [possible bug]
Pull Request Feedback 🔍
|
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.
PR Summary
This pull request reverts recent changes and restores the original numeric conversion behavior so that both MySQL and PostgreSQL DECIMAL/NUMERIC types map to Spanner's NUMERIC type, ensuring precision is preserved.
- mysql/README.md: Updated documentation now accurately reflects that DECIMAL/NUMERIC convert to NUMERIC (29/9) instead of FLOAT64.
- .github/workflows/integration-tests-against-emulator.yaml: Emulator updated to 1.1.1 with the "-enable_numeric_type" flag to support numeric types.
- mysql/data.go: Reintroduced convNumeric handling via big.Rat with spanner.NumericString for proper conversion.
- MySQL and PostgreSQL tests (e.g. infoschema_test.go, mysqldump_test.go, toddl_test.go, pgdump_test.go): Updated expectations to validate NUMERIC mapping and precision preservation.
- Documentation updates for PostgreSQL (postgres/README.md): Revised numeric type mapping explanations to match the restored conversion logic.
16 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile
| # needed because numeric support isn't on by default in emulator:1.1.1 | ||
| # note: future versions will default enable this and deprecate the flag | ||
| # TODO: upgrade emuator version, confirm enable_numeric_type is no longer needed and delete it | ||
| args: ["-enable_numeric_type"] |
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.
syntax: Fix typo in the TODO comment on line 26: change 'emuator' to 'emulator'.
| # needed because numeric support isn't on by default in emulator:1.1.1 | |
| # note: future versions will default enable this and deprecate the flag | |
| # TODO: upgrade emuator version, confirm enable_numeric_type is no longer needed and delete it | |
| args: ["-enable_numeric_type"] | |
| # needed because numeric support isn't on by default in emulator:1.1.1 | |
| # note: future versions will default enable this and deprecate the flag | |
| # TODO: upgrade emulator version, confirm enable_numeric_type is no longer needed and delete it | |
| args: ["-enable_numeric_type"] |
User description
Fixes #<issue_number_goes_here>
CodeAnt-AI Description
This PR enhances the handling of the NUMERIC data type by mapping it directly to Spanner's NUMERIC type, improving precision and compatibility. It updates tests, documentation, and configuration to support these changes, ensuring comprehensive coverage and clarity.
Changes walkthrough
8 files
integration_test.go
Enhance integration tests to support NUMERIC typeintegration_test.go
checkCoreTypesfunction to include more data types.infoschema_test.go
Update MySQL info schema tests for NUMERIC typemysql/infoschema_test.go
mysqldump_test.go
Adjust MySQL dump tests for NUMERIC type supportmysql/mysqldump_test.go
toddl_test.go
Verify NUMERIC type mapping in MySQL to Spanner conversionmysql/toddl_test.go
infoschema_test.go
Update PostgreSQL info schema tests for NUMERIC typepostgres/infoschema_test.go
pgdump_test.go
Adjust PostgreSQL dump tests for NUMERIC type supportpostgres/pgdump_test.go
report_test.go
Update report tests for NUMERIC type supportpostgres/report_test.go
toddl_test.go
Verify NUMERIC type mapping in PostgreSQL to Spanner conversionpostgres/toddl_test.go
4 files
data.go
Add conversion support for NUMERIC type in MySQLmysql/data.go
convNumericfunction to handle NUMERIC type conversion.convScalarto include NUMERIC type.toddl.go
Map MySQL NUMERIC type to Spanner NUMERICmysql/toddl.go
data.go
Add NUMERIC type conversion support for PostgreSQLpostgres/data.go
convNumericfunction for NUMERIC type conversion.convScalarto handle NUMERIC type.toddl.go
Map PostgreSQL NUMERIC type to Spanner NUMERICpostgres/toddl.go
1 files
integration-tests-against-emulator.yaml
Update GitHub workflow for NUMERIC type support in emulator.github/workflows/integration-tests-against-emulator.yaml
2 files
README.md
Update MySQL README for NUMERIC type supportmysql/README.md
README.md
Update PostgreSQL README for NUMERIC type supportpostgres/README.md
💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
EntelligenceAI PR Summary
Purpose
Enhance integration tests and type mappings for improved compatibility with Spanner's NUMERIC type.
Changes
Enhancement
Documentation
Test
Impact
These changes enhance the reliability and accuracy of data type handling in applications using Spanner, leading to a better user experience and reduced potential for data-related issues.