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

fix(cubesql): Break cost symmetry for (non)-push-to-Cube WrappedSelect #9155

Merged
merged 1 commit into from
Feb 24, 2025

Conversation

mcheshkov
Copy link
Member

@mcheshkov mcheshkov commented Jan 29, 2025

Check List

  • Tests have been run in packages where changes made if available
  • Linter has been run for changed code
  • Tests for the changes have been added if not covered yet
  • Docs have been added / updated if required

Description of Changes Made (if issue reference is not provided)

Break symmetry for pust-to-Cube vs non-push-to-Cube plans, and test pushed members count on simple query.

Consider this query:

SELECT
    CAST(dim_date0 AS DATE) AS "dim_date0"
FROM
    MultiTypeCube
LIMIT 10
;

It can be represented with these two wrapper plans:

image

Without changes in cost both of those would have the same cost:

CubePlanCost {
    replacers: 0,
    table_scans: 0,
    empty_wrappers: 0,
    non_detected_cube_scans: 0,
    unwrapped_subqueries: 0,
    member_errors: 0,
    ungrouped_aggregates: 0,
    non_pushed_down_window: 0,
    non_pushed_down_grouping_sets: 0,
    non_pushed_down_limit_sort: 0,
    joins: 0,
    wrapper_nodes: 1,
    ast_size_outside_wrapper: 0,
    wrapped_select_ungrouped_scan: 1,
    filters: 0,
    structure_points: 0,
    filter_members: 0,
    zero_members_wrapper: 1,
    cube_members: 0,
    errors: 0,
    time_dimensions_used_as_dimensions: 0,
    max_time_dimensions_granularity: 0,
    cube_scan_nodes: 1,
    ast_size_without_alias: 38,
    ast_size: 40,
    ast_size_inside_wrapper: 1,
    ungrouped_nodes: 1,
}

Which one of those will get executed is implementation defined, and right now top-down and bottom-up cost choose different plans.

@mcheshkov mcheshkov force-pushed the test-wrapper-cast-zero-members branch from c0065a7 to 7deae12 Compare January 29, 2025 12:43
@mcheshkov mcheshkov changed the title test(cubesql): Test for explicit members in a simple wrapper with cast test(cubesql): Test for explicit members in a simple wrapper with cast and limit Jan 29, 2025
Copy link

codecov bot commented Jan 29, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.66%. Comparing base (aba6430) to head (0626929).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #9155   +/-   ##
=======================================
  Coverage   83.65%   83.66%           
=======================================
  Files         227      227           
  Lines       81646    81688   +42     
=======================================
+ Hits        68304    68346   +42     
  Misses      13342    13342           
Flag Coverage Δ
cubesql 83.66% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mcheshkov mcheshkov force-pushed the test-wrapper-cast-zero-members branch from 7deae12 to 09b6b59 Compare January 31, 2025 11:36
@mcheshkov mcheshkov changed the title test(cubesql): Test for explicit members in a simple wrapper with cast and limit fix(cubesql): Break cost symmetry for (non)-push-to-Cube WrappedSelect Jan 31, 2025
@mcheshkov mcheshkov force-pushed the test-wrapper-cast-zero-members branch from 09b6b59 to d509852 Compare January 31, 2025 11:36
@mcheshkov mcheshkov marked this pull request as ready for review January 31, 2025 13:45
@mcheshkov mcheshkov requested a review from a team as a code owner January 31, 2025 13:45
@mcheshkov mcheshkov force-pushed the test-wrapper-cast-zero-members branch 2 times, most recently from 443b1d0 to 7befc62 Compare February 24, 2025 14:54
Without this two different representations can have zero members, different push-to-Cube and same cost.
It could be done in a single cost component, like zero_members_wrapper, but would require to have a complex dispatch instead of add_child
@mcheshkov mcheshkov force-pushed the test-wrapper-cast-zero-members branch from 7befc62 to 0626929 Compare February 24, 2025 22:47
@mcheshkov mcheshkov merged commit 2c0e443 into master Feb 24, 2025
83 checks passed
@mcheshkov mcheshkov deleted the test-wrapper-cast-zero-members branch February 24, 2025 23:39
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.

2 participants