Skip to content

Add configs to control aggregate/Window/Orderby Spilling#16581

Merged
rschlussel merged 1 commit intoprestodb:masterfrom
pgupta2:add_config_aggregate_window_orderby_spill
Aug 12, 2021
Merged

Add configs to control aggregate/Window/Orderby Spilling#16581
rschlussel merged 1 commit intoprestodb:masterfrom
pgupta2:add_config_aggregate_window_orderby_spill

Conversation

@pgupta2
Copy link
Contributor

@pgupta2 pgupta2 commented Aug 7, 2021

spill_enabled session property enable spills for all operators. Currently, there is
no way to control aggregation/Window/Orderby spilling explicitly. This PR adds configs to
control these 3 operators spilling.

Test plan - Unit tests

== RELEASE NOTES ==

General Changes
* Add a new configuration property ``experimental.aggregation-spill-enabled`` to control aggregate spills explicitly. This can be overridden by ``aggregation_spill_enabled`` session property.
* Add a new configuration property ``experimental.window-spill-enabled`` to control window spills explicitly. This can be overridden by ``window_spill_enabled`` session property.
* Add a new configuration property ``experimental.order-by-spill-enabled`` to control order by spills explicitly. This can be overridden by ``order_by_spill_enabled`` session property.

@pgupta2 pgupta2 requested a review from a team August 8, 2021 00:26
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change this and orderby to check isAggregationSpillEnabled() instead of isSpillEnabled(). (or alternatively, just remove the isSpillEnabled check for both since we check all three properties anyway when build the aggregation operator)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't need to pass in spillEnabled anymore since aggregationSpillEnabled includes a check for spillEnabled.

Copy link
Contributor

@rschlussel rschlussel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also add tests for window and order by in TestSpilledWindowQueries and TestSpilledOrderByQueries

@rschlussel
Copy link
Contributor

@pgupta2 pgupta2 force-pushed the add_config_aggregate_window_orderby_spill branch from ddb0bd9 to e028381 Compare August 9, 2021 21:16
`spill_enabled` session property enable spills for all operators. Currently, there is
no way to control aggergation/Window/Orderby spilling explicitly. This PR adds configs to
control these 3 operators spilling.
@pgupta2 pgupta2 force-pushed the add_config_aggregate_window_orderby_spill branch from e028381 to 1fe34c4 Compare August 9, 2021 21:24
@pgupta2
Copy link
Contributor Author

pgupta2 commented Aug 10, 2021

@rschlussel : Addressed all the comments.

Copy link
Contributor

@rschlussel rschlussel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a minor comment

return planGroupByAggregation(
node,
source,
spillEnabled,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can remove the spillEnabled variable now since its unused

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rschlussel : This variable is already removed. We now only pass isAggregationSpillEnabled flag along with isDistinctAggregationSpillEnabled and isOrderByAggregationSpillEnabled.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry got it confused due to collapsed code in github. Looks good.

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