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

Add query nft trades labels #1513

Merged
merged 6 commits into from
Sep 8, 2022
Merged

Add query nft trades labels #1513

merged 6 commits into from
Sep 8, 2022

Conversation

soispoke
Copy link
Contributor

@soispoke soispoke commented Sep 6, 2022

Brief comments on the purpose of your changes:
This PR aims to give an example of adding query labels to the spellbook repo

For Dune Engine V2
I've checked that:
General checks:

  • I tested the query on dune.com after compiling the model with dbt compile (compiled queries are written to the target directory)
  • I used "refs" to reference other models in this repo and "sources" to reference raw or decoded tables
  • if adding a new model, I added a test
  • the filename is unique and ends with .sql
  • each sql file is a select statement and has only one view, table or function defined
  • column names are lowercase_snake_cased
  • if adding a new model, I edited the dbt project YAML file with new directory path for both models and seeds (if applicable)
  • if adding a new model, I edited the alter table macro to display new database object (table or view) in UI explorer
  • if adding a new materialized table, I edited the optimize table macro

Join logic:

  • if joining to base table (i.e. ethereum transactions or traces), I looked to make it an inner join if possible

Incremental logic:

  • I used is_incremental & not is_incremental jinja block filters on both base tables and decoded tables
    • where block_time >= date_trunc("day", now() - interval '1 week')
  • if joining to base table (i.e. ethereum transactions or traces), I applied join condition where block_time >= date_trunc("day", now() - interval '1 week')
  • if joining to prices view, I applied join condition where minute >= date_trunc("day", now() - interval '1 week')

@soispoke soispoke requested review from aalan3 and masquot September 6, 2022 06:31
{{ config(
alias = 'all',
materialized = 'table',
file_format = 'delta')
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to also add an alter table statement for this. Could you try adding it to post_hook here, would be interesting to see if it works. Because then we can keep those things in the files and not forget.

Copy link
Contributor Author

@soispoke soispoke Sep 7, 2022

Choose a reason for hiding this comment

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

Yes, I initially didn't want to expose the labels yet, until we have added a couple more queries/static labels. I also don't really want to add alter table statements as post hook as I've done this before and it ended up being extremely long and messy in the dbt_project.yml file. I think the most promising avenue is to alter table properties in the models directly (databricks/dbt-databricks#33), but we have to wait until the databricks adapter is supported by dbt cloud.

In any case, maybe we could open another PR/discussion around this ?

'nft' AS category,
'soispoke' AS contributor,
'query' AS source,
timestamp('2022-09-24') as created_at,
Copy link
Contributor

Choose a reason for hiding this comment

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

not that important but these are all in the future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh right thanks haha, this is fixed now 👍

- name: created_at
description: "When were labels created"
- name: updated_at
description: "When were labels updated for the last time ?"
Copy link
Contributor

Choose a reason for hiding this comment

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

drop ?, here and below

Copy link
Contributor Author

@soispoke soispoke Sep 7, 2022

Choose a reason for hiding this comment

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

sounds good, done @aalan3

@jeff-dude jeff-dude mentioned this pull request Sep 6, 2022
14 tasks
@soispoke soispoke requested a review from aalan3 September 7, 2022 04:26
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.

this builds fine, but a bit slow to query the end result in 'all' table, at least when trying to do select * on it. not a huge surprise as it's on large volume of nft trades data.

also curious how you proceed with the few PRs all for labels? merge one PR, then merge main into other branch(s) and increment from there, considering they all need to union into the 'all' table?

@soispoke
Copy link
Contributor Author

soispoke commented Sep 8, 2022

@jeff-dude thanks, yes exactly the goal was to give examples for static labels and query labels for it to be easier to review, but now I'll merge this, and then merge main into the bigger Labels WIP PR #1474. If you guys can review/approve that one when you have time it would be awesome

@soispoke soispoke merged commit 7289639 into main Sep 8, 2022
@jeff-dude jeff-dude deleted the Add-query-NFT-trades-labels branch September 8, 2022 14:34
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