Skip to content

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 <liugs963@gmail.com>

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

Co-authored-by: Jax Liu <liugs963@gmail.com>

* Update

* More tests

---------

Co-authored-by: Jax Liu <liugs963@gmail.com>
kczimm pushed a commit that referenced this pull request Aug 19, 2025
* dissallow pushdown of volatile PhysicalExprs

* fix

* add FilteredVec helper to handle filter / remap pattern (#34)

* checkpoint: Address PR feedback in https://github.com/apach...

* add FilteredVec to consolidate handling of filter / remap pattern

* lint

* Add slt test for pushing volatile predicates down (#35)

---------

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
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