-
Notifications
You must be signed in to change notification settings - Fork 20
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
bugfix/evancole-untitled-casting update #25
bugfix/evancole-untitled-casting update #25
Conversation
@evancole-untitled would you be able to try the following package dependency and see if the issue still persists? packages:
- git: https://github.com/fivetran/dbt_quickbooks.git
revision: bugfix/evancole-untitled-casting
warn-unpinned: false |
@evancole-untitled I was able to finally get the package to compile! 🎉 I actually ended up needing to cast all of the intentional Would you be able to try the working branch of the modeling package and let me know if you still see the issue persist? packages:
- git: https://github.com/fivetran/dbt_quickbooks.git
revision: bugfix/evancole-untitled-casting
warn-unpinned: false |
It almost ran! I crashed out in the quickbooks package with this error 19:23:42 Completed with 1 error and 0 warnings: |
@evancole-untitled hmmm I wonder why it is passing on my side 🤔 If you look into the compiled output of the model with the error, are you able to see which field is causing the issue on your end? |
i suspect it's this block down at the very bottom:
final as (
select *
from general_ledger_balances
union all
select *
from retained_earnings
)
As for why it worked for you and not for me, my first guess would be that
you're using testing it using a grownup warehouse like Snowflake, which
will attempt to do typing and promotions during unions or joins on complex
queries, instead of Redshift, which will attempt to ridicule you :/
I tested this out with this code:
WITH
t1 AS (SELECT 'text' as c1)
SELECT c1 FROM t1
INNER JOIN t1 AS t2 USING (c1)
It runs just fine in Snowflake, but in Redshift I get the same "failed to
find conversion function from "unknown" to character varying" error.
…On Fri, Mar 25, 2022 at 4:09 PM Joe Markiewicz ***@***.***> wrote:
@evancole-untitled <https://github.com/evancole-untitled> hmmm I wonder
why it is passing on my side 🤔
If you look into the compiled output of the model with the error, are you
able to see which field is causing the issue on your end?
—
Reply to this email directly, view it on GitHub
<#25 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AVERHMFPO3D5SPZIA7CVU2TVBYMRHANCNFSM5RSAUUZA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Thanks @evancole-untitled! I wonder if the constant expressions created within the Also, would you be able to try and |
Sure! I can confirm that the error persists even with the --full-refresh
flag.
…On Mon, Mar 28, 2022 at 11:35 AM Joe Markiewicz ***@***.***> wrote:
Thanks @evancole-untitled <https://github.com/evancole-untitled>!
I wonder if the constant expressions created within the
int_quickbooks__retained_earnings model is causing this issue within
redshift. I just pushed some changes to the branch the add explicit casting
to hopefully resolve this "unknown" issue. Would you mind trying the branch
again and let me know if you still see the issue?
Also, would you be able to try and dbt run --full-refresh just as a check
to ensure there isn't an error from a previous run that is causing the
hangup.
—
Reply to this email directly, view it on GitHub
<#25 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AVERHMGQ2YMPG6CEWKT5THDVCHGUHANCNFSM5RSAUUZA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Darn 😞 I am a bit stuck since I have tested this on my own Redshift instance and am unable to replicate the error. I think the next best step if the latest changes didn't resolve the issue would be to setup some time during out office hours to dig into this together live. |
It was found that the latest update to this branch works for @evancole-untitled. This is ready for review! |
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.
Heya @fivetran-joemarkiewicz, great PR, that was quite a lot of changes indeed! I will need your help getting access the the QB data to test the models, but I double checked all the macros and it looks like you've got all the ID columns specified as datatype string.
I left a couple of comments regarding the final models -- I'd like to understand the logic that you followed to cast certain ID columns and not all of them so that I can properly review that portion of the PR.
Let me know if you have any questions!
@fivetran-joemarkiewicz dbt test ran great, however dbt test had one failure (that is also the failure that is encountered for the modeling pkg due to source test failure) -- is this expected?
|
@fivetran-sheringuyen I just looked at the source data and this seems to be a case where the test failed successfully. Looking at the source data there are in fact This will be something we want to investigate within the source data, but this is expected behavior for the package. |
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.
lgtm!
Are you a current Fivetran customer?
Fivetran created PR, but takes the bulk of the work from PR #24 that was opened by @evancole-untitled.
What change(s) does this PR introduce?
Did you update the CHANGELOG?
Does this PR introduce a breaking change?
Since this is such a large update that changes the datatypes of multiple fields that could be used in downstream join conditions, this should be a breaking change.
Did you update the dbt_project.yml files with the version upgrade (please leverage standard semantic versioning)? (In both your main project and integration_tests)
Is this PR in response to a previously created Bug or Feature Request
How did you test the PR changes?
Select which warehouse(s) were used to test the PR
Provide an emoji that best describes your current mood
💸
Feedback
We are so excited you decided to contribute to the Fivetran community dbt package! We continue to work to improve the packages and would greatly appreciate your feedback on our existing dbt packages or what you'd like to see next.