-
Notifications
You must be signed in to change notification settings - Fork 76
feat: Transform strings with respect to property schema when conforming properties #2997
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: main
Are you sure you want to change the base?
feat: Transform strings with respect to property schema when conforming properties #2997
Conversation
Reviewer's Guide by SourceryThis pull request adds support for automatically transforming string values to the data type specified by a property's JSON schema during property conformance. This is achieved by introducing a new helper function that handles the conversion logic for various target types (boolean, integer, number, array, object) and integrating it into the existing property conformance function. Extensive test cases are added to cover different string inputs and target types. No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2997 +/- ##
=========================================
+ Coverage 0 91.70% +91.70%
=========================================
Files 0 62 +62
Lines 0 5330 +5330
Branches 0 690 +690
=========================================
+ Hits 0 4888 +4888
- Misses 0 311 +311
- Partials 0 131 +131 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
CodSpeed Performance ReportMerging #2997 will not alter performanceComparing Summary
|
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.
Hey @ReubenFrankel - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider refactoring the
_transform_string_property
function to delegate type-specific transformations to smaller helper functions, improving clarity and maintainability. - Define the expected behavior when a non-empty string cannot be successfully transformed into the target schema type (e.g., invalid JSON).
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 3 issues found
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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.
Thanks @ReubenFrankel!
My main concern with this would've been performance, but the benchmark seems unaffected? I could be missing something though.
It looks like it was slightly affected: https://codspeed.io/meltano/sdk/branches/ReubenFrankel%3Afeat%2Fconform-primitive-interpret-string?uri=tests%2Fcore%2Ftest_typing.py%3A%3Atest_bench_conform_record_data_types Possibly due to this new check, which will check the property schema type for every sdk/singer_sdk/helpers/_typing.py Line 546 in 6c0f474
|
return None | ||
return elem | ||
return elem if math.isfinite(elem) else None | ||
if isinstance(elem, str) and not is_string_type(property_schema): |
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.
(commenting here to start a thread)
It looks like it was slightly affected: https://codspeed.io/meltano/sdk/branches/ReubenFrankel%3Afeat%2Fconform-primitive-interpret-string?uri=tests%2Fcore%2Ftest_typing.py%3A%3Atest_bench_conform_record_data_types
Possibly due to this new check, which will check the property schema type for every
str
value:sdk/singer_sdk/helpers/_typing.py
Line 546 in 6c0f474
if isinstance(elem, str) and not is_string_type(property_schema):
Do you think there's a way we could prevent this regression? Perhaps caching a mapping of property > is_.
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.
Yes, I was thinking about some kind of type caching also. We did something like that here, or were you imagining it to be per-property?
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.
Yeah, something like or like the selection mask we use internally here.
e86ce26
to
e2aac19
Compare
Adds support for transforming string values to their defined property schema types.
Related discussion: #2994
Closes #3014
Summary by Sourcery
Add automatic type conversion for string properties based on their schema during data conforming.
New Features:
Tests: