Skip to content

Use config default for determine_partition_count_for_write_enabled#17769

Merged
sopel39 merged 1 commit intotrinodb:masterfrom
starburstdata:ks/use_def
Jun 6, 2023
Merged

Use config default for determine_partition_count_for_write_enabled#17769
sopel39 merged 1 commit intotrinodb:masterfrom
starburstdata:ks/use_def

Conversation

@sopel39
Copy link
Copy Markdown
Member

@sopel39 sopel39 commented Jun 6, 2023

No description provided.

@sopel39 sopel39 added the no-release-notes This pull request does not require release notes entry label Jun 6, 2023
@sopel39 sopel39 requested review from losipiuk and raunaqmorarka June 6, 2023 11:02
@cla-bot cla-bot bot added the cla-signed label Jun 6, 2023
Copy link
Copy Markdown
Member

@raunaqmorarka raunaqmorarka left a comment

Choose a reason for hiding this comment

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

Is query.determine-partition-count-for-write-enabled in the right place at QueryManagerConfig ?
It looks like this should be in OptimizerConfig.
Why is it disabled by default given that DeterminePartitionCount is enabled by default for non-write cases ?

DETERMINE_PARTITION_COUNT_FOR_WRITE_ENABLED,
"Determine the number of partitions based on amount of data read and processed by the query for write queries",
false,
queryManagerConfig.isDeterminePartitionCountForWriteEnabled(),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think there should be a test that verifies changes to plan based on usage of this session property.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

possibly, but in this PR I'm just plugging correct property so that users have session properties consistent with feature config

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@raunaqmorarka : turning it on might cause OOM regressions related to partition writes. We do have it enabled for FTE internally while keeping lower bound of partitions to be 50.

@raunaqmorarka raunaqmorarka requested a review from linzebing June 6, 2023 11:18
@sopel39
Copy link
Copy Markdown
Member Author

sopel39 commented Jun 6, 2023

Why is it disabled by default given that DeterminePartitionCount is enabled by default for non-write cases ?

One of the reasons is distributing partition writers across more drivers within cluster so that memory limit is not breached. DeterminePartitionCount doesn't look at NDVs

@sopel39 sopel39 requested a review from raunaqmorarka June 6, 2023 11:28
@sopel39 sopel39 merged commit 3298d6d into trinodb:master Jun 6, 2023
@sopel39 sopel39 deleted the ks/use_def branch June 6, 2023 14:55
@github-actions github-actions bot added this to the 420 milestone Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed no-release-notes This pull request does not require release notes entry

Development

Successfully merging this pull request may close these issues.

4 participants