impl(bigquery): add FromSql trait to read/convert columns from a Row#5924
impl(bigquery): add FromSql trait to read/convert columns from a Row#5924alvarowolfx wants to merge 1 commit into
Conversation
product-auto-label
Bot
added
the
api: bigquery
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the FromSql trait in src/bigquery/src/query/from_sql.rs to convert BigQuery internal wkt::Value types into Rust types such as String, i64, f64, bool, and Option<T>, along with comprehensive unit tests. The feedback suggests replacing .unwrap() with .expect() in a test case attribute to adhere to the repository's style guide regarding panic handling in tests.
| FromSql::from_sql(value).map_err(TestConvertError::from) | ||
| } | ||
|
|
||
| #[test_case(wkt::Value::Number(serde_json::Number::from_f64(123.45).unwrap()) => Ok(123.45) ; "f64 from number")] |
There was a problem hiding this comment.
According to the repository style guide, .unwrap() should be avoided in tests unless there is no other option, and .expect() should be preferred. Since this is inside a #[test_case] attribute where the ? operator cannot be used, please use .expect() with a descriptive message instead of .unwrap().
| #[test_case(wkt::Value::Number(serde_json::Number::from_f64(123.45).unwrap()) => Ok(123.45) ; "f64 from number")] | |
| #[test_case(wkt::Value::Number(serde_json::Number::from_f64(123.45).expect("valid f64")) => Ok(123.45) ; "f64 from number")] |
References
- Panics: unwrap() and expect() should typically be avoided in production code and examples (use ? or handle errors). ... In tests, prefer ?. If that is not possible use .expect(). Only use .unwrap() as a last resort. (link)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5924 +/- ##
=======================================
Coverage 97.90% 97.90%
=======================================
Files 234 235 +1
Lines 59940 60021 +81
=======================================
+ Hits 58683 58764 +81
Misses 1257 1257 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| // Test-only representation of `ConvertError` that implements `PartialEq`. | ||
| // This allows testing error outcomes using `test_case` assertions without | ||
| // implementing `PartialEq` on the production `ConvertError`. | ||
| #[derive(Debug, PartialEq)] | ||
| enum TestConvertError { | ||
| NotNull, | ||
| TypeMismatch(&'static str), | ||
| Convert(String), | ||
| } | ||
|
|
||
| impl From<ConvertError> for TestConvertError { | ||
| fn from(err: ConvertError) -> Self { | ||
| match err { | ||
| ConvertError::NotNull => Self::NotNull, | ||
| ConvertError::TypeMismatch { expected, .. } => Self::TypeMismatch(expected), | ||
| ConvertError::Convert(e) => Self::Convert(e.to_string()), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Can you try something like:
#[derive(...)]
#[cfg_attr(test, derive(PartialEq))]
enum ConvertError { ... }| wkt::Value::Number(n) => n | ||
| .as_i64() | ||
| .ok_or_else(|| ConvertError::Convert("number is not a valid i64".into())), | ||
| wkt::Value::String(s) => s | ||
| .parse::<i64>() | ||
| .map_err(|e| ConvertError::Convert(Box::new(e))), |
There was a problem hiding this comment.
comment: Ah, we try to accept both. That's good.
2 participants
Towards #5844