Skip to content

ESQL: Limit memory usage of fold #118602

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

Merged
merged 30 commits into from
Jan 13, 2025
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
8fa64b1
WIP
nik9000 Dec 6, 2024
8ccd678
Merge branch 'main' into fold_ctx_2
nik9000 Dec 10, 2024
a126e84
WIP:
nik9000 Dec 10, 2024
bc661ce
Merge branch 'main' into fold_ctx_2
nik9000 Dec 12, 2024
28ac84f
Fixup
nik9000 Dec 12, 2024
facd394
actual tests
nik9000 Dec 12, 2024
17d2fc4
Update docs/changelog/118602.yaml
nik9000 Dec 12, 2024
faf0e5e
Merge branch 'main' into fold_ctx_2
nik9000 Dec 13, 2024
dd9eeba
Compile again
nik9000 Dec 13, 2024
f46447e
Merge remote-tracking branch 'nik9000/fold_ctx_2' into fold_ctx_2
nik9000 Dec 13, 2024
739e9bf
Merge branch 'main' into fold_ctx_2
nik9000 Dec 27, 2024
6660b88
BigArrays too
nik9000 Dec 27, 2024
b9ea47f
Merge branch 'main' into fold_ctx_2
nik9000 Jan 3, 2025
2e24625
Update x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/…
nik9000 Jan 9, 2025
5a7fe2e
Merge branch 'main' into fold_ctx_2
nik9000 Jan 9, 2025
2d063c8
Merge
nik9000 Jan 9, 2025
115477a
Merge remote-tracking branch 'nik9000/fold_ctx_2' into fold_ctx_2
nik9000 Jan 9, 2025
bfbc7eb
Update foldctx
nik9000 Jan 9, 2025
f3fd791
Review
nik9000 Jan 9, 2025
fa12629
Merge branch 'main' into fold_ctx_2
nik9000 Jan 10, 2025
d790556
Make FoldContext have equality
nik9000 Jan 10, 2025
2abbeef
Helper
nik9000 Jan 10, 2025
39440d8
Catch my bug
nik9000 Jan 10, 2025
0d5afb1
Merge branch 'main' into fold_ctx_2
nik9000 Jan 10, 2025
b20f541
Contextualizification
nik9000 Jan 10, 2025
456833d
5%
nik9000 Jan 13, 2025
573a238
Fix hash
nik9000 Jan 13, 2025
5742c23
Moar tests
nik9000 Jan 13, 2025
b43b8d8
Merge branch 'main' into fold_ctx_2
nik9000 Jan 13, 2025
0651b78
Update heap attack now
nik9000 Jan 13, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Make FoldContext have equality
  • Loading branch information
nik9000 committed Jan 10, 2025
commit d7905562f658b78bf482fbf894ed2f58469970fc
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
import org.elasticsearch.xpack.esql.core.QlClientException;
import org.elasticsearch.xpack.esql.core.tree.Source;

import java.util.Objects;

/**
* Context passed to {@link Expression#fold}. This is not thread safe.
*/
Expand All @@ -33,6 +35,26 @@ public FoldContext(long allowedBytes) {
this.allowedBytes = allowedBytes;
}

long initialAllowedBytes() {
return initialAllowedBytes;
}

long allowedBytes() {
return allowedBytes;
}

@Override
public boolean equals(Object o) {
if (o == null || getClass() != o.getClass()) return false;
FoldContext that = (FoldContext) o;
return initialAllowedBytes == that.initialAllowedBytes && allowedBytes == that.allowedBytes;
}

@Override
public int hashCode() {
return Objects.hash(initialAllowedBytes, allowedBytes);
}

/**
* Track an allocation. Best to call this <strong>before</strong> allocating
* if possible, but after is ok if the allocation is small.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,45 @@

import org.elasticsearch.common.breaker.CircuitBreaker;
import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.test.EqualsHashCodeTestUtils;
import org.elasticsearch.xpack.esql.core.tree.Source;

import static org.hamcrest.Matchers.equalTo;

public class FoldContextTests extends ESTestCase {
public void testEq() {
EqualsHashCodeTestUtils.checkEqualsAndHashCode(randomFoldContext(), this::copy, this::mutate);
}

private FoldContext randomFoldContext() {
FoldContext ctx = new FoldContext(randomNonNegativeLong());
if (randomBoolean()) {
ctx.trackAllocation(Source.EMPTY, randomLongBetween(0, ctx.initialAllowedBytes()));
}
return ctx;
}

private FoldContext copy(FoldContext ctx) {
FoldContext copy = new FoldContext(ctx.initialAllowedBytes());
copy.trackAllocation(Source.EMPTY, ctx.initialAllowedBytes() - ctx.allowedBytes());
return copy;
}

private FoldContext mutate(FoldContext ctx) {
if (randomBoolean()) {
FoldContext differentInitial = new FoldContext(ctx.initialAllowedBytes() + 1);
differentInitial.trackAllocation(Source.EMPTY, differentInitial.initialAllowedBytes() - ctx.allowedBytes());
assertThat(differentInitial.allowedBytes(), equalTo(ctx.allowedBytes()));
return differentInitial;
} else {
FoldContext differentAllowed = new FoldContext(ctx.initialAllowedBytes());
long allowed = randomValueOtherThan(ctx.allowedBytes(), () -> randomLongBetween(0, ctx.initialAllowedBytes()));
differentAllowed.trackAllocation(Source.EMPTY, ctx.initialAllowedBytes() - allowed);
assertThat(differentAllowed.allowedBytes(), equalTo(allowed));
return differentAllowed;
}
}

public void testTrackAllocation() {
FoldContext ctx = new FoldContext(10);
ctx.trackAllocation(Source.synthetic("shouldn't break"), 10);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,7 @@ public boolean foldable() {
* {@code EVAL c=CASE(b, bar, bort)}.
*/
public Expression partiallyFold(FoldContext ctx) {
// TODO don't fold here - look for literal TRUE
// TODO don't throw away the results of any `fold`. That might mean looking for literal TRUE on the conditions.
List<Expression> newChildren = new ArrayList<>(children().size());
boolean modified = false;
for (Condition condition : conditions) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ public ExpressionEvaluator.Factory toEvaluator(ToEvaluator toEvaluator) {
if (children().get(0).foldable()) {
ChronoField chrono = chronoField(toEvaluator.foldCtx());
if (chrono == null) {
BytesRef field = (BytesRef) children().getFirst().fold(toEvaluator.foldCtx());
BytesRef field = (BytesRef) children().get(0).fold(toEvaluator.foldCtx());
throw new InvalidArgumentException("invalid date field for [{}]: {}", sourceText(), field.utf8ToString());
}
return new DateExtractConstantEvaluator.Factory(source(), fieldEvaluator, chrono, configuration().zoneId());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ public boolean equals(Object obj) {
if (obj == this) return true;
if (obj == null || obj.getClass() != this.getClass()) return false;
var that = (LogicalOptimizerContext) obj;
return Objects.equals(this.configuration, that.configuration);
return this.configuration.equals(that.configuration) && this.foldCtx.equals(that.foldCtx);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we update the hashCode implementation, too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. Super important. You should block the merge for a mistake like that. Sorry.

It's not that it's likely to break anything, but it's sort of a bomb set up for someone years in the future.

}

@Override
Expand Down