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

Twilio numeric fields cast as strings #7

Closed
raphaelvarieras opened this issue May 13, 2024 · 8 comments · Fixed by #8
Closed

Twilio numeric fields cast as strings #7

raphaelvarieras opened this issue May 13, 2024 · 8 comments · Fixed by #8
Assignees

Comments

@raphaelvarieras
Copy link
Contributor

          @raphaelvarieras that is a good callout, but would you mind creating a new issue for that? I'd prefer to tackle in it another release to ensure we have ample time to figure out each field we'd want to (and logically should) cast as an integer, as I see that other tables have quite a few numeric fields cast as strings (ex: `duration` and `price` in `CALL` and `count` and `usage` in `USAGE_RECORD`)

Originally posted by @fivetran-jamie in #6 (comment)

@fivetran-jamie
Copy link
Contributor

thank you 🙌

@fivetran-jamie
Copy link
Contributor

so, we need to investigate the following tables' fields, which are coming in from the connector as varchars/strings but sound (and look) like they should be numeric types:

  • message:
    • num_segments
    • num_media
    • price
  • call:
    • duration
    • price
    • queue_time
  • usage_record:
    • count
    • price
    • usage

i'll talk to the data PMs to understand why this might be the case from the connector-side of things, but otherwise this would be straightforward to implement ion the package. in the final CTE of each staging model we'd wanna cast each of these fields as a {{ dbt.type_int() }} or {{ dbt.type_numeric() }} or float or whatever is most appropriate for the field

@raphaelvarieras
Copy link
Contributor Author

I already got some answers from Fivetran support. Apparently this is because the specifications provided by Twilio for their API is that these are provided as strings. So it seems maybe there are circumstances under which the API might return non-numeric answers?? I'm just not sure that is enough of a reason to compromise the ease with which to perform analysis using these models. I imagine many will need to add a casting operator in their project if it's not part of this package.

@fivetran-jamie
Copy link
Contributor

Yeah I do see that Twiliio lists these fields as strings in the API Docs, but I completely agree with you that the fields are really only valuable as actual numerics. Going to investigate some more and will report back 🫡

@fivetran-jamie
Copy link
Contributor

@raphaelvarieras We're currently trying to get in contact with Twilio to get some clarification on these fields, but it could be helpful for you to open a case with Twilio directly as well, as they might prioritize getting to customer questions over partner ones.

@fivetran-jamie
Copy link
Contributor

So finally got some answers (somewhat) from Twilio:

GENERAL ADVICE ON TREATING STRING FIELDS AS NUMBERS
When dealing with API responses, it's common to encounter fields that are represented as strings but contain numerical data. This often happens for several reasons, including but not limited to:

Ensuring precision for very large numbers that might exceed the limits of numerical types in some programming languages.

Maintaining compatibility across different systems where the interpretation of numerical types might vary.

With this, I think the package can safely apply the below updates in the relevant staging models:

  1. Scrub out non-numeric characters (it doesn't sound like there will typically be any but just in case)
  2. Cast each of the fields as {{ dbt.type_numeric() }}

@raphaelvarieras We'll plan ot take this on next sprint, which starts 6/13, but if you have availability to open a PR we will happily review and merge before then!

@fivetran-reneeli fivetran-reneeli mentioned this issue Jul 1, 2024
7 tasks
@fivetran-reneeli
Copy link
Contributor

Thanks @fivetran-jamie and @raphaelvarieras for the context and scoping, will start working on an update to the package

@fivetran-reneeli fivetran-reneeli self-assigned this Jul 1, 2024
@fivetran-reneeli fivetran-reneeli linked a pull request Jul 1, 2024 that will close this issue
7 tasks
@fivetran-reneeli
Copy link
Contributor

Thanks @raphaelvarieras! This has been addressed in our latest release.

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 a pull request may close this issue.

3 participants