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

Limit subquery results to 1 #3181

Merged
merged 1 commit into from
Dec 25, 2024
Merged

Limit subquery results to 1 #3181

merged 1 commit into from
Dec 25, 2024

Conversation

sunce86
Copy link
Contributor

@sunce86 sunce86 commented Dec 25, 2024

Description

Fixes failing of queries when orders with multiple order executions are queried from database.

Introduced in #2966

Changes

  • In case mutliple rows exist, return only one. Right now it's guaranteed that all of them are the same.

How to test

trust me bro
will be tested on staging since all staging pods are down

@sunce86 sunce86 self-assigned this Dec 25, 2024
@sunce86 sunce86 requested a review from a team as a code owner December 25, 2024 13:19
@@ -32,7 +32,7 @@ NULL AS ethflow_data,
NULL AS onchain_user,
NULL AS onchain_placement_error,
COALESCE((SELECT SUM(executed_fee) FROM order_execution oe WHERE oe.order_uid = o.uid), 0) as executed_fee,
COALESCE((SELECT executed_fee_token FROM order_execution oe WHERE oe.order_uid = o.uid), o.sell_token) as executed_fee_token, -- TODO surplus token
COALESCE((SELECT executed_fee_token FROM order_execution oe WHERE oe.order_uid = o.uid LIMIT 1), o.sell_token) as executed_fee_token, -- TODO surplus token
Copy link
Contributor

Choose a reason for hiding this comment

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

I have the full query compiled. Tested to be 100% sure. Works fine. But should we use SELECT DISTINCT oe.executed_fee_token here instead? This would fail if there were multiple different executed fee tokens for the same order, which would signal that something was definitely wrong. While limiting ignores this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's too early to use DISTINCT here for the purpose you explained.

Right now we save all fees in sell token so for a single order, these are guaranteed to be same.
Later, when we switch to saving fee in surplus token, we will potentially have orders with different tokens - but this will be a temporary bug that I explained here #2966 (comment)
Once I do manual db migration to convert all historical fees to be expressed over surplus token, then again all fees will be the same for a single order, and we can use DISTINCT again.

Copy link
Contributor Author

@sunce86 sunce86 Dec 25, 2024

Choose a reason for hiding this comment

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

But yes, you are right, we CAN use "SELECT DISTINCT" instead at this point.

But I'd rather not fail the whole query if it fails (which it shouldn't but 🤷 )

Copy link
Contributor

@squadgazzz squadgazzz left a comment

Choose a reason for hiding this comment

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

Tested manually. LGTM, assuming the limit will be replaced with distinct in the future.

@sunce86 sunce86 merged commit bbd1de3 into main Dec 25, 2024
11 checks passed
@sunce86 sunce86 deleted the fix-subquery-for-executed-fee branch December 25, 2024 13:37
@github-actions github-actions bot locked and limited conversation to collaborators Dec 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants