-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat: add AstMemoryEstimator for SQL AST memory estimation #16882
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
Conversation
JackieTien97
left a comment
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.
IMO, making Node implements Accountable interface may be the best way.
| } | ||
|
|
||
| private void addNodeSize(Node node) { | ||
| totalMemorySize += RamUsageEstimator.shallowSizeOfInstance(node.getClass()); |
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.
Node's InstanceSize should be a constant, no need to calculate each time.
| import java.util.Optional; | ||
|
|
||
| /** Utility class for estimating memory usage of AST nodes. */ | ||
| public final class AstMemoryEstimator { |
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.
It's hard to maintain this class, if any Node has some changes or new Node being added.
…are-ast # Conflicts: # iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/relational/sql/ast/LoadTsFile.java
...ava/org/apache/iotdb/db/schemaengine/schemaregion/read/resp/info/impl/ShowDevicesResult.java
Outdated
Show resolved
Hide resolved
...ava/org/apache/iotdb/db/schemaengine/schemaregion/read/resp/info/impl/ShowDevicesResult.java
Outdated
Show resolved
Hide resolved
.../java/org/apache/iotdb/db/queryengine/plan/relational/sql/ast/AstMemoryEstimationHelper.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/apache/iotdb/db/queryengine/plan/relational/sql/ast/ColumnDefinition.java
Outdated
Show resolved
Hide resolved
...e/src/main/java/org/apache/iotdb/db/queryengine/plan/relational/sql/ast/AliasedRelation.java
Outdated
Show resolved
Hide resolved
...node/src/main/java/org/apache/iotdb/db/queryengine/plan/relational/sql/ast/SingleColumn.java
Outdated
Show resolved
Hide resolved
...ore/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/relational/sql/ast/Trim.java
Outdated
Show resolved
Hide resolved
...anode/src/main/java/org/apache/iotdb/db/queryengine/plan/relational/sql/ast/UnloadModel.java
Outdated
Show resolved
Hide resolved
...e/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/relational/sql/ast/Update.java
Outdated
Show resolved
Hide resolved
...atanode/src/main/java/org/apache/iotdb/db/queryengine/plan/relational/sql/ast/WithQuery.java
Outdated
Show resolved
Hide resolved
JackieTien97
left a comment
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.
Remember to review the codes by yourself if the codes are mostly generated by LLM.
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.
Pull request overview
This PR adds memory estimation capabilities for SQL AST (Abstract Syntax Tree) nodes by implementing the Accountable interface across the codebase. The feature enables tracking memory usage of parsed SQL statements through the ramBytesUsed() method, which is useful for memory management and resource planning.
Key Changes:
- Implements
Accountableinterface on the baseNodeandJoinCriteriaclasses - Adds
ramBytesUsed()method implementations to 100+ AST node classes - Introduces comprehensive test suite with 80+ test cases covering various SQL constructs
- Adds
getLocationInternal()accessor method toNodeclass for memory estimation
Reviewed changes
Copilot reviewed 207 out of 207 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| AstMemoryEstimatorTest.java | New comprehensive test suite with 80+ tests covering literals, expressions, queries, and complex SQL statements |
| Node.java | Base class updated to implement Accountable and add getLocationInternal() method |
| JoinCriteria.java | Updated to implement Accountable interface |
| QualifiedName.java | Implements Accountable with memory estimation logic for qualified names |
| ShowDevicesResult.java | Adds Accountable implementation for schema result objects |
| 100+ AST classes | Each implements ramBytesUsed() using consistent pattern: INSTANCE_SIZE + field sizes |
| AbstractTraverseDevice.java | Adds getter method for attributeColumns field |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…rectly call the methods of RamUsageEstimator
…are-ast # Conflicts: # pom.xml
No description provided.