Conversation
Signed-off-by: Weihao Li <18110526956@163.com>
Signed-off-by: Weihao Li <18110526956@163.com>
Signed-off-by: Weihao Li <18110526956@163.com> # Conflicts: # iotdb-core/datanode/src/main/java/org/apache/iotdb/db/protocol/thrift/impl/ClientRPCServiceImpl.java
Signed-off-by: Weihao Li <18110526956@163.com>
There was a problem hiding this comment.
Pull request overview
This pull request adds support for the DEBUG keyword in SQL queries for both tree model and table model. When a query is prefixed with the DEBUG keyword, it enables detailed debug logging throughout the query execution pipeline, which is useful for troubleshooting and understanding query behavior.
Changes:
- Added DEBUG keyword to ANTLR grammar files for both tree and table models
- Threaded debug flag through the entire query execution pipeline (Statement, QueryContext, MPPQueryContext, FragmentInstance, FragmentInstanceContext)
- Updated all QueryContext constructor calls to include the debug parameter
- Added integration tests to verify DEBUG functionality for both tree and table model queries
- Added helper methods to AbstractNodeWrapper to clear and check log contents
Reviewed changes
Copilot reviewed 42 out of 42 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| RelationalSql.g4 | Added DEBUG token to grammar and included it in nonReserved keywords |
| Statement.java (relational) | Added debug field and getter/setter methods |
| Statement.java (tree model) | Already had debug support - no changes needed |
| AstBuilder.java | Extracts debug flag from parse tree and sets it on Statement |
| QueryContext.java | Made debug field final and added it to all constructors |
| MPPQueryContext.java | Added debug field with getter/setter methods |
| FragmentInstance.java | Added debug field, updated constructors, and implemented serialization/deserialization |
| FragmentInstanceContext.java | Updated all constructors to accept and propagate debug flag |
| Coordinator.java | Updated execution methods with debug parameter and added deprecated compatibility methods |
| Test files | Updated all QueryContext instantiations to pass false for debug flag |
| IoTDBDebugQueryIT.java | Added integration tests for DEBUG keyword functionality |
| AbstractNodeWrapper.java | Added clearLogContent() and logContains() helper methods for log verification |
| Various other files | Updated calls to pass debug flag through the execution pipeline |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @Test | ||
| public void tableTest() throws IOException { | ||
| // clear log content to reduce lines spanned in logContains check | ||
| dataNodeWrapper.clearLogContent(); | ||
|
|
||
| String[] expectedHeader = | ||
| new String[] { | ||
| "Time", "root.test.departments.department_id", "root.test.departments.dep_name" | ||
| }; | ||
| String[] retArray = new String[] {"1,D001,研发部,"}; | ||
| resultSetEqualTest( | ||
| "debug select department_id, dep_name from root.test.departments", | ||
| expectedHeader, | ||
| retArray); | ||
|
|
||
| assertTrue(dataNodeWrapper.logContains("Cache miss: root.test.departments")); | ||
| } |
There was a problem hiding this comment.
Test method name is misleading. The method is named "tableTest" but it's actually testing the tree model query with "debug select department_id, dep_name from root.test.departments". Consider renaming to "treeTest" for clarity.
| if (userQuery) { | ||
| System.out.println("--------------" + debug); |
There was a problem hiding this comment.
Debug print statement should be removed before merging to production. This System.out.println was likely added for development testing but should not be part of the final code.
| if (userQuery) { | |
| System.out.println("--------------" + debug); | |
| if (userQuery && LOGGER.isDebugEnabled()) { | |
| LOGGER.debug("[QueryDebugFlag] debug: {}", debug); |
| @Test | ||
| public void treeTest() throws IOException { | ||
| // clear log content to reduce lines spanned in logContains check | ||
| dataNodeWrapper.clearLogContent(); | ||
|
|
||
| String[] expectedHeader = new String[] {"time", "device", "value"}; | ||
| String[] retArray = new String[] {"2020-01-01T00:00:01.000Z,d1,1,"}; | ||
| tableResultSetEqualTest( | ||
| "debug select time,device,value from table1", expectedHeader, retArray, DATABASE_NAME); | ||
|
|
||
| assertTrue(dataNodeWrapper.logContains("Cache miss: table1.d1")); | ||
| } |
There was a problem hiding this comment.
Test method name is misleading. The method is named "treeTest" but it's actually testing the table model query with "debug select time,device,value from table1". Consider renaming to "tableTest" for clarity.
Signed-off-by: Weihao Li <18110526956@163.com>
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #17178 +/- ##
============================================
- Coverage 39.50% 39.50% -0.01%
Complexity 282 282
============================================
Files 5101 5101
Lines 341767 341817 +50
Branches 43555 43556 +1
============================================
+ Hits 135009 135027 +18
- Misses 206758 206790 +32 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
No description provided.