Add a direct sub classes data structure to the Painless lookup (#76955)#9
Open
MitchLewis930 wants to merge 1 commit intopr_019_beforefrom
Open
Add a direct sub classes data structure to the Painless lookup (#76955)#9MitchLewis930 wants to merge 1 commit intopr_019_beforefrom
MitchLewis930 wants to merge 1 commit intopr_019_beforefrom
Conversation
…ic#76955) This change has two main components. The first is to have method/field resolution for compile-time and run-time use the same code path for now. This removes copying of member methods between super and sub classes and instead does a resolution through the class hierarchy. This allows us to correctly implement the next change. The second is a data structure that allows for the lookup of direct sub classes for all allow listed classes/interfaces within Painless.
📝 WalkthroughWalkthroughThis change introduces explicit tracking of direct subclass relationships in the Painless lookup system through a new field and corresponding getter. The builder now constructs class hierarchies during initialization. Lookup methods are refactored to use generalized traversal logic with improved breadth-first interface traversal and cycle detection. Changes
Sequence DiagramsequenceDiagram
participant Caller
participant PainlessLookup
participant ClassHierarchy as Hierarchy Map
participant Queue as Interface Queue
Caller->>PainlessLookup: lookupPainlessObject(targetClass)
PainlessLookup->>ClassHierarchy: Check targetClass mapping
alt Class Found
ClassHierarchy-->>PainlessLookup: Return match
else Class Not Found
PainlessLookup->>PainlessLookup: Check if interface?
opt Is Interface
PainlessLookup->>PainlessLookup: lookup on Object.class
end
PainlessLookup->>Queue: Initialize with getInterfaces()
PainlessLookup->>PainlessLookup: Create resolvedInterfaces set
loop While queue not empty
Queue->>PainlessLookup: Next interface
PainlessLookup->>ClassHierarchy: lookup on interface
alt Match found
ClassHierarchy-->>Caller: Return result
else No match
PainlessLookup->>Queue: Add parent interfaces (if not in resolvedInterfaces)
end
end
PainlessLookup->>PainlessLookup: Walk superclass chain
PainlessLookup->>ClassHierarchy: lookup on superclass
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@modules/lang-painless/src/main/java/org/elasticsearch/painless/lookup/PainlessLookup.java`:
- Around line 289-304: The traversal skips enqueuing parent interfaces when a
visited interface isn't allow‑listed because targetInterface.getInterfaces() is
only added inside the "if (targetPainlessClass != null)" block; modify the loop
in PainlessLookup so that after a targetInterface is marked visited
(resolvedInterfaces.add) you always enqueue its super-interfaces (call
targetInterfaces.addAll(Arrays.asList(targetInterface.getInterfaces())))
regardless of whether classesToPainlessClasses.containsKey(targetInterface) or
objectLookup returned null, preserving the existing checks for painlessObject
via objectLookup and avoiding duplicate visits via resolvedInterfaces.
🧹 Nitpick comments (1)
modules/lang-painless/src/main/java/org/elasticsearch/painless/lookup/PainlessLookup.java (1)
86-88: Return an unmodifiable copy fromgetDirectSubClassesto prevent external mutation.The current getter exposes internal mutable sets. A defensive copy keeps lookup state immutable and avoids accidental corruption from callers.
♻️ Suggested change
public Set<Class<?>> getDirectSubClasses(Class<?> superClass) { - return classesToDirectSubClasses.get(superClass); + Set<Class<?>> direct = classesToDirectSubClasses.get(superClass); + return direct == null ? null : Set.copyOf(direct); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
modules/lang-painless/src/main/java/org/elasticsearch/painless/lookup/PainlessLookup.javamodules/lang-painless/src/main/java/org/elasticsearch/painless/lookup/PainlessLookupBuilder.javamodules/lang-painless/src/test/java/org/elasticsearch/painless/LookupTests.javamodules/lang-painless/src/test/resources/org/elasticsearch/painless/org.elasticsearch.painless.lookup
🧰 Additional context used
🧬 Code graph analysis (1)
modules/lang-painless/src/test/java/org/elasticsearch/painless/LookupTests.java (2)
modules/lang-painless/src/main/java/org/elasticsearch/painless/lookup/PainlessLookup.java (1)
PainlessLookup(27-313)modules/lang-painless/src/main/java/org/elasticsearch/painless/lookup/PainlessLookupBuilder.java (1)
PainlessLookupBuilder(73-1562)
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Comment on lines
+289
to
304
|
|
||
| while (targetInterfaces.isEmpty() == false) { | ||
| Class<?> targetInterface = targetInterfaces.remove(0); | ||
|
|
||
| if (resolvedInterfaces.add(targetInterface)) { | ||
| PainlessClass targetPainlessClass = classesToPainlessClasses.get(targetInterface); | ||
|
|
||
| if (targetPainlessClass != null) { | ||
| T painlessObject = objectLookup.apply(targetPainlessClass); | ||
|
|
||
| if (targetPainlessClass != null) { | ||
| T painlessObject = objectLookup.apply(targetPainlessClass); | ||
| if (painlessObject != null) { | ||
| return painlessObject; | ||
| } | ||
|
|
||
| if (painlessObject != null) { | ||
| return painlessObject; | ||
| targetInterfaces.addAll(Arrays.asList(targetInterface.getInterfaces())); | ||
| } |
There was a problem hiding this comment.
Ensure interface traversal reaches allow-listed ancestors even through non-allow-listed interfaces.
Currently, parent interfaces are only enqueued when the current interface is allow‑listed. If a class implements a non‑allow‑listed interface that extends an allow‑listed one, lookups can miss valid members. This is a correctness regression in hierarchy resolution.
🔧 Proposed fix
- if (resolvedInterfaces.add(targetInterface)) {
- PainlessClass targetPainlessClass = classesToPainlessClasses.get(targetInterface);
-
- if (targetPainlessClass != null) {
- T painlessObject = objectLookup.apply(targetPainlessClass);
-
- if (painlessObject != null) {
- return painlessObject;
- }
-
- targetInterfaces.addAll(Arrays.asList(targetInterface.getInterfaces()));
- }
- }
+ if (resolvedInterfaces.add(targetInterface)) {
+ PainlessClass targetPainlessClass = classesToPainlessClasses.get(targetInterface);
+
+ if (targetPainlessClass != null) {
+ T painlessObject = objectLookup.apply(targetPainlessClass);
+ if (painlessObject != null) {
+ return painlessObject;
+ }
+ }
+ // Always traverse parent interfaces, even when this interface isn't allow-listed.
+ targetInterfaces.addAll(Arrays.asList(targetInterface.getInterfaces()));
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| while (targetInterfaces.isEmpty() == false) { | |
| Class<?> targetInterface = targetInterfaces.remove(0); | |
| if (resolvedInterfaces.add(targetInterface)) { | |
| PainlessClass targetPainlessClass = classesToPainlessClasses.get(targetInterface); | |
| if (targetPainlessClass != null) { | |
| T painlessObject = objectLookup.apply(targetPainlessClass); | |
| if (targetPainlessClass != null) { | |
| T painlessObject = objectLookup.apply(targetPainlessClass); | |
| if (painlessObject != null) { | |
| return painlessObject; | |
| } | |
| if (painlessObject != null) { | |
| return painlessObject; | |
| targetInterfaces.addAll(Arrays.asList(targetInterface.getInterfaces())); | |
| } | |
| while (targetInterfaces.isEmpty() == false) { | |
| Class<?> targetInterface = targetInterfaces.remove(0); | |
| if (resolvedInterfaces.add(targetInterface)) { | |
| PainlessClass targetPainlessClass = classesToPainlessClasses.get(targetInterface); | |
| if (targetPainlessClass != null) { | |
| T painlessObject = objectLookup.apply(targetPainlessClass); | |
| if (painlessObject != null) { | |
| return painlessObject; | |
| } | |
| } | |
| // Always traverse parent interfaces, even when this interface isn't allow-listed. | |
| targetInterfaces.addAll(Arrays.asList(targetInterface.getInterfaces())); | |
| } |
🤖 Prompt for AI Agents
In
`@modules/lang-painless/src/main/java/org/elasticsearch/painless/lookup/PainlessLookup.java`
around lines 289 - 304, The traversal skips enqueuing parent interfaces when a
visited interface isn't allow‑listed because targetInterface.getInterfaces() is
only added inside the "if (targetPainlessClass != null)" block; modify the loop
in PainlessLookup so that after a targetInterface is marked visited
(resolvedInterfaces.add) you always enqueue its super-interfaces (call
targetInterfaces.addAll(Arrays.asList(targetInterface.getInterfaces())))
regardless of whether classesToPainlessClasses.containsKey(targetInterface) or
objectLookup returned null, preserving the existing checks for painlessObject
via objectLookup and avoiding duplicate visits via resolvedInterfaces.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR_019
Summary by CodeRabbit
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.