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

Zeroex performance #6203

Merged
merged 15 commits into from
Jun 21, 2024
Merged

Zeroex performance #6203

merged 15 commits into from
Jun 21, 2024

Conversation

aalan3
Copy link
Contributor

@aalan3 aalan3 commented Jun 19, 2024

This PR hopes to improve performance on some zeroex models.

For ethereum I added an incremental filter on prices where it was missing.

For BNB I broke out the part of the query that's doing a full scan on bnb logs into its own model. We should then be able to run that model incrementally and joining on it should be much lighter as well.

@aalan3
Copy link
Contributor Author

aalan3 commented Jun 19, 2024

@RantumBits could you please review this to make sure I'm not messing up the logic on BNB here?

schema = 'zeroex_bnb',
alias = 'fills_uni_v2_pair_creation',
materialized='incremental',
unique_key = ['pair', 'makerToken', 'takerToken'],
Copy link
Member

Choose a reason for hiding this comment

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

not sure all three of these should be unique keys, otherwise only block_time is left to be updated. likely just pair?

Copy link
Member

Choose a reason for hiding this comment

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

or potentially this could be append-only or something rather than merge? if so, we'd have to left join {{ this }} to make sure not already processed. merge may still be easier with correct keys

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding of how this works is that you get the latest events for each pair, the old query implied there could be duplicates so we'd want to replace those values with the latest. This could be wrong however.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

block time might actually not even be needed here

Copy link
Member

Choose a reason for hiding this comment

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

i think pair should be the only unique key, if we partition by that field in the rn and then the maker/taker tokens can change?

@jeff-dude
Copy link
Member

fyi on this one @Hosuke

@jeff-dude jeff-dude self-assigned this Jun 19, 2024
@jeff-dude jeff-dude added the in review Assignee is currently reviewing the PR label Jun 19, 2024
@Pluies
Copy link

Pluies commented Jun 19, 2024

(Rebasing this to trigger a new CI run and hopefully reproduce issues that happened before for investigation 👍 )

@aalan3
Copy link
Contributor Author

aalan3 commented Jun 20, 2024

Overlaps with #6192

@RantumBits
Copy link
Contributor

RantumBits commented Jun 21, 2024

@RantumBits could you please review this to make sure I'm not messing up the logic on BNB here?

@aalan3 looks good ser, thanks for your help with this 🫡

@aalan3
Copy link
Contributor Author

aalan3 commented Jun 21, 2024

@RantumBits thanks for reviewing this!

@aalan3 aalan3 merged commit 6b2d412 into main Jun 21, 2024
2 of 3 checks passed
@aalan3 aalan3 deleted the zeroex-performance branch June 21, 2024 05:39
@github-actions github-actions bot locked and limited conversation to collaborators Jun 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
in review Assignee is currently reviewing the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants