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

SPE-15 migrate dex_aggregator.trades to dunesql engine #4320

Merged
merged 11 commits into from
Sep 29, 2023
Merged

Conversation

jeff-dude
Copy link
Member

todo:

  • migrate zeroex lineage, for now it's commented out

@jeff-dude jeff-dude added WIP work in progress dbt: dex covers the DEX dbt subproject DuneSQL migration labels Sep 12, 2023
@jeff-dude jeff-dude changed the title migrate dex_aggregator.trades to dunesql engine SPE-15 migrate dex_aggregator.trades to dunesql engine Sep 12, 2023
@Hosuke
Copy link
Collaborator

Hosuke commented Sep 22, 2023

#4285 (zeroex op spells) is ready for review.

@Hosuke
Copy link
Collaborator

Hosuke commented Sep 23, 2023

#4273 (zeroex polygon spells) is ready for review.

@Hosuke
Copy link
Collaborator

Hosuke commented Sep 24, 2023

#4410 (zeroex arbitrum, avaxc, fantom spells) is ready for review.

@aalan3
Copy link
Contributor

aalan3 commented Sep 29, 2023

Added some missing block_month columns

@aalan3
Copy link
Contributor

aalan3 commented Sep 29, 2023

I removed the partioning, the table was overpartitioned with block_month now it's slightly under partitioned but I think it's fine.

@aalan3
Copy link
Contributor

aalan3 commented Sep 29, 2023

@jeff-dude I removed the deduplication, let me know if we should add it back.

block_time,
block_number,
tx_hash,
evt_index,
CAST(NULL as array<bigint>) as trace_address,
ARRAY[-1] as trace_address,
Copy link
Contributor

Choose a reason for hiding this comment

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

We should use a null array here

Copy link
Contributor

Choose a reason for hiding this comment

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

why is that? we are using this across all models it seems, not sure why though. One thing to keep in mind is that this is used in the unique key and nulls can cause issues there sometimes.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it was null before though so shouldn't really be a problem. @jeff-dude @couralex6 do you have any context on why we are using ARRAY[-1] vs null array?

Copy link
Contributor

Choose a reason for hiding this comment

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

I will merge this and do a sweeping PR (since they are all over the place already) of ARRAY[-1] replacements if we decide that it's needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, the column is a unique key, so null doesn't find a match on merge joins, therefore inserting rows again. null arrays must have worked differently on spark then. i vaguely recall a small difference during migration (may have been different table/column). you can give it a test on one of the base-level project models that use the hardcoded -1 to see what happens

@aalan3 aalan3 merged commit bfaf1ef into main Sep 29, 2023
@aalan3 aalan3 deleted the migrate-dex-agg branch September 29, 2023 10:30
@github-actions github-actions bot locked and limited conversation to collaborators Sep 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
blocked dbt: dex covers the DEX dbt subproject WIP work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants