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

Dex trades beta dunesql #4107

Merged
merged 22 commits into from
Aug 22, 2023
Merged

Dex trades beta dunesql #4107

merged 22 commits into from
Aug 22, 2023

Conversation

aalan3
Copy link
Contributor

@aalan3 aalan3 commented Aug 17, 2023

This is to test dex trades under the alias trades_migration_beta to make sure that everything builds correctly and to check the data.

@jeff-dude
Copy link
Member

nice, you'll just have to comment out refs which aren't migrated yet (and likely the jinja comment, based on bug we found)

@aalan3 aalan3 force-pushed the dex-trades-beta-dunesql branch 2 times, most recently from 4678934 to cbd541c Compare August 17, 2023 13:58
@aalan3 aalan3 requested a review from jeff-dude August 18, 2023 13:44
{{ config(
tags=['dunesql'],
alias = alias('trades_migration_beta'),
partition_by = ['block_date'],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

block_month here to match pattern of upstream spells?

Copy link
Member

@jeff-dude jeff-dude left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good to go outside of partition question

@aalan3
Copy link
Contributor Author

aalan3 commented Aug 18, 2023

Good catch changed to block_month now.

Copy link
Contributor

@antonio-mendes antonio-mendes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beautiful, looks good to me too

@aalan3
Copy link
Contributor Author

aalan3 commented Aug 18, 2023

This can't be merged until block_time exists on all models that are being referenced. I will create some PRs to fix this.

@aalan3 aalan3 force-pushed the dex-trades-beta-dunesql branch from 527efb7 to c32ecb8 Compare August 21, 2023 13:31
@aalan3 aalan3 mentioned this pull request Aug 21, 2023
@aalan3
Copy link
Contributor Author

aalan3 commented Aug 21, 2023

I figured out the issue! The incremental part of statement was incorrectly formated due to the loop, so the last model didn't have the time filter (fixed in b29627f) . This was not a problem before but since I added the incremental_predicate, the last model was inserting the entire model which caused duplicates because the destination has a time filter now.

@aalan3 aalan3 merged commit e45b53e into main Aug 22, 2023
@aalan3 aalan3 deleted the dex-trades-beta-dunesql branch August 22, 2023 08:05
@github-actions github-actions bot locked and limited conversation to collaborators Aug 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants