-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Support new agate Integer data_type in adapter code #9004
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #9004 +/- ##
==========================================
- Coverage 86.51% 86.50% -0.02%
==========================================
Files 179 179
Lines 26508 26526 +18
==========================================
+ Hits 22934 22945 +11
- Misses 3574 3581 +7
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
@@ -0,0 +1,6 @@ | |||
kind: Fixes | |||
body: Fix exception running empty seed file |
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.
The PR and this changelog indicate we're solving multiple issues, can we add a second changelog to cover new data_type?
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.
This doesn't solve multiple issues, it's all the same issue. The problem with the empty seed file was happening because of the lack of support for the new Integer data type.
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.
Oh, you mean additional info in the existing changelog? Sure.
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.
yup! not questioning the PR just want to make sure our changelogs cover everything that's being changed
do we need to beef up our tests around dbt-show or docs to catch this? https://github.com/dbt-labs/dbt-core/blob/main/tests/functional/show/test_show.py#L104C3-L104C3 |
I think the "show" tests are adequate. The problem was that the show command was fixed by changing infrastructure that also affected the code used to create an empty table for an empty seed. I added a new seed test, but probably that test should be moved to the adapter zone seed tests. |
I want to consider different scenarios with how adapter maintainers may have implemented this, and see how this solution would potentially affect them. (I'm basically processing out loud here.)
@gshank / @colin-rogers-dbt, are there any other scenarios that you can think of? And in particular, are we comfortable with scenario 4 above, where we will break adapters which have not tried to implement something like this in the past and which do not support "integer" as a valid type? |
The Integer agate type is already out in 1.7.0, so this fixes a regression. Right now, as far as I know, the main fallout from that is that empty seeds generate incorrect SQL (they have None as a type instead of "integer"). It's possible there are other edge cases that we just haven't encountered yet. So integer types were not actually "flowing through", they were failing. I did put "convert_integer_type" into both base/impl.py and sql/impl.py, to cover as much ground as possible. I tried reusing the existing "convert_number_type" and having Integer inherit from Number, but there is code buried in the middle of agate that expects an actual float, not an integer, so that was a dead end. |
I'll focus again on scenario 4 above, since I think that's the only problematic scenario. Given the following observations:
I think I'm comfortable with the approach. To confirm, are the observations above valid? |
@mikealfare I believe so, I'm comfortable with this approach |
Yes, those observations are valid. Although it's not just that combining the integer and number types makes the code complicated, with the current method of calling MaxPrecision it can't be gotten to work... We'd have to update every "convert_number_type" method to do ... something else. Thanks for all of the attention! |
The backport to
To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-1.6.latest 1.6.latest
# Navigate to the new working tree
cd .worktrees/backport-1.6.latest
# Create a new branch
git switch --create backport-9004-to-1.6.latest
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 46b9a1d621400be6204baed2680033ac0d5f1045
# Push it to GitHub
git push --set-upstream origin backport-9004-to-1.6.latest
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-1.6.latest Then, create a pull request where the |
Add test for empty seed file with only headers (cherry picked from commit 46b9a1d)
resolves #8895
Problem
In #8153 we added an agate Integer data type, but did not include conversion methods that are used when constructing a csv table.
Solution
Add Integer type to 'convert_agate_type' and add new 'convert_integer_type' method.
Checklist