fix(dm): support EXPLAIN execution plans - #2771
Conversation
- route DM SQL through the DM command executor - add dedicated DM lexer and parser support - retrieve execution plans through DmdbConnection.getExplainInfo - preserve SQL type, metrics, streaming, and error behavior - cover explicit EXPLAIN, Explain actions, cancellation, and driver isolation Fixes OtterMind#2762
Aias00
left a comment
There was a problem hiding this comment.
Reviewed the DM EXPLAIN support after the main-branch update.
I did not find any blocking code issues in this pass. The DM executor now routes EXPLAIN through getExplainInfo instead of falling back to JDBC PreparedStatement.execute(), buildExplain avoids double-prefixing EXPLAIN, the streaming path publishes the materialized explain plan consistently, and the explicit guardStatement hook covers the non-JDBC-statement execution path.
Local verification already run:
DMCommandExecutorTest: 16 tests, 0 failures/errorsgit diff --check origin/main...HEAD: passed
Remaining risk: I did not run against a real DM database / real DM JDBC driver locally, so that part remains covered by CI or manual integration verification.
|
Thanks for the review. I have now completed manual integration verification against a real DM8 Directly executing I have updated the PR verification section and added the screenshots. |
Aias00
left a comment
There was a problem hiding this comment.
I found one remaining parser issue that is not covered by the manual DM8 verification.
DMSimpleParserVisitor.visitExplain_statement sets the current statement type to EXPLAIN, but then returns super.visitExplain_statement(ctx). That descends into the inner statement and can overwrite the outer type, for example visitSelect_statement sets the same statement to SELECT. The same pattern appears in DMParserVisitor and DMValidTableVisitor.
The DM executor masks this on the execution path by mutating the statement back to EXPLAIN, so the real-DM getExplainInfo verification can still pass. But shared parser consumers such as SqlUtils.parseStatements, syntax parsing, AI tooling, refresh metadata, or validation can still see an explicit EXPLAIN SELECT ... as SELECT instead of EXPLAIN.
Could you preserve the outer EXPLAIN type in these visitors, either by not descending into the inner DML for an EXPLAIN statement or by restoring EXPLAIN after visiting children? Please also add focused parser coverage for explicit EXPLAIN SELECT/UPDATE/DELETE/INSERT/MERGE through the affected parser entry points.
|
Thanks for catching this. I updated all three DM parser visitors to stop descending into the inner DML after setting |
Aias00
left a comment
There was a problem hiding this comment.
I am requesting changes for two verified behavioral issues.
-
The DM EXPLAIN streaming path is not cancellable while
DmdbConnection.getExplainInfo(...)is in flight. The normal path registers itsPreparedStatementthroughstatementListener.onStatementCreated(...), and the web/task cancellation paths callStatement.cancel().DMCommandExecutorbypasses that lifecycle and only checks cancellation before and after the blocking vendor call, so cancel cannot interrupt a long-running EXPLAIN. Please preserve or replace that cancellable-resource contract and add a test that cancels whilegetExplainInfois blocked, ideally through the real streaming caller path. -
Correction to my previous parser comment:
Statement#setTypeis write-once, so descending into the inner DML did not overwrite an already assignedEXPLAINtype. I verified that all 15 newDMSqlParserTestcases already pass at commit44de37696, before the visitor change in6a8950c40. Replacingsuper.visitExplain_statement(ctx)withreturn nullnow prevents the valid-table visitor from collecting inner table and column metadata:EXPLAIN SELECT NAME FROM SYSOBJECTSreturnedSYSOBJECTS/NAMEbefore that change, while the current head returns null table/column maps. Please restore child traversal and add assertions that the outer type remainsEXPLAINwhile inner table/column metadata is preserved.
Current focused verification: DMCommandExecutorTest and DMSqlParserTest, 31 tests, 0 failures/errors; git diff --check also passes. The requested changes concern behavior that the current tests do not cover.
Related issue
Closes #2762
Summary
Add DM-specific EXPLAIN execution support so execution plans are retrieved through the DM JDBC driver's
DmdbConnection.getExplainInfoAPI instead of the default JDBC execution path.Key changes:
DMCommandExecutor.DefaultSQLExecutor.EXPLAIN <SQL>statements and the Explain action.EXPLAIN EXPLAIN <SQL>statements.Affected surfaces
Verification
DMCommandExecutorTest: 16 tests, 0 failures/errors.git diff --check origin/main...HEAD: passed.using the DM JDBC driver.
Manual verification results:
EXPLAIN SELECT ...returned the actual DM execution plan(
#NSET2,#PRJT2,#BLKUP2, and#SSEK2) instead of an update count.INSERT,SELECT, andUPDATEstatements executed normally.ID = 1andNAME = DM Explain OK.UI evidence
DM EXPLAIN result
Regular DM SQL result
Risk and compatibility
DmdbConnection.getExplainInfo(String). Unsupported drivers return an explicit SQL error instead of falling back to theJDBC EXPLAIN path.
Reviewer map
DMCommandExecutorfor SQL routing and EXPLAIN result handling.DMExplainClientfor connection unwrapping and vendor API invocation.DMSqlParserand the DM ANTLR grammar for EXPLAIN recognition.DMMetaDataandDMSqlBuilderfor executor registration and SQL construction.DMCommandExecutorTestfor expected behavior and regression coverage.getExplainInfo.Contributor declaration
AI assistance: AI assistance was used to draft and organize this PR description.