add columns to polymarket models#9107
Conversation
github-actions
bot
added
WIP
github-actions
bot
added
ready-for-review
PR SummaryIntroduces new analytics surfaces and enriches existing Polymarket models for better querying and pricing.
Written by Cursor Bugbot for commit d775b77. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
...ily_spellbook/models/_projects/polymarket/polygon/polymarket_polygon_market_prices_daily.sql
Outdated
Show resolved
Hide resolved
...ects/daily_spellbook/models/_projects/polymarket/polygon/polymarket_polygon_protocol_tvl.sql
Outdated
Show resolved
Hide resolved
...ts/daily_spellbook/models/_projects/polymarket/polygon/polymarket_polygon_open_positions.sql
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
dbt_subprojects/daily_spellbook/models/_projects/polymarket/polygon/polymarket_polygon_users_balance_changes.sql (3)
1-11:⚠️ Potential issue | 🟠 MajorAdd
expose_spellsin the model config.This new model config is still missing
post_hookexposure, so it won’t be published as expected.As per coding guidelines: "Use expose_spells macro in post_hook to make models publicly accessible on dune.com."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dbt_subprojects/daily_spellbook/models/_projects/polymarket/polygon/polymarket_polygon_users_balance_changes.sql` around lines 1 - 11, The model config for alias 'users_balance_changes' is missing the post_hook that calls the expose_spells macro; update the config(...) block to add a post_hook that invokes expose_spells for this model so it is published on dune.com (ensure the post_hook references the model alias 'users_balance_changes' or uses the expose_spells macro with the correct model context); keep other config fields (schema, alias, materialized, file_format, incremental_strategy, unique_key, incremental_predicates) unchanged.
8-10:⚠️ Potential issue | 🟠 MajorUse partition-friendly incremental keying/filtering (
block_date).Line 9 and Lines 40/64 still predicate on
block_time, andunique_keyomits the partition column. This hurts partition pruning and incremental merge behavior for this model.Suggested change
- unique_key = ['block_number', 'tx_index', 'evt_index', 'direction'], - incremental_predicates = [incremental_predicate('DBT_INTERNAL_DEST.block_time')] + unique_key = ['block_date', 'block_number', 'tx_index', 'evt_index', 'direction'], + incremental_predicates = [incremental_predicate('DBT_INTERNAL_DEST.block_date')] ... - AND {{ incremental_predicate('t.block_time') }} + AND {{ incremental_predicate('t.block_date') }} ... - AND {{ incremental_predicate('t.block_time') }} + AND {{ incremental_predicate('t.block_date') }}Based on learnings: "For incremental models, include the partition column in unique_key and use block_date filters (not block_time) in non-incremental mode."
Also applies to: 39-41, 63-65
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dbt_subprojects/daily_spellbook/models/_projects/polymarket/polygon/polymarket_polygon_users_balance_changes.sql` around lines 8 - 10, Change the model to use the partition column and date-based filtering: update unique_key to include the partition column (e.g., add "DBT_INTERNAL_DEST.block_date" or "block_date" alongside ['block_number','tx_index','evt_index','direction']) and replace all uses of incremental_predicate('DBT_INTERNAL_DEST.block_time') / predicates on block_time with incremental_predicate('DBT_INTERNAL_DEST.block_date') (and any non-incremental WHEREs that filter on block_time to filter on block_date instead) so partition pruning and incremental merges work correctly (adjust every occurrence where unique_key and incremental_predicate('...block_time') appear).
160-203:⚠️ Potential issue | 🟠 MajorMake inflow/outflow/internal predicates mutually exclusive.
Internal transfers currently satisfy inflow/outflow predicates too, which can overcount downstream aggregates.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dbt_subprojects/daily_spellbook/models/_projects/polymarket/polygon/polymarket_polygon_users_balance_changes.sql` around lines 160 - 203, The inflow/outflow rows must exclude internal transfers so aggregates don't double-count: in the SELECT that emits 'inflow' (currently WHERE to_polymarket_wallet IS NOT NULL ...) add AND from_polymarket_wallet IS NULL (and keep the first_funded_block check on to_first_funded_block); in the 'outflow' SELECT add AND to_polymarket_wallet IS NULL (keeping the from_first_funded_block check); leave the 'internal' SELECT as the case where both from_polymarket_wallet IS NOT NULL AND to_polymarket_wallet IS NOT NULL. All changes should be applied to the queries reading from relevant_transfers_with_ffb and preserve the existing first_funded_block conditions (to_first_funded_block / from_first_funded_block) for each side.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@dbt_subprojects/daily_spellbook/models/_projects/polymarket/polygon/polymarket_polygon_users_balance_changes.sql`:
- Around line 68-104: The CTE relevant_transfers_all selects many bare column
names from relevant_transfers_in twice; update both SELECT blocks to use
explicit table aliases (e.g., alias relevant_transfers_in AS r) and prefix every
projected column (r.block_time, r.block_date, r.block_month, r.block_number,
r.tx_hash, r.tx_from, r.tx_to, r.tx_index, r.evt_index, r.amount,
r.from_address, r.to_address, r.unique_key, etc.) and keep the explicit
CAST(...) aliases for from_polymarket_wallet/to_polymarket_wallet; ensure both
the first SELECT and the UNION ALL second SELECT consistently use the same
aliasing.
- Around line 88-104: The UNION branch that selects from_polymarket_wallet (and
CAST(NULL AS VARBINARY) AS to_polymarket_wallet) is mistakenly using FROM
relevant_transfers_in; update that branch to read FROM relevant_transfers_out so
the available column from_polymarket_wallet is sourced correctly (ensure the
SELECT still projects block_time, block_date, block_month, block_number,
tx_hash, tx_from, tx_to, tx_index, evt_index, amount, from_address, to_address,
unique_key, from_polymarket_wallet, CAST(NULL AS VARBINARY) AS
to_polymarket_wallet from relevant_transfers_out).
- Around line 126-127: The query references relevant_transfers_with_ffb and
fields to_first_funded_block/from_first_funded_block but no CTE defines them;
fix by creating a CTE (named relevant_transfers_with_ffb) that joins the
existing relevant_transfers CTE to the polymarket_first_funded CTE (or rename
polymarket_first_funded into the expected name) so that each transfer row
includes to_first_funded_block and from_first_funded_block; update downstream
WHERE predicates to use those joined fields (ensure the join keys are the
relevant address/contract columns used in polymarket_first_funded and
relevant_transfers).
---
Duplicate comments:
In
`@dbt_subprojects/daily_spellbook/models/_projects/polymarket/polygon/polymarket_polygon_users_balance_changes.sql`:
- Around line 1-11: The model config for alias 'users_balance_changes' is
missing the post_hook that calls the expose_spells macro; update the config(...)
block to add a post_hook that invokes expose_spells for this model so it is
published on dune.com (ensure the post_hook references the model alias
'users_balance_changes' or uses the expose_spells macro with the correct model
context); keep other config fields (schema, alias, materialized, file_format,
incremental_strategy, unique_key, incremental_predicates) unchanged.
- Around line 8-10: Change the model to use the partition column and date-based
filtering: update unique_key to include the partition column (e.g., add
"DBT_INTERNAL_DEST.block_date" or "block_date" alongside
['block_number','tx_index','evt_index','direction']) and replace all uses of
incremental_predicate('DBT_INTERNAL_DEST.block_time') / predicates on block_time
with incremental_predicate('DBT_INTERNAL_DEST.block_date') (and any
non-incremental WHEREs that filter on block_time to filter on block_date
instead) so partition pruning and incremental merges work correctly (adjust
every occurrence where unique_key and incremental_predicate('...block_time')
appear).
- Around line 160-203: The inflow/outflow rows must exclude internal transfers
so aggregates don't double-count: in the SELECT that emits 'inflow' (currently
WHERE to_polymarket_wallet IS NOT NULL ...) add AND from_polymarket_wallet IS
NULL (and keep the first_funded_block check on to_first_funded_block); in the
'outflow' SELECT add AND to_polymarket_wallet IS NULL (keeping the
from_first_funded_block check); leave the 'internal' SELECT as the case where
both from_polymarket_wallet IS NOT NULL AND to_polymarket_wallet IS NOT NULL.
All changes should be applied to the queries reading from
relevant_transfers_with_ffb and preserve the existing first_funded_block
conditions (to_first_funded_block / from_first_funded_block) for each side.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dbt_subprojects/daily_spellbook/models/_projects/polymarket/polygon/polymarket_polygon_users_balance_changes.sql
...y_spellbook/models/_projects/polymarket/polygon/polymarket_polygon_users_balance_changes.sql
Outdated
Show resolved
Hide resolved
...y_spellbook/models/_projects/polymarket/polygon/polymarket_polygon_users_balance_changes.sql
Outdated
Show resolved
Hide resolved
...y_spellbook/models/_projects/polymarket/polygon/polymarket_polygon_users_balance_changes.sql
Show resolved
Hide resolved
There was a problem hiding this comment.
♻️ Duplicate comments (5)
dbt_subprojects/daily_spellbook/models/_projects/polymarket/polygon/polymarket_polygon_users_balance_changes.sql (5)
1-11:⚠️ Potential issue | 🟠 MajorMissing
expose_spellspost-hook in model config.This model config does not expose the spell publicly; please add the
post_hookforusers_balance_changesinpolymarket_polygon.As per coding guidelines: "Use expose_spells macro in post_hook to make models publicly accessible on dune.com".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dbt_subprojects/daily_spellbook/models/_projects/polymarket/polygon/polymarket_polygon_users_balance_changes.sql` around lines 1 - 11, Add a post_hook to the model config that calls the expose_spells macro so the model alias is publicly exposed; update the config block in polymarket_polygon.models (the config for alias 'users_balance_changes') to include a post_hook entry invoking expose_spells with the fully-qualified dataset (use the model alias/schema combination or the same identifier your project uses) so the model is registered on dune.com; ensure the post_hook is added alongside the existing config keys (materialized, unique_key, incremental_predicates) and references the expose_spells macro name exactly.
69-104:⚠️ Potential issue | 🟡 MinorUse explicit source aliases in SELECT projections.
These SELECT lists project many bare columns (for example Line 69+ and Line 128+). Please prefix projected fields with table aliases consistently.
As per coding guidelines: "Always use explicit table aliases — prefix all columns (t.column, p.column), never use bare column names in DuneSQL/Trino SQL".
Also applies to: 128-204
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dbt_subprojects/daily_spellbook/models/_projects/polymarket/polygon/polymarket_polygon_users_balance_changes.sql` around lines 69 - 104, The two UNION ALL SELECT blocks are projecting bare columns — update both SELECTs that read from relevant_transfers_in and relevant_transfers_out to prefix every column with the appropriate source alias (e.g., r.block_time, r.block_date, r.block_month, r.block_number, r.tx_hash, r.tx_from, r.tx_to, r.tx_index, r.evt_index, r.amount, r.from_address, r.to_address, r.unique_key) and keep the CASTs aliased (e.g., CAST(NULL AS VARBINARY) AS from_polymarket_wallet) so all projections consistently use explicit table aliases for clarity and to satisfy the coding guideline.
8-10:⚠️ Potential issue | 🟠 MajorIncremental partition strategy is using
block_timeinstead ofblock_date.Line 9, Line 40, and Line 64 currently use
block_time, andunique_keyomits the partition column. This prevents partition-friendly pruning and conflicts with project incremental standards.Suggested fix
- unique_key = ['block_number', 'tx_index', 'evt_index', 'direction'], - incremental_predicates = [incremental_predicate('DBT_INTERNAL_DEST.block_time')] + unique_key = ['block_date', 'block_number', 'tx_index', 'evt_index', 'direction'], + incremental_predicates = [incremental_predicate('DBT_INTERNAL_DEST.block_date')] ... - AND {{ incremental_predicate('t.block_time') }} + AND {{ incremental_predicate('t.block_date') }} ... - AND {{ incremental_predicate('t.block_time') }} + AND {{ incremental_predicate('t.block_date') }}Based on learnings: "For incremental models, include the partition column in unique_key and use block_date filters (not block_time) in non-incremental mode."
Also applies to: 39-41, 63-65
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dbt_subprojects/daily_spellbook/models/_projects/polymarket/polygon/polymarket_polygon_users_balance_changes.sql` around lines 8 - 10, The incremental config currently uses incremental_predicate('DBT_INTERNAL_DEST.block_time') and a unique_key that omits the partition column; change the incremental predicates to use DBT_INTERNAL_DEST.block_date instead of block_time, and add the partition column (block_date) into the unique_key array so the model becomes partition-friendly; update any non-incremental WHERE filters (e.g., in functions or CTEs referencing DBT_INTERNAL_DEST.block_time) to use DBT_INTERNAL_DEST.block_date for date-based pruning and ensure unique_key includes ['block_date', 'block_number', 'tx_index', 'evt_index', 'direction'] (or equivalent ordering used in this model).
160-203:⚠️ Potential issue | 🟠 MajorDirection predicates still overlap and can double-count internal transfers.
inflowandoutflowbranches currently include rows where both polymarket wallets are present, which also match theinternalbranch.Suggested exclusivity fix
FROM relevant_transfers_with_ffb WHERE to_polymarket_wallet IS NOT NULL + AND from_polymarket_wallet IS NULL AND (to_first_funded_block IS NULL OR to_first_funded_block <= block_number) ... FROM relevant_transfers_with_ffb WHERE from_polymarket_wallet IS NOT NULL + AND to_polymarket_wallet IS NULL AND (from_first_funded_block IS NULL OR from_first_funded_block <= block_number)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dbt_subprojects/daily_spellbook/models/_projects/polymarket/polygon/polymarket_polygon_users_balance_changes.sql` around lines 160 - 203, The inflow/outflow/internal UNION branches in the query over relevant_transfers_with_ffb overlap and can double-count rows; make the predicates mutually exclusive by changing the inflow branch to require to_polymarket_wallet IS NOT NULL AND from_polymarket_wallet IS NULL, the outflow branch to require from_polymarket_wallet IS NOT NULL AND to_polymarket_wallet IS NULL, and keep the internal branch as from_polymarket_wallet IS NOT NULL AND to_polymarket_wallet IS NOT NULL; preserve the existing first-funded-block checks (to_first_funded_block / from_first_funded_block <= block_number or IS NULL) on each branch.
159-201:⚠️ Potential issue | 🔴 Critical
relevant_transfers_with_ffbis referenced but never defined.Line 159, Line 180, and Line 201 select from
relevant_transfers_with_ffb, but no CTE defines it. This will fail at compile time.Minimal fix
, relevant_transfers AS ( SELECT MAX(block_time) AS block_time , MAX(block_date) AS block_date , MAX(block_month) AS block_month , MAX(block_number) AS block_number , MAX(tx_hash) AS tx_hash , MAX(tx_from) AS tx_from , MAX(tx_to) AS tx_to , MAX(tx_index) AS tx_index , MAX(evt_index) AS evt_index , MAX(amount) AS amount , MAX(from_address) AS from_address , MAX(to_address) AS to_address , unique_key , MAX(from_polymarket_wallet) AS from_polymarket_wallet , MAX(to_polymarket_wallet) AS to_polymarket_wallet FROM relevant_transfers_all GROUP BY unique_key ) + +, relevant_transfers_with_ffb AS ( + SELECT rt.block_time + , rt.block_date + , rt.block_month + , rt.block_number + , rt.tx_hash + , rt.tx_from + , rt.tx_to + , rt.tx_index + , rt.evt_index + , rt.amount + , rt.from_address + , rt.to_address + , rt.unique_key + , rt.from_polymarket_wallet + , rt.to_polymarket_wallet + , from_ffb.first_funded_block AS from_first_funded_block + , to_ffb.first_funded_block AS to_first_funded_block + FROM relevant_transfers rt + LEFT JOIN polymarket_first_funded from_ffb ON rt.from_polymarket_wallet = from_ffb.address + LEFT JOIN polymarket_first_funded to_ffb ON rt.to_polymarket_wallet = to_ffb.address +)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dbt_subprojects/daily_spellbook/models/_projects/polymarket/polygon/polymarket_polygon_users_balance_changes.sql` around lines 159 - 201, The query references a CTE named relevant_transfers_with_ffb in three SELECTs but that CTE is not defined; either add a CTE definition for relevant_transfers_with_ffb above these UNION ALL blocks (building the same columns: block_time, block_date, block_month, block_number, tx_hash, tx_from, tx_to, tx_index, evt_index, from_polymarket_wallet, to_polymarket_wallet, amount, to_address, unique_key, from_first_funded_block, to_first_funded_block) or update the three FROM clauses to the correct existing CTE name if it was misnamed—ensure the chosen CTE returns the exact columns used by the 'inflow', 'outflow', and 'internal' SELECTs so the UNION ALL compiles.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@dbt_subprojects/daily_spellbook/models/_projects/polymarket/polygon/polymarket_polygon_users_balance_changes.sql`:
- Around line 1-11: Add a post_hook to the model config that calls the
expose_spells macro so the model alias is publicly exposed; update the config
block in polymarket_polygon.models (the config for alias
'users_balance_changes') to include a post_hook entry invoking expose_spells
with the fully-qualified dataset (use the model alias/schema combination or the
same identifier your project uses) so the model is registered on dune.com;
ensure the post_hook is added alongside the existing config keys (materialized,
unique_key, incremental_predicates) and references the expose_spells macro name
exactly.
- Around line 69-104: The two UNION ALL SELECT blocks are projecting bare
columns — update both SELECTs that read from relevant_transfers_in and
relevant_transfers_out to prefix every column with the appropriate source alias
(e.g., r.block_time, r.block_date, r.block_month, r.block_number, r.tx_hash,
r.tx_from, r.tx_to, r.tx_index, r.evt_index, r.amount, r.from_address,
r.to_address, r.unique_key) and keep the CASTs aliased (e.g., CAST(NULL AS
VARBINARY) AS from_polymarket_wallet) so all projections consistently use
explicit table aliases for clarity and to satisfy the coding guideline.
- Around line 8-10: The incremental config currently uses
incremental_predicate('DBT_INTERNAL_DEST.block_time') and a unique_key that
omits the partition column; change the incremental predicates to use
DBT_INTERNAL_DEST.block_date instead of block_time, and add the partition column
(block_date) into the unique_key array so the model becomes partition-friendly;
update any non-incremental WHERE filters (e.g., in functions or CTEs referencing
DBT_INTERNAL_DEST.block_time) to use DBT_INTERNAL_DEST.block_date for date-based
pruning and ensure unique_key includes ['block_date', 'block_number',
'tx_index', 'evt_index', 'direction'] (or equivalent ordering used in this
model).
- Around line 160-203: The inflow/outflow/internal UNION branches in the query
over relevant_transfers_with_ffb overlap and can double-count rows; make the
predicates mutually exclusive by changing the inflow branch to require
to_polymarket_wallet IS NOT NULL AND from_polymarket_wallet IS NULL, the outflow
branch to require from_polymarket_wallet IS NOT NULL AND to_polymarket_wallet IS
NULL, and keep the internal branch as from_polymarket_wallet IS NOT NULL AND
to_polymarket_wallet IS NOT NULL; preserve the existing first-funded-block
checks (to_first_funded_block / from_first_funded_block <= block_number or IS
NULL) on each branch.
- Around line 159-201: The query references a CTE named
relevant_transfers_with_ffb in three SELECTs but that CTE is not defined; either
add a CTE definition for relevant_transfers_with_ffb above these UNION ALL
blocks (building the same columns: block_time, block_date, block_month,
block_number, tx_hash, tx_from, tx_to, tx_index, evt_index,
from_polymarket_wallet, to_polymarket_wallet, amount, to_address, unique_key,
from_first_funded_block, to_first_funded_block) or update the three FROM clauses
to the correct existing CTE name if it was misnamed—ensure the chosen CTE
returns the exact columns used by the 'inflow', 'outflow', and 'internal'
SELECTs so the UNION ALL compiles.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dbt_subprojects/daily_spellbook/models/_projects/polymarket/polygon/polymarket_polygon_users_balance_changes.sql
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
dbt_subprojects/daily_spellbook/models/_projects/polymarket/polygon/polymarket_polygon_users_balance_changes.sql (1)
70-130: Use explicit table aliases for projected columns.The
relevant_transfers_allandrelevant_transfersCTEs select bare column names without source alias prefixes. While there's no ambiguity in single-source CTEs, this violates the coding guideline for explicit aliases.Example fix for relevant_transfers_all
, relevant_transfers_all AS ( - SELECT block_time - , block_date + SELECT rti.block_time + , rti.block_date ... - FROM relevant_transfers_in + FROM relevant_transfers_in rtiAs per coding guidelines: "Always use explicit table aliases — prefix all columns (t.column, p.column), never use bare column names in DuneSQL/Trino SQL."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dbt_subprojects/daily_spellbook/models/_projects/polymarket/polygon/polymarket_polygon_users_balance_changes.sql` around lines 70 - 130, The CTEs relevant_transfers_all and relevant_transfers use bare column names; update both SELECT lists to prefix every projected column with the appropriate source table alias (e.g., use alias for relevant_transfers_in and relevant_transfers_out rows in relevant_transfers_all like ri.block_time, ri.block_date, ..., and ro.block_time, ro.block_date, ...), and in the aggregating CTE relevant_transfers alias relevant_transfers_all (e.g., ra.max(block_time) becomes MAX(ra.block_time), MAX(ra.tx_hash), etc.) and use ra.unique_key in the GROUP BY; ensure all columns (from_polymarket_wallet, to_polymarket_wallet, from_first_funded_block, to_first_funded_block, unique_key, etc.) are consistently prefixed with the chosen aliases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@dbt_subprojects/daily_spellbook/models/_projects/polymarket/polygon/polymarket_polygon_users_balance_changes.sql`:
- Line 165: The model fails because the query references a non-existent CTE
relevant_transfers_with_ffb and also dropped the from_first_funded_block and
to_first_funded_block columns in the relevant_transfers CTE; fix by either
renaming the existing relevant_transfers CTE to relevant_transfers_with_ffb or
adding a new CTE named relevant_transfers_with_ffb that SELECTs the same rows
but preserves from_first_funded_block and to_first_funded_block (i.e.,
remove/adjust any aggregation or GROUP BY that dropped those fields in the
relevant_transfers CTE), then ensure subsequent references (FROM
relevant_transfers_with_ffb and WHERE predicates) use those preserved columns.
- Around line 166-167: The three direction branches (inflow, outflow, internal)
currently overlap when both from_polymarket_wallet and to_polymarket_wallet are
non-NULL; make them mutually exclusive by tightening predicates: define internal
transfers as WHERE from_polymarket_wallet IS NOT NULL AND to_polymarket_wallet
IS NOT NULL (plus the existing funding/block filters), define inflows as WHERE
to_polymarket_wallet IS NOT NULL AND from_polymarket_wallet IS NULL (plus
to_first_funded_block filter), and define outflows as WHERE
from_polymarket_wallet IS NOT NULL AND to_polymarket_wallet IS NULL (plus
from_first_funded_block or related filters); update the WHERE clauses that
reference to_polymarket_wallet, from_polymarket_wallet, to_first_funded_block
and from_first_funded_block accordingly so each row matches exactly one branch.
---
Nitpick comments:
In
`@dbt_subprojects/daily_spellbook/models/_projects/polymarket/polygon/polymarket_polygon_users_balance_changes.sql`:
- Around line 70-130: The CTEs relevant_transfers_all and relevant_transfers use
bare column names; update both SELECT lists to prefix every projected column
with the appropriate source table alias (e.g., use alias for
relevant_transfers_in and relevant_transfers_out rows in relevant_transfers_all
like ri.block_time, ri.block_date, ..., and ro.block_time, ro.block_date, ...),
and in the aggregating CTE relevant_transfers alias relevant_transfers_all
(e.g., ra.max(block_time) becomes MAX(ra.block_time), MAX(ra.tx_hash), etc.) and
use ra.unique_key in the GROUP BY; ensure all columns (from_polymarket_wallet,
to_polymarket_wallet, from_first_funded_block, to_first_funded_block,
unique_key, etc.) are consistently prefixed with the chosen aliases.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dbt_subprojects/daily_spellbook/models/_projects/polymarket/polygon/polymarket_polygon_users_balance_changes.sql
...y_spellbook/models/_projects/polymarket/polygon/polymarket_polygon_users_balance_changes.sql
Outdated
Show resolved
Hide resolved
| WHERE to_polymarket_wallet IS NOT NULL | ||
| AND (to_first_funded_block IS NULL OR to_first_funded_block <= block_number) |
There was a problem hiding this comment.
Direction predicates overlap — internal transfers counted three times.
When both from_polymarket_wallet and to_polymarket_wallet are non-NULL, the transfer matches all three branches (inflow, outflow, internal), causing overcounting in downstream aggregates.
Fix: Make branches mutually exclusive
FROM relevant_transfers_with_ffb
WHERE to_polymarket_wallet IS NOT NULL
+ AND from_polymarket_wallet IS NULL
AND (to_first_funded_block IS NULL OR to_first_funded_block <= block_number)
UNION ALL
...
FROM relevant_transfers_with_ffb
WHERE from_polymarket_wallet IS NOT NULL
+ AND to_polymarket_wallet IS NULL
AND (from_first_funded_block IS NULL OR from_first_funded_block <= block_number)
UNION ALL
...
FROM relevant_transfers_with_ffb
WHERE from_polymarket_wallet IS NOT NULL AND to_polymarket_wallet IS NOT NULL
AND (from_first_funded_block IS NULL OR from_first_funded_block <= block_number)Also applies to: 187-188, 208-209
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@dbt_subprojects/daily_spellbook/models/_projects/polymarket/polygon/polymarket_polygon_users_balance_changes.sql`
around lines 166 - 167, The three direction branches (inflow, outflow, internal)
currently overlap when both from_polymarket_wallet and to_polymarket_wallet are
non-NULL; make them mutually exclusive by tightening predicates: define internal
transfers as WHERE from_polymarket_wallet IS NOT NULL AND to_polymarket_wallet
IS NOT NULL (plus the existing funding/block filters), define inflows as WHERE
to_polymarket_wallet IS NOT NULL AND from_polymarket_wallet IS NULL (plus
to_first_funded_block filter), and define outflows as WHERE
from_polymarket_wallet IS NOT NULL AND to_polymarket_wallet IS NULL (plus
from_first_funded_block or related filters); update the WHERE clauses that
reference to_polymarket_wallet, from_polymarket_wallet, to_first_funded_block
and from_first_funded_block accordingly so each row matches exactly one branch.
There was a problem hiding this comment.
♻️ Duplicate comments (4)
dbt_subprojects/daily_spellbook/models/_projects/polymarket/polygon/polymarket_polygon_users_balance_changes.sql (4)
8-10:⚠️ Potential issue | 🟠 MajorUse
block_datefor incremental predicate/keying instead ofblock_time.Line 9 uses
DBT_INTERNAL_DEST.block_time, and Line 41/Line 66 uset.block_time; also Line 8unique_keyomits the partition column. This hurts partition pruning on transfers.Suggested patch
- unique_key = ['block_number', 'tx_index', 'evt_index', 'direction'], - incremental_predicates = [incremental_predicate('DBT_INTERNAL_DEST.block_time')] + unique_key = ['block_date', 'block_number', 'tx_index', 'evt_index', 'direction'], + incremental_predicates = [incremental_predicate('DBT_INTERNAL_DEST.block_date')] ... - AND {{ incremental_predicate('t.block_time') }} + AND {{ incremental_predicate('t.block_date') }} ... - AND {{ incremental_predicate('t.block_time') }} + AND {{ incremental_predicate('t.block_date') }}Based on learnings: "For incremental models, include the partition column in unique_key and use block_date filters (not block_time) in non-incremental mode." As per coding guidelines: "Filter on block_date (not block_time) for partition pruning, and always include partition columns in WHERE and JOIN conditions."
Also applies to: 41-42, 66-67
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dbt_subprojects/daily_spellbook/models/_projects/polymarket/polygon/polymarket_polygon_users_balance_changes.sql` around lines 8 - 10, The model is using block_time in incremental_predicates and filters and omits the partition column from unique_key which prevents partition pruning; update unique_key to include the partition column (block_date) and change any references to DBT_INTERNAL_DEST.block_time and t.block_time to use DBT_INTERNAL_DEST.block_date / t.block_date (or the model's partition column) for incremental_predicate and all WHERE/JOIN filters so partition pruning works correctly; ensure the incremental_predicates call uses incremental_predicate('DBT_INTERNAL_DEST.block_date') and that any joins/filters reference the same block_date column consistently.
1-11:⚠️ Potential issue | 🟠 MajorAdd
expose_spellspost_hookin model config.Line 2-Line 10 defines the model config but does not expose the model publicly; please add the
expose_spellshook.As per coding guidelines: "Use expose_spells macro in post_hook to make models publicly accessible on dune.com."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dbt_subprojects/daily_spellbook/models/_projects/polymarket/polygon/polymarket_polygon_users_balance_changes.sql` around lines 1 - 11, Add a post_hook to the model config that calls the expose_spells macro so the model (alias 'users_balance_changes' configured in the config(...) block) is publicly exposed; update the config(...) in this file to include a post_hook entry that invokes expose_spells with the model/schema context (using the same schema 'polymarket_polygon' and alias 'users_balance_changes') so Dune can access the model.
70-131:⚠️ Potential issue | 🟡 MinorPrefix projected columns with explicit table aliases.
Line 71+ and Line 136+ still project many bare columns (
block_time,tx_hash,unique_key, etc.). Please alias CTEs/subqueries and prefix every selected column (r.block_time,t.block_time, ...).As per coding guidelines: "Always use explicit table aliases — prefix all columns (t.column, p.column), never use bare column names in DuneSQL/Trino SQL."
Also applies to: 136-212
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dbt_subprojects/daily_spellbook/models/_projects/polymarket/polygon/polymarket_polygon_users_balance_changes.sql` around lines 70 - 131, The SELECT lists in the CTEs relevant_transfers_all and relevant_transfers use bare column names; alias the source CTEs/subqueries (e.g., name relevant_transfers_all rows as r_all or r, and the grouped source as r_agg) and prefix every projected column (e.g., r.block_time, r.tx_hash, r.unique_key, r.amount, r.from_address, r.to_address, r.from_polymarket_wallet, r.to_polymarket_wallet, r.from_first_funded_block, r.to_first_funded_block) to remove ambiguity; do the same for the UNION inputs (relevant_transfers_in and relevant_transfers_out) so every SELECT element is explicitly qualified with its table alias.
168-190:⚠️ Potential issue | 🟠 MajorMake direction branches mutually exclusive to avoid double counting internal transfers.
Rows where both wallets are present currently match inflow/outflow and internal branches, so downstream aggregates can be inflated.
Suggested predicate tightening
FROM relevant_transfers WHERE to_polymarket_wallet IS NOT NULL + AND from_polymarket_wallet IS NULL AND (to_first_funded_block IS NULL OR to_first_funded_block <= block_number) UNION ALL ... FROM relevant_transfers WHERE from_polymarket_wallet IS NOT NULL + AND to_polymarket_wallet IS NULL AND (from_first_funded_block IS NULL OR from_first_funded_block <= block_number)Also applies to: 210-211
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dbt_subprojects/daily_spellbook/models/_projects/polymarket/polygon/polymarket_polygon_users_balance_changes.sql` around lines 168 - 190, The inflow/outflow branches in the UNION ALL (the SELECTs that set 'inflow' and 'outflow' as direction from relevant_transfers) must be made mutually exclusive with the internal-transfer branch: tighten the WHERE predicates by adding "AND from_polymarket_wallet IS NULL" to the inflow branch (the SELECT that sets 'inflow' AS direction and tests to_polymarket_wallet) and "AND to_polymarket_wallet IS NULL" to the outflow branch (the SELECT that sets 'outflow' AS direction and tests from_polymarket_wallet), and apply the same predicate tightening to the other duplicate branch occurrences referenced (lines ~210-211) so rows where both wallets are present are only captured by the internal-transfer logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@dbt_subprojects/daily_spellbook/models/_projects/polymarket/polygon/polymarket_polygon_users_balance_changes.sql`:
- Around line 8-10: The model is using block_time in incremental_predicates and
filters and omits the partition column from unique_key which prevents partition
pruning; update unique_key to include the partition column (block_date) and
change any references to DBT_INTERNAL_DEST.block_time and t.block_time to use
DBT_INTERNAL_DEST.block_date / t.block_date (or the model's partition column)
for incremental_predicate and all WHERE/JOIN filters so partition pruning works
correctly; ensure the incremental_predicates call uses
incremental_predicate('DBT_INTERNAL_DEST.block_date') and that any joins/filters
reference the same block_date column consistently.
- Around line 1-11: Add a post_hook to the model config that calls the
expose_spells macro so the model (alias 'users_balance_changes' configured in
the config(...) block) is publicly exposed; update the config(...) in this file
to include a post_hook entry that invokes expose_spells with the model/schema
context (using the same schema 'polymarket_polygon' and alias
'users_balance_changes') so Dune can access the model.
- Around line 70-131: The SELECT lists in the CTEs relevant_transfers_all and
relevant_transfers use bare column names; alias the source CTEs/subqueries
(e.g., name relevant_transfers_all rows as r_all or r, and the grouped source as
r_agg) and prefix every projected column (e.g., r.block_time, r.tx_hash,
r.unique_key, r.amount, r.from_address, r.to_address, r.from_polymarket_wallet,
r.to_polymarket_wallet, r.from_first_funded_block, r.to_first_funded_block) to
remove ambiguity; do the same for the UNION inputs (relevant_transfers_in and
relevant_transfers_out) so every SELECT element is explicitly qualified with its
table alias.
- Around line 168-190: The inflow/outflow branches in the UNION ALL (the SELECTs
that set 'inflow' and 'outflow' as direction from relevant_transfers) must be
made mutually exclusive with the internal-transfer branch: tighten the WHERE
predicates by adding "AND from_polymarket_wallet IS NULL" to the inflow branch
(the SELECT that sets 'inflow' AS direction and tests to_polymarket_wallet) and
"AND to_polymarket_wallet IS NULL" to the outflow branch (the SELECT that sets
'outflow' AS direction and tests from_polymarket_wallet), and apply the same
predicate tightening to the other duplicate branch occurrences referenced (lines
~210-211) so rows where both wallets are present are only captured by the
internal-transfer logic.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dbt_subprojects/daily_spellbook/models/_projects/polymarket/polygon/polymarket_polygon_users_balance_changes.sql
|
PR is ready btw, it has a a few new spells and some new columns in a few tables I also optimised the spells a bunch since runtime was crazy on some of those. It's far from perfect but should require a lot less compute than they used to. I made sure to keep all old columns for backward compatibility. The only missing thing is for the market details spell which needs to have uploaded base table switched from dune's (deprecated afaik) to polymarket's uploaded one but it's missing a needed column so I pinged Primo to ask if he could add it. We can merge as is and tackle that in another follow up PR |
|
@jeff-dude could you review this PR please? |
before diving deep, want to give you heads up that we have three open PRs to materialize polymarket views into incremental tables. once those are deployed and rebuilt, you can merge changes into your branch here. then we can look into the additional columns and tables. our changes will not change the data output or schema at all, simply materialize into tables for performance downstream. |
do as you need, I switched out of dune to using another more reliable source for polymarket data since most things blow past dune's query runtime anyways because the spells are in a rough state and merging this took way too long |
understood. this is why we materialized, to help with query performance. we are also spending time Q2 on enhancing prediction market datasets. |
|
@jeff-dude yes but materialising them won't fix this fyi. you can't even get volume from them right now and need to defer to decoded even tables, same for tvl (costs insane compute because dates are string for example) and many other super basic metrics. the queries are very suboptimal in their current state, hence my PR here. In this PR I tried to keep all currently active columns while adding new ones that I think are more useful (so it will remain with a ton of technical debt of unneeded stuff either way) |
okay thanks for clarifying. i have shared internally with the team for reference. |
3 participants
added columns to existing tables (needed for much more efficient queries) and created 3 tables: