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

fix: Convert integral values to String before converting to Value #1056

Merged
merged 5 commits into from
Apr 30, 2021

Conversation

ethowitz
Copy link
Contributor

Description

In the ToSpannerValue implementations for i32 and u32, we were using Value::set_number_value() to construct a Value from an integer. In doing so, we coerced the integers into floats, which is what created the errors on Spanner. To resolve this, I convert the integral values to strings prior to creating a Value.

Testing

The issue can be reproduced on the main branch by adding a new bookmark and syncing (your local server should throw the same error that appeared in Sentry. Switching to this feature branch, adding a new bookmark, (recompiling,) and syncing should result in a successful sync, with no errors reported.

Issue(s)

Closes #1055

@ethowitz ethowitz requested a review from a team April 28, 2021 19:53
jrconlin
jrconlin previously approved these changes Apr 29, 2021
"batch_id" => params.batch.id.clone(),
"timestamp" => as_rfc3339.clone(),
};
sqlparam_types.insert("timestamp".to_owned(), as_type(TypeCode::TIMESTAMP));
Copy link
Member

Choose a reason for hiding this comment

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

NOT THAT WE NEED TO DO THIS NOW

But we really could take advantage of they SyncTimestamp type better for stuff like this.
(heh, Good first bug material.)

@ethowitz
Copy link
Contributor Author

ethowitz commented Apr 29, 2021

@jrconlin I took a stab at having SQL parameter types automatically generated by the same macro that builds the param hashmaps. In short, I updated the params! macro to return a tuple consisting of (params, param_types), which strongly suggests to the caller that the types should be used. I have a few concerns, and I'd be curious to hear your thoughts about them:

  • There are certain params whose automatically generated types are incorrect. For example, we store timestamps in rust as strings (the macro will thus return TypeCode::STRING as the type), but Spanner stores timestamps as TypeCode::TIMESTAMPs (a possible solution to this would be to change how we represent timestamps in memory). If a developer forgets to update the timestamp type before passing sqlparam_types to the queryer, a database error would occur at runtime.
  • I'm not sure how to comprehensively test these changes 😬 I made sure that I could perform a successful sync with spanner after adding a new bookmark, but I don't know how to comprehensively test each of the database transactions that use the new param types functionality. Do you have any ideas?

@jrconlin
Copy link
Member

As for the comments:

  • Yeah, the problem is that we break "type" by converting values to strings before we pass them to params! so there's no carry over type information. I noted it in the comment, but a solution to that would be "not convert these to strings" and use From<foo> to String impl's with associated types for the odder ones. We could probably also create SyncTimeStamp consts for PRETOUCH_TS. Timestamps seem to be the most problematic for these.
  • I suppose the testing strategy here is to test the function of what you're doing, not necessarily how it's being used? This change really introduces a new behavior for params! which takes a HashMap and returns two separate HashMaps. What you're testing for is that values and types are properly recognized and converted. (e.g. values are converted to Values with correct .kind, and param_types are proper Type with a proper .code. I'd probably construct a "one of each" sort of test HashMap and validate that everything got converted correctly.

@ethowitz
Copy link
Contributor Author

I ran the spanner tests locally and they all passed 🙌🏻

@ethowitz ethowitz merged commit 21da763 into master Apr 30, 2021
@ethowitz ethowitz deleted the fix/1055-spanner-int64-error branch April 30, 2021 16:09
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.

Invalid value for column collection_id in table bsos: Expected INT64.
2 participants