-
Notifications
You must be signed in to change notification settings - Fork 25.6k
ESQL: Make intermediate LocalRelation limit configurable #135339
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,7 +19,7 @@ | |
| /** | ||
| * Values for cluster level settings used in physical planning. | ||
| */ | ||
| public class PhysicalSettings { | ||
| public class PlannerSettings { | ||
| public static final Setting<DataPartitioning> DEFAULT_DATA_PARTITIONING = Setting.enumSetting( | ||
| DataPartitioning.class, | ||
| "esql.default_data_partitioning", | ||
|
|
@@ -45,26 +45,42 @@ public class PhysicalSettings { | |
| Setting.Property.Dynamic | ||
| ); | ||
|
|
||
| public static final Setting<ByteSizeValue> INTERMEDIATE_LOCAL_RELATION_MAX_SIZE = Setting.memorySizeSetting( | ||
| "esql.intermediate_local_relation_max_size", | ||
| "0.1%", | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On a 512mb heap this is half a megabyte. That feels fine. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeh.. I dropped the min/max bounds for the limit, since I think this proportionality should be working by itself. But can add them back if it proves it'll be useful. |
||
| Setting.Property.NodeScope, | ||
| Setting.Property.Dynamic | ||
| ); | ||
|
|
||
| private volatile DataPartitioning defaultDataPartitioning; | ||
| private volatile ByteSizeValue valuesLoadingJumboSize; | ||
| private volatile int luceneTopNLimit; | ||
| private volatile ByteSizeValue intermediateLocalRelationMaxSize; | ||
|
|
||
| /** | ||
| * Ctor for prod that listens for updates from the {@link ClusterService}. | ||
| */ | ||
| public PhysicalSettings(ClusterService clusterService) { | ||
| clusterService.getClusterSettings().initializeAndWatch(DEFAULT_DATA_PARTITIONING, v -> this.defaultDataPartitioning = v); | ||
| clusterService.getClusterSettings().initializeAndWatch(VALUES_LOADING_JUMBO_SIZE, v -> this.valuesLoadingJumboSize = v); | ||
| clusterService.getClusterSettings().initializeAndWatch(LUCENE_TOPN_LIMIT, v -> this.luceneTopNLimit = v); | ||
| public PlannerSettings(ClusterService clusterService) { | ||
| var clusterSettings = clusterService.getClusterSettings(); | ||
| clusterSettings.initializeAndWatch(DEFAULT_DATA_PARTITIONING, v -> this.defaultDataPartitioning = v); | ||
| clusterSettings.initializeAndWatch(VALUES_LOADING_JUMBO_SIZE, v -> this.valuesLoadingJumboSize = v); | ||
| clusterSettings.initializeAndWatch(LUCENE_TOPN_LIMIT, v -> this.luceneTopNLimit = v); | ||
| clusterSettings.initializeAndWatch(INTERMEDIATE_LOCAL_RELATION_MAX_SIZE, v -> this.intermediateLocalRelationMaxSize = v); | ||
| } | ||
|
|
||
| /** | ||
| * Ctor for testing. | ||
| */ | ||
| public PhysicalSettings(DataPartitioning defaultDataPartitioning, ByteSizeValue valuesLoadingJumboSize, int luceneTopNLimit) { | ||
| public PlannerSettings( | ||
| DataPartitioning defaultDataPartitioning, | ||
| ByteSizeValue valuesLoadingJumboSize, | ||
| int luceneTopNLimit, | ||
| ByteSizeValue intermediateLocalRelationMaxSize | ||
| ) { | ||
| this.defaultDataPartitioning = defaultDataPartitioning; | ||
| this.valuesLoadingJumboSize = valuesLoadingJumboSize; | ||
| this.luceneTopNLimit = luceneTopNLimit; | ||
| this.intermediateLocalRelationMaxSize = intermediateLocalRelationMaxSize; | ||
| } | ||
|
|
||
| public DataPartitioning defaultDataPartitioning() { | ||
|
|
@@ -92,4 +108,8 @@ public ByteSizeValue valuesLoadingJumboSize() { | |
| public int luceneTopNLimit() { | ||
| return luceneTopNLimit; | ||
| } | ||
|
|
||
| public ByteSizeValue intermediateLocalRelationMaxSize() { | ||
| return intermediateLocalRelationMaxSize; | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah. I was expecting a
LogicalSettingson day. but this is just as good.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was a bit hesitant about this rename, but I thought it might make sense since there's no hard distinguishing between them so far and even the new one is actually using while executing (subplans). But yes, then can be untangled if needed.