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

Migrate cex_ethereum.flows to a sector spell with macros to easily add new chains #5521

Merged
merged 2 commits into from
Mar 11, 2024

Conversation

hildobby
Copy link
Collaborator

@hildobby hildobby commented Mar 6, 2024

Goal here is to create this macro so i can easily add cex_{chain}.flows for all other evm chains afterwards @jeff-dude

@hildobby hildobby added WIP work in progress ready-for-review this PR development is complete, please review and removed WIP work in progress labels Mar 6, 2024
@hildobby hildobby requested a review from jeff-dude March 11, 2024 12:30
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.

music to my ears!

one quick comment below, if you don't mind checking performance

FROM {{ ref('tokens_ethereum_transfers')}} t
INNER JOIN {{ ref('cex_ethereum_addresses')}} a ON a.address IN (t."from", t.to)
FROM {{transfers}} t
INNER JOIN {{addresses}} a ON a.address IN (t."from", t.to)
Copy link
Member

Choose a reason for hiding this comment

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

can you run a quick test on performance to see how runtime looks when you remove IN and do (a.address = t."from" or a.address = t.to)?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

okay, no real change. feel free to keep as-is or revert to your preference

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it's good as is, feel free to merge

@jeff-dude jeff-dude self-assigned this Mar 11, 2024
@jeff-dude jeff-dude added question Further information is requested in review Assignee is currently reviewing the PR and removed ready-for-review this PR development is complete, please review labels Mar 11, 2024
@jeff-dude jeff-dude removed the question Further information is requested label Mar 11, 2024
@jeff-dude jeff-dude removed the in review Assignee is currently reviewing the PR label Mar 11, 2024
@jeff-dude jeff-dude merged commit 92ad189 into duneanalytics:main Mar 11, 2024
3 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Mar 11, 2024
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.

2 participants