github-actions[bot] commented on code in PR #66817:
URL: https://github.com/apache/doris/pull/66817#discussion_r3793619525
##########
fe/fe-sql-parser/src/main/antlr4/org/apache/doris/nereids/DorisParser.g4:
##########
@@ -1186,11 +1186,7 @@ planType
;
replayCommand
- : PLAN REPLAYER replayType;
-
-replayType
- : DUMP query
- | PLAY filePath=STRING_LITERAL;
+ : PLAN REPLAYER DUMP query;
Review Comment:
[P2] Preserve the public DUMP parse-tree hook
`fe-sql-parser` is a standalone library whose README explicitly tells
downstream users to subclass the generated visitors/listeners, with each
grammar rule serving as an override point. Flattening this rule removes
`ReplayTypeContext` and `visitReplayType`/listener hooks even for the
still-supported `PLAN REPLAYER DUMP`, so a DUMP-only extension using that
documented hook no longer compiles against the new jar (and binaries resolving
the removed symbols fail linkage). Rejecting PLAY does not require that
collateral API break; please keep `replayCommand : PLAN REPLAYER replayType`
and reduce `replayType` to `DUMP query`.
##########
fe/fe-core/src/test/java/org/apache/doris/nereids/parser/NereidsParserTest.java:
##########
@@ -392,13 +392,9 @@ public void testPlanReplayer() {
String sql = "plan replayer dump select `AD``D` from t1 where a = 1";
NereidsParser nereidsParser = new NereidsParser();
LogicalPlan logicalPlan = nereidsParser.parseSingle(sql);
- ReplayCommand replayCommand = (ReplayCommand) logicalPlan;
- Assertions.assertEquals(ReplayCommand.ReplayType.DUMP,
replayCommand.getReplayType());
- sql = "plan replayer play 'path'";
- logicalPlan = nereidsParser.parseSingle(sql);
- replayCommand = (ReplayCommand) logicalPlan;
- Assertions.assertEquals(ReplayCommand.ReplayType.PLAY,
replayCommand.getReplayType());
- Assertions.assertEquals("path", replayCommand.getDumpFileFullPath());
+ Assertions.assertInstanceOf(ReplayCommand.class, logicalPlan);
+ Assertions.assertThrows(ParseException.class,
+ () -> nereidsParser.parseSingle("plan replayer play 'path'"));
Review Comment:
[P2] Cover PLAY's new identifier behavior
This assertion only proves that the old command no longer parses; it would
also pass if `PLAY` stayed reserved after deleting the command alternative. The
other user-visible part of this change is that every case variant of `play`
becomes an ordinary unquoted identifier, but there is no positive case for that
behavior. Please add a parse assertion such as `SELECT pLaY FROM play` (or the
equivalent identifier-validation check) so this compatibility change is
actually protected.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/minidump/Minidump.java:
##########
@@ -110,35 +105,4 @@ public Map<String, ColumnStatistic>
getTotalColumnStatisticMap() {
public Map<String, Histogram> getTotalHistogramMap() {
return totalHistogramMap;
}
-
- /** Nereids minidump entry, argument should be absolute address of
minidump path */
- public static void main(String[] args) {
Review Comment:
[P2] Retire the packaged runner with its Java entry point
`build.sh:1167` still copies the `minidump/` directory into every FE
package, the bundled README instructs developers to run `nereids_ut.sh`, and
that script invokes this exact class. Once this `main` is deleted, every
documented invocation exits with Java's missing-main error before processing a
dump. If the offline replay flow is intentionally being removed, please retire
or update the runner, README, and packaging in this PR (or point it at a
replacement entry point), rather than shipping a command that can no longer
start.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
