Skip to content

Cache field metadata during checking and planning #913

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.

Sign up for GitHub

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
snazy merged 1 commit into from
Jul 17, 2026
Merged

Cache field metadata during checking and planning #913

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 15 additions & 1 deletion core/src/main/java/org/projectnessie/cel/checker/Checker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ public final class Checker {
private final SourceInfo sourceInfo;
private final Map<Long, Type> types = new HashMap<>();
private final Map<Long, Reference> references = new HashMap<>();
private final Map<String, FieldType> fieldTypes = new HashMap<>();

private Checker(
CheckerEnv env,
Expand Down Expand Up @@ -643,7 +644,7 @@ FieldType lookupFieldType(Location l, String messageType, String fieldName) {
return null;
}

FieldType ft = env.provider.findFieldType(messageType, fieldName);
FieldType ft = findFieldType(messageType, fieldName);
if (ft != null) {
return ft;
}
Expand All @@ -652,6 +653,19 @@ FieldType lookupFieldType(Location l, String messageType, String fieldName) {
return null;
}

private FieldType findFieldType(String messageType, String fieldName) {
String key = messageType + '\n' + fieldName;
FieldType ft = fieldTypes.get(key);
if (ft != null) {
return ft;
}
ft = env.provider.findFieldType(messageType, fieldName);
if (ft != null) {
fieldTypes.put(key, ft);
}
return ft;
}

void setType(Expr.Builder e, Type t) {
Type old = types.get(e.getId());
if (old != null && !old.equals(t)) {
Expand Down
22 changes: 20 additions & 2 deletions core/src/main/java/org/projectnessie/cel/interpreter/InterpretablePlanner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ final class Planner implements InterpretablePlanner {
private final Container container;
private final Map<Long, Reference> refMap;
private final Map<Long, Type> typeMap;
private final Map<String, FieldType> fieldTypes = new HashMap<>();
private final InterpretableDecorator[] decorators;

Planner(
Expand Down Expand Up @@ -249,7 +250,7 @@ Interpretable planSelect(Expr expr) {
FieldType fieldType = null;
Type opType = typeMap.get(sel.getOperand().getId());
if (opType != null && !opType.getMessageType().isEmpty()) {
FieldType ft = provider.findFieldType(opType.getMessageType(), sel.getField());
FieldType ft = findFieldType(opType.getMessageType(), sel.getField());

@XN137 XN137 Jul 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i assume its not worth caching negative lookup results?
(i have no idea how frequently this gets called and how expensive provider.findFieldType is)

@snazy snazy Jul 17, 2026

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.

Not really, no

if (ft != null && ft.isSet != null && ft.getFrom != null) {
fieldType = ft;
}
Expand All @@ -273,7 +274,11 @@ Interpretable planSelect(Expr expr) {
return new EvalTestOnly(expr.getId(), op, stringOf(sel.getField()), fieldType);
}
// Build a qualifier.
Qualifier qual = attrFactory.newQualifier(opType, expr.getId(), sel.getField());
Qualifier qual =
fieldType != null
? new AttributeFactory.FieldQualifier(
expr.getId(), sel.getField(), fieldType, adapter)
: attrFactory.newQualifier(opType, expr.getId(), sel.getField());

@XN137 XN137 Jul 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i assume this change avoids a redundant provider.findFieldType call in AttrFactory.newQualifier ?
is the field lookup that expensive to be worth kind of breaking the factory abstraction?

@snazy snazy Jul 17, 2026

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.

It's a bit of repeated work, yea.

if (qual == null) {
return null;
}
Expand All @@ -292,6 +297,19 @@ Interpretable planSelect(Expr expr) {
return relAttr;
}

private FieldType findFieldType(String messageType, String fieldName) {
String key = messageType + '\n' + fieldName;
FieldType ft = fieldTypes.get(key);
if (ft != null) {
return ft;
}
ft = provider.findFieldType(messageType, fieldName);
if (ft != null) {
fieldTypes.put(key, ft);
}
return ft;
}

/**
* planCall creates a callable Interpretable while specializing for common functions and
* invocation patterns. Specifically, conditional operators &&, ||, ?:, and (in)equality
Expand Down
Loading