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

make epochs table instead of incremental #6518

Merged
merged 1 commit into from
Aug 7, 2024
Merged

make epochs table instead of incremental #6518

merged 1 commit into from
Aug 7, 2024

Conversation

andrewhong5297
Copy link
Collaborator

Thank you for contributing to Spellbook 🪄

Update!

Please build spells in the proper subproject directory. For more information, please see the main readme, which also links to a GH discussion with the option to ask questions.

Contribution type

Please check the type of contribution this pull request is for:

  • New spell(s)
  • Adding to existing spell lineage
  • Bug fix

I'm making solana_utils.epoch a table to refresh fully each time versus incremental, because the incremental is messing with the row_number() to get first_block_epoch. because each time it runs, it will add a new first_block_epoch window function ordering which then messes up the query. Open to creative solutions here too if anyone thinks of one, too late in the night for me to find one haha

@andrewhong5297 andrewhong5297 changed the title make table instead of incremental make epochs table instead of incremental Aug 7, 2024
@andrewhong5297 andrewhong5297 added the ready-for-review this PR development is complete, please review label Aug 7, 2024
@andrewhong5297
Copy link
Collaborator Author

seems to be some solution to using incremental tables with window function columns but would make this sql maybe 100-200 lines longer lol. it's a cheap query so maybe we just do the full refresh each time. I'm sure we've run into this in the past with other models in here.

https://butterflymx.com/blog/dbt-incremental-models/

@0xRobin
Copy link
Collaborator

0xRobin commented Aug 7, 2024

@andrewhong5297 a creative solution here is to manually look back more then the standard incremental window. So that the first result in the incremental window still has all the necessary rows to completely fill it's window.

So if an epoch here is ~2 days, we need to look back 1d (incr window) + 2d (epoch duration) on blocks, let's do 4d just to be safe.
So on solana.blocks add:

{% if is_incremental() %}
        WHERE time > now() - interval '4' day
{% endif %}

And on the final select we still add:

{% if is_incremental() %}
WHERE {{incremental_predicate('block_time')}}
{% endif %}

So we only merge the results in the standard incremental window.

@andrewhong5297
Copy link
Collaborator Author

But then wouldn't it overwrite the past epoch it runs into? i.e it might get the current epoch right but then mess up the past epoch.

@andrewhong5297
Copy link
Collaborator Author

andrewhong5297 commented Aug 7, 2024

Like basically using a day filter will always end up in the middle of one epoch or another. but also we still need to add the slots in the middle of an epoch, and update them too.

@jeff-dude
Copy link
Member

lets merge for sake of time, but i need to revisit existing spells to find if we had a good solution to window functions / incremental. building an example one could be useful too.

@jeff-dude jeff-dude merged commit 093d926 into main Aug 7, 2024
2 checks passed
@jeff-dude jeff-dude deleted the fix_epoch branch August 7, 2024 15:07
@andrewhong5297
Copy link
Collaborator Author

agree, worth revisiting in the future as this is a simple table we can use as an example

@github-actions github-actions bot locked and limited conversation to collaborators Aug 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
ready-for-review this PR development is complete, please review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants