-
Notifications
You must be signed in to change notification settings - Fork 88
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
Conversation
@@ -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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 🤷 )
There was a problem hiding this 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.
Description
Fixes failing of queries when orders with multiple order executions are queried from database.
Introduced in #2966
Changes
How to test
trust me bro
will be tested on staging since all staging pods are down