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

[MYSQL] convert explicitely smallint and tinyint #35897

Merged
merged 3 commits into from Jun 21, 2021
Merged

[MYSQL] convert explicitely smallint and tinyint #35897

merged 3 commits into from Jun 21, 2021

Conversation

ying-jeanne
Copy link
Contributor

@ying-jeanne ying-jeanne commented Jun 17, 2021

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #35876

Special notes for your reviewer:

seems when smallint or tinyint is nullable or not would impact its scantype. when they are nullable, scantype becomes nullableint64, for which we would generate the default string converter.

@ying-jeanne ying-jeanne requested a review from a team as a code owner June 17, 2021 16:59
@ying-jeanne ying-jeanne requested review from wbrowne and marefr and removed request for a team June 17, 2021 16:59
Copy link
Member

@marefr marefr left a comment

Choose a reason for hiding this comment

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

Looks good I think. Added some comments and a suggestion to get the CI to not run mysql_tests during the integration tests when database is sqlite.

To avoid this kind of issue, maybe it makes more sense to think about enhance convert value to float64 to support also string type?

Maybe. Is the type really string, isn't it string array? If you would like to evaluate that, please push changes to this PR or open an alternative PR so that we can understand what it actually means in terms of code and changes.

pkg/tsdb/mysql/mysql_test.go Outdated Show resolved Hide resolved
pkg/tsdb/mysql/mysql_test.go Show resolved Hide resolved
@ying-jeanne
Copy link
Contributor Author

Looks good I think. Added some comments and a suggestion to get the CI to not run mysql_tests during the integration tests when database is sqlite.

To avoid this kind of issue, maybe it makes more sense to think about enhance convert value to float64 to support also string type?

Maybe. Is the type really string, isn't it string array? If you would like to evaluate that, please push changes to this PR or open an alternative PR so that we can understand what it actually means in terms of code and changes.

i think this fix is urgent, let's fix the issue and i would create a ticket for the improvement, we can discuss that next week.

@kminehart
Copy link
Contributor

kminehart commented Jun 21, 2021

Are these specifically for NULLABLE smallint / tinyint? will this also work for non-nullable columns with these types?

@ying-jeanne
Copy link
Contributor Author

Are these specifically for NULLABLE smallint / tinyint? will this also work for non-nullable columns with these types?

they are working for both nullable and no nullable.

@ying-jeanne ying-jeanne added the old backport v8.0.x Mark PR for automatic backport to v8.0.x label Jun 21, 2021
@ying-jeanne ying-jeanne merged commit 73fdd9d into main Jun 21, 2021
@ying-jeanne ying-jeanne deleted the mysqlbug branch June 21, 2021 16:10
grafanabot pushed a commit that referenced this pull request Jun 21, 2021
* [MYSQL] convert explicitely smallint and tinyint

* clean up code

* fix comments

(cherry picked from commit 73fdd9d)
ying-jeanne added a commit that referenced this pull request Jun 21, 2021
* [MYSQL] convert explicitely smallint and tinyint

* clean up code

* fix comments

(cherry picked from commit 73fdd9d)

Co-authored-by: ying-jeanne <74549700+ying-jeanne@users.noreply.github.com>
@ying-jeanne ying-jeanne added this to the 8.0.4 milestone Jun 22, 2021
bryanuribe pushed a commit to open-o11y/grafana that referenced this pull request Aug 6, 2021
* [MYSQL] convert explicitely smallint and tinyint

* clean up code

* fix comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/backend datasource/MSSQL Microsoft SQL Server Data Source datasource/MySQL old backport v8.0.x Mark PR for automatic backport to v8.0.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"convert value to float failed: metricIndex 1 type can't be converted to float" after upgrade to 8.0.2
4 participants