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

Use inner joins instead of left joins on transactions #1465

Merged
merged 4 commits into from
Aug 31, 2022

Conversation

aalan3
Copy link
Contributor

@aalan3 aalan3 commented Aug 30, 2022

This fixes a bug that I introduced with the performance fixes.

On incremental runs we still select all of the decoded data in some cases. When we then left join instead of inner join the data that is more than 1 week old will have empty transaction data. To fix this we should use inner join. This should not be a problem because all decoded data has a transaction.

In the cases where we already filter decoded data on where evt_block_time >= (select max(block_time) from {{ this }}) for incremental runs, this should not have caused any issue. But for opensea which doesn't have that for example it did.

If we still have to do a left join like we seemingly do on opensea fees, we can add the filter in the where statement as well to make sure we don't get any extra data.

Looksrare doesn't have this problem because it already uses an inner join.

I've verified the following:

  • Opensea inner join vs left join count is the same
  • Opensea join on fees needs to be left join so added a where statement as well
  • Opensea no null values for block_time
  • Opensea incremental update contains no null values for block_time like before
  • x2y2 inner join vs left join count is the same
  • seaport inner join vs left join count is the same
  • sudoswap inner join vs left join count is the same for join on transactions
  • sudoswap inner join vs left join count is the same for join on traces

@aalan3 aalan3 marked this pull request as ready for review August 30, 2022 10:04
@jeff-dude
Copy link
Member

fyi @springzh on comments in description here related to opensea block_time values

@soispoke
Copy link
Contributor

soispoke commented Aug 31, 2022

@aalan3 @jeff-dude I've run more tests on the opensea spell to make sure underlying data wasn't modified for some reason, and everything looks good !
I've also added an incremental filter on wyvern_call_data, and this seems to lead to a performance increase in update time (now only takes 55 seconds compared to 200 seconds previously on a dedicated cluster). We'll have to monitor on prod but looks promising !

@soispoke soispoke merged commit efedfb1 into main Aug 31, 2022
@aalan3 aalan3 deleted the fix-nft-trades-joins branch August 31, 2022 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants