The Good
Using subqueries instead of CTEs (which would impose an optimization boundary).
The Bad
You form a window in three different subqueries; it should only be needed in one.
When txn_type = 'deposit' Then txn_amount
is redundant.
This query does not call for formatting. In real life, essentially all queries are consumed by applications and not by people, so the output should be a single floating-point fraction from 0 to 1.
You should not need top 1
with a properly-written count
. You should not count distinct
.
Never label subqueries A
, X
etc.; always give them meaningful names.
txn_amount * -1
is just -txn_amount
.
More broadly, your approach needs to be re-thought. Instead of going through all of the trouble to calculate the actual close-balance change (which is not needed), all you need is a boolean indicating whether that change was high enough. This interpretation has significant benefits, including that you don't need to care about divide-by-zero for the first-row edge case within a window since you don't need to divide at all, only compare.
Your results are incorrect. At least one bug: if there are only deposits and no withdrawals or purchases, the fraction should be 1.0 but your query does not produce any rows.
After a bunch of work, I believe all three methods below to be equivalent and correct. But you'll find that they all return slightly different results! This is explained by the fact that the test data are ambiguous: there is no clear "last transaction" since many transactions overlap and only have dates set, not times. This is the fault of the course, which should have used unique timestamps instead of overlapping dates.
Critically, this calls for unit tests (which are certainly possible, especially in PostgreSQL). If you do not unit test with small, well-defined, repeatable cases, there is no way to know whether your output percentage is correct. As a matter of practice and diligence, any non-trivial queries like this should get unit tests. For instance, with these data:
INSERT INTO customer_transactions
(customer_id, txn_date, txn_type, txn_amount)
VALUES
('429', '2020-01-01', 'deposit', '500'),
('429', '2020-01-02', 'deposit', '500'),
('429', '2020-01-03', 'deposit', '49'),
('622', '2020-01-01', 'deposit', '1'),
('622', '2020-01-02', 'deposit', '1'),
('622', '2020-01-03', 'deposit', '-1'),
('266', '2020-01-01', 'deposit', '1'),
('266', '2020-01-02', 'deposit', '1'),
('266', '2020-01-03', 'deposit', '-1');
your query should produce 0, but replacing the 49 with a 51 should produce 0.333. Without running more such tests it will be difficult to figure out why your query produces 0.2% and mine 41.6%.
Format()
is not a SQL-standard function. This makes me guess that you're using TSQL, which is not as good of an idea as using the course's PostgreSQL.
The course should not have taught you to use a string for a transaction type. You should be using an enum instead.
The course has a poor table definition. It should be closer to something like:
CREATE TABLE customer_transactions (
customer_id INTEGER not null,
txn_date DATE not null,
txn_type VARCHAR(10) not null check(txn_type in ('deposit', 'purchase', 'withdrawal')),
txn_amount INTEGER check (txn_amount >= 0)
);
It's also missing a foreign key.
The Ugly
Standard practice for SQL keywords is to CAPITALIZE them. You use a random mix of CAPITALS and TitleCase. I personally don't like either and prefer lowercase, but you should be consistent. Also keep in mind that your identifiers like BalanceAccumSum
lose their case in many systems (PostgreSQL included), and so balance_accum_sum
stands a better chance at its legibility surviving.
Moving commas to be a prefix on the following line is deeply bizarre and I find illegible. Any small benefit of "some commas lining up on the same column" is thoroughly outweighed by the loss of legibility.
Your indentation ranges from 4 (pretty reasonable) to 8 (not reasonable).
Suggested Query
Again, please test this with small, well-understood cases, comparing your own results to mine (also see fiddle).
set search_path = data_bank;
select (
count(*) filter (where big_gain)
) / cast(count(*) as real) as customers_with_gain_frac
from (
select distinct on (customer_id) customer_id, big_gain
from (
select customer_id,
-- curr/(cumsum - curr) > 0.05
-- curr > cumsum*0.05/1.05
signed_amount > 0.05/1.05*abs(
sum(signed_amount) over cust_window
) as big_gain,
row_number() over cust_window as idx
from (
select
customer_id, txn_date,
case
when txn_type in ('purchase', 'withdrawal') then -txn_amount
else txn_amount
end as signed_amount
from customer_transactions
) transactions_with_sign
window cust_window as (partition by customer_id order by txn_date)
) transactions_with_change
order by customer_id, idx desc
) customers_with_gains;
For a quite different take, this is also possible with no windowing:
with transactions_with_sign as (
select customer_id, txn_date,
case
when txn_type in ('purchase', 'withdrawal') then -txn_amount
else txn_amount
end as signed_amount
from customer_transactions
)
select
count(*) filter (
where last_transactions.signed_amount > 0.05/1.05*abs(accounts.balance)
) / cast(count(*) as real) as customers_with_gain_frac
from
(
select distinct on (customer_id) customer_id, signed_amount
from transactions_with_sign
order by customer_id, txn_date desc
) last_transactions
join (
select customer_id, sum(signed_amount) as balance
from transactions_with_sign
group by customer_id
) accounts on accounts.customer_id = last_transactions.customer_id;
Or, even, with no CTE:
select
count(*) filter (
where last_transactions.txn_amount > 0.05/1.05*abs(accounts.balance)
and last_transactions.txn_type = 'deposit'
) / cast(count(*) as real) as customers_with_gain_frac
from
(
select customer_id,
sum(
case
when txn_type in ('purchase', 'withdrawal') then -txn_amount
else txn_amount
end
) as balance
from customer_transactions
group by customer_id
) accounts
left join (
select distinct on (customer_id) customer_id, txn_type, txn_amount
from customer_transactions
order by customer_id, txn_date desc
) last_transactions
on last_transactions.customer_id = accounts.customer_id;