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

Support unparsing plans with both Aggregation and Window functions #35

Merged
merged 1 commit into from
Sep 30, 2024

Conversation

sgrebnov
Copy link

Which issue does this PR close?

Improve unparsing to support queries containing simultaneously Aggregation and Window functions. Existing version fails with error Tried to unproject agg expr not found in provided Aggregate!

This makes unparsing to correctly handle TPC-DS Q47, Q53, Q57, Q63, Q89

Query Q47:

with v1 as(
 select i_category, i_brand,
        s_store_name, s_company_name,
        d_year, d_moy,
        sum(ss_sales_price) sum_sales,
        avg(sum(ss_sales_price)) over
          (partition by i_category, i_brand,
                     s_store_name, s_company_name, d_year)
          avg_monthly_sales,
        rank() over
          (partition by i_category, i_brand,
                     s_store_name, s_company_name
           order by d_year, d_moy) rn
 from item, store_sales, date_dim, store
 where ss_item_sk = i_item_sk and
       ss_sold_date_sk = d_date_sk and
       ss_store_sk = s_store_sk and
       (
         d_year = 2001 or
         ( d_year = 2001-1 and d_moy =12) or
         ( d_year = 2001+1 and d_moy =1)
       )
 group by i_category, i_brand,
          s_store_name, s_company_name,
          d_year, d_moy),
 v2 as(
 select v1.i_category, v1.i_brand, v1.s_store_name, v1.s_company_name
        ,v1.d_year
        ,v1.avg_monthly_sales
        ,v1.sum_sales, v1_lag.sum_sales psum, v1_lead.sum_sales nsum
 from v1, v1 v1_lag, v1 v1_lead
 where v1.i_category = v1_lag.i_category and
       v1.i_category = v1_lead.i_category and
       v1.i_brand = v1_lag.i_brand and
       v1.i_brand = v1_lead.i_brand and
       v1.s_store_name = v1_lag.s_store_name and
       v1.s_store_name = v1_lead.s_store_name and
       v1.s_company_name = v1_lag.s_company_name and
       v1.s_company_name = v1_lead.s_company_name and
       v1.rn = v1_lag.rn + 1 and
       v1.rn = v1_lead.rn - 1)
  select  *
 from v2
 where  d_year = 2001 and    
        avg_monthly_sales > 0 and
        case when avg_monthly_sales > 0 then abs(sum_sales - avg_monthly_sales) / avg_monthly_sales else null end > 0.1
 order by sum_sales - avg_monthly_sales, nsum
  LIMIT 100;

@sgrebnov sgrebnov self-assigned this Sep 29, 2024
@sgrebnov sgrebnov changed the title Support unparsing plans with both Aggregation and Window Support unparsing plans with both Aggregation and Window functions Sep 29, 2024
@sgrebnov
Copy link
Author

This is an improvement for the following PR that added Window functions unparsing support:
https://github.com/apache/datafusion/pull/10767/files#diff-b44e5791766f489e13ea0934506db44851a2b2c7f66056280b8bc012b33a28ebL29

There is the following comment. TPCDS queries contain such cases, this PR addressed them
https://github.com/apache/datafusion/pull/10767/files#r1623447634

This assumes a SELECT query can exclusively have only a window function or an aggregate function but not both. A LogicalPlan can certainly have both, but I could not find an example of a single SELECT query without any nesting/derived table factors that is allowed to have both.

@sgrebnov sgrebnov merged commit 1ed3963 into spiceai-42 Sep 30, 2024
@sgrebnov sgrebnov deleted the sgrebnov/improve-agg-unparsing branch September 30, 2024 18:11
sgrebnov added a commit that referenced this pull request Oct 3, 2024
…pache#12705)

* Support unparsing plans with both Aggregation and Window functions (#35)

* Fix unparsing for aggregation grouping sets

* Add test for grouping set unparsing

* Update datafusion/sql/src/unparser/utils.rs

Co-authored-by: Jax Liu <[email protected]>

* Update datafusion/sql/src/unparser/utils.rs

Co-authored-by: Jax Liu <[email protected]>

* Update

* More tests

---------

Co-authored-by: Jax Liu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants