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

Rename chain_ids > chain_info_chain_ids hotfix #3432

Merged
merged 19 commits into from
Jun 13, 2023

Conversation

hildobby
Copy link
Collaborator

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 wanting to expose a model in the UI (Dune data explorer), I added a post-hook in the JINJA config to add metadata (blockchains, sector/project, name and contributor Dune usernames)

Pricing checks:

  • coin_id represents the ID of the coin on coinpaprika.com
  • all the coins are active on coinpaprika.com (please remove inactive ones)

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

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.

thank you ser, fixing dbt compile warnings

@jeff-dude
Copy link
Member

hmm, looks the the anti join isn't dunesql compatible. since these are views, they'd need to be run on dunesql syntax too. i wonder if these were built prior to our gh action test for dunesql compatability. we could also check if they use the expose_spells_hide_trino macro in config since they are known to fail there, then we could look into merging regardless of passing that test

@Hosuke
Copy link
Collaborator

Hosuke commented May 27, 2023

hmm, looks the the anti join isn't dunesql compatible. since these are views, they'd need to be run on dunesql syntax too. i wonder if these were built prior to our gh action test for dunesql compatability. we could also check if they use the expose_spells_hide_trino macro in config since they are known to fail there, then we could look into merging regardless of passing that test

We may rewrite anti join in equivalent left join logics.
eg.

select *
from a left join b
  on a.id = b.id
where b.id is NULL

@hildobby
Copy link
Collaborator Author

hmm, looks the the anti join isn't dunesql compatible. since these are views, they'd need to be run on dunesql syntax too. i wonder if these were built prior to our gh action test for dunesql compatability. we could also check if they use the expose_spells_hide_trino macro in config since they are known to fail there, then we could look into merging regardless of passing that test

We may rewrite anti join in equivalent left join logics. eg.

select *
from a left join b
  on a.id = b.id
where b.id is NULL

Issued another commit where I did that, lmk if fix is insufficient

@hildobby hildobby added question Further information is requested and removed do not merge labels May 30, 2023
@Hosuke
Copy link
Collaborator

Hosuke commented May 30, 2023

Hi @hildobby, for the test failure, I cannot identify the exact cause at the moment.
But I am thinking it is not caused by the anti join.

It may be caused by the = operation in WHERE OR CASE OR ON CLAUSE.
eg.
image

One brute-force way to solve this problem, is to change the roll-up spell into incremental view.

Another appropriate solution, is to locate the column comparison and cast type on either side.
https://github.com/duneanalytics/spellbook/actions/runs/5120014833/jobs/9205990766?pr=3432#step:14:18

@MSilb7
Copy link
Contributor

MSilb7 commented Jun 6, 2023

Tested the compiled bridge_flows and ran on dune.com in Spark and it worked with no datatype errors

@hildobby hildobby added ready-for-review this PR development is complete, please review and removed question Further information is requested labels Jun 8, 2023
@jeff-dude
Copy link
Member

Tested the compiled bridge_flows and ran on dune.com in Spark and it worked with no datatype errors

is this still the case? i think we've seen this on other PRs too, where dunesql test is failing and i was a bit confused

@MSilb7
Copy link
Contributor

MSilb7 commented Jun 13, 2023

Tested the compiled bridge_flows and ran on dune.com in Spark and it worked with no datatype errors

is this still the case? i think we've seen this on other PRs too, where dunesql test is failing and i was a bit confused

we should likely just cast the values anyway, I'll add a comment.

@Hosuke
Copy link
Collaborator

Hosuke commented Jun 13, 2023

Wow it fixes.

@jeff-dude
Copy link
Member

thanks all! i'll look to get this pushed through asap

@jeff-dude jeff-dude added in review Assignee is currently reviewing the PR ready-for-merging and removed ready-for-review this PR development is complete, please review in review Assignee is currently reviewing the PR labels Jun 13, 2023
@jeff-dude jeff-dude merged commit 679a342 into duneanalytics:main Jun 13, 2023
2 checks passed
olgafetisova pushed a commit to olgafetisova/spellbook that referenced this pull request Nov 14, 2023
* rename file properly

* ANTI > LEFT JOIN WHERE NULL

* Apply cast to chain_id

* Add size

* Revert "Add size"

This reverts commit 9537e3e.

* Revert "Apply cast to chain_id"

This reverts commit dbae438.

* cast all to decimal38

* fix attempt #1

* cast numbers to double

* Apply cast()

---------

Co-authored-by: Huang Geyang <Sukebeta@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants