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

remove uniqueness test on hash for optimism transactions #1517

Merged
merged 3 commits into from
Sep 7, 2022

Conversation

soispoke
Copy link
Contributor

@soispoke soispoke commented Sep 6, 2022

Brief comments on the purpose of your changes:

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')

@chuxinh
Copy link
Contributor

chuxinh commented Sep 6, 2022

Thanks for this!
I think it'd be better instead of dropping unique_ids unique test, we should instead update the unique id to tx_hash and block number and trace_address instead. That also means the table needs a rebuild if necessary?

Copy link
Contributor Author

@soispoke soispoke left a comment

Choose a reason for hiding this comment

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

@chuxin we wouldn't need to rebuild the table, we could change the test to something similar to the commit I last pushed. However, the trace_address doesn't seem to be a column in the transactions table ? (so we can't use it as a unique id)

@chuxinh
Copy link
Contributor

chuxinh commented Sep 7, 2022

@chuxin we wouldn't need to rebuild the table, we could change the test to something similar to the commit I last pushed. However, the trace_address doesn't seem to be a column in the transactions table ? (so we can't use it as a unique id)

ohhh i got confused here. thought you were referring to transfer_optimism_eth. yea that looks good to me!

@chuxinh
Copy link
Contributor

chuxinh commented Sep 7, 2022

is this going to re-enable of incremental build for transfers_optimism.eth after merge?
it's been paused since Aug 27

@soispoke
Copy link
Contributor Author

soispoke commented Sep 7, 2022

@chuxinh ok great, will merge today then 👍

And unfortunately I think the pause on optimism is due to other factors not related to this particular issue

Idk if you saw comments/updates I made regarding your other PR here ? #1520

@chuxinh
Copy link
Contributor

chuxinh commented Sep 7, 2022

just wonder is there's any error or issue that's blocking the incremental build of transfers_optimism.eth?

@soispoke
Copy link
Contributor Author

soispoke commented Sep 7, 2022

@chuxinh Opened a PR here #1522 to improve the incremental filters, let's also wait for @aalan3 or @jeff-dude's inputs to make sure there aren't any other issues with that table

@jeff-dude
Copy link
Member

updated the unique test to remove tx_hash which doesn't exist on base table, results pass

@jeff-dude
Copy link
Member

@chuxinh Opened a PR here #1522 to improve the incremental filters, let's also wait for @aalan3 or @jeff-dude's inputs to make sure there aren't any other issues with that table

this PR looks good (and it's merged already), plus dbt shows that this model has been refreshing today 👍

@jeff-dude jeff-dude merged commit 4cdac10 into main Sep 7, 2022
@jeff-dude jeff-dude deleted the remove-test-on-transactions-hash-optimism branch September 7, 2022 20:51
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