Re: [PR] IGNITE-25092 Sql. Running queries view should return query string in valid format [ignite-3]
AMashenkov commented on code in PR #5603:
URL: https://github.com/apache/ignite-3/pull/5603#discussion_r2041939923
##
modules/sql-engine/src/main/codegen/includes/parserImpls.ftl:
##
@@ -787,3 +787,39 @@ Boolean SqlKillNoWait():
return noWait;
}
}
+
+/**
+ * Parses an EXPLAIN PLAN statement.
+ */
+SqlNode SqlIgniteExplain() :
+{
+SqlNode stmt;
+SqlExplainLevel detailLevel = SqlExplainLevel.EXPPLAN_ATTRIBUTES;
+SqlExplain.Depth depth;
+Span s;
+final SqlExplainFormat format;
+}
+{
+ { s = span(); }
+[ detailLevel = ExplainDetailLevel() ]
+depth = ExplainDepth()
+(
+LOOKAHEAD(2)
+ { format = SqlExplainFormat.XML; }
+|
+LOOKAHEAD(2)
+ { format = SqlExplainFormat.JSON; }
+|
+ { format = SqlExplainFormat.DOT; }
Review Comment:
Should we use LOOKAHEAD(2) here as well or it is enough to set it once for
the very first option?
--
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]
Re: [PR] IGNITE-25092 Sql. Running queries view should return query string in valid format [ignite-3]
korlov42 commented on code in PR #5603:
URL: https://github.com/apache/ignite-3/pull/5603#discussion_r2042073042
##
modules/sql-engine/src/main/codegen/includes/parserImpls.ftl:
##
@@ -787,3 +787,39 @@ Boolean SqlKillNoWait():
return noWait;
}
}
+
+/**
+ * Parses an EXPLAIN PLAN statement.
+ */
+SqlNode SqlIgniteExplain() :
+{
+SqlNode stmt;
+SqlExplainLevel detailLevel = SqlExplainLevel.EXPPLAN_ATTRIBUTES;
+SqlExplain.Depth depth;
+Span s;
+final SqlExplainFormat format;
+}
+{
+ { s = span(); }
+[ detailLevel = ExplainDetailLevel() ]
+depth = ExplainDepth()
+(
+LOOKAHEAD(2)
+ { format = SqlExplainFormat.XML; }
+|
+LOOKAHEAD(2)
+ { format = SqlExplainFormat.JSON; }
+|
+ { format = SqlExplainFormat.DOT; }
Review Comment:
It's enough to use it for first two options.
LOOKAHEAD is used when you have several options with the same prefix. For
instance, without LA(2) parser will choose ` ` as soon as it see
`` token. With LA(2) it check the next token as well. Given that there is
nothing to choose from after ` ` it is OK to omit LA(2)
--
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]
Re: [PR] IGNITE-25092 Sql. Running queries view should return query string in valid format [ignite-3]
korlov42 merged PR #5603: URL: https://github.com/apache/ignite-3/pull/5603 -- 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]
Re: [PR] IGNITE-25092 Sql. Running queries view should return query string in valid format [ignite-3]
korlov42 commented on code in PR #5603:
URL: https://github.com/apache/ignite-3/pull/5603#discussion_r2042086946
##
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/sql/ParserServiceImplTest.java:
##
@@ -142,7 +143,52 @@ void parseMultiStatementQuery() {
assertThat(parsedTree, notNullValue());
assertThat(parsedTree.toString(),
equalTo(singleStatementResult.parsedTree().toString()));
assertThat(result.normalizedQuery(),
equalTo(singleStatementResult.normalizedQuery()));
-assertThat(result.originalQuery(),
equalTo(singleStatementResult.normalizedQuery()));
+assertThat(result.originalQuery(),
containsString(singleStatementResult.originalQuery()));
+}
+}
+
+@Test
+void originalQueryMatchesTheWayItIsSpecifiedInScript() {
+ParserService service = new ParserServiceImpl();
+
+@SuppressWarnings("ConcatenationWithEmptyString")
+String script = ""
++ "-- simple comment before first statement \n"
++ "seLECT * FROM Table_1; -- simple comment after first\n"
++ "/* multiline\n"
++ "comment\n"
++ "before second */ \n"
++ "select /*+ USE_INDEX(table_2_idx)*/ Table_2.* \n"
++ " FROM table_2; /* multiline\n"
++ "comment"
++ "after second */";
+
+List results = service.parseScript(script);
+
+for (ParsedResult result : results) {
+assertThat(
+script,
+containsString(result.originalQuery())
+);
+}
Review Comment:
makes sense! Added additional check
--
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]
Re: [PR] IGNITE-25092 Sql. Running queries view should return query string in valid format [ignite-3]
AMashenkov commented on code in PR #5603:
URL: https://github.com/apache/ignite-3/pull/5603#discussion_r2041955775
##
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/sql/ParserServiceImplTest.java:
##
@@ -142,7 +143,52 @@ void parseMultiStatementQuery() {
assertThat(parsedTree, notNullValue());
assertThat(parsedTree.toString(),
equalTo(singleStatementResult.parsedTree().toString()));
assertThat(result.normalizedQuery(),
equalTo(singleStatementResult.normalizedQuery()));
-assertThat(result.originalQuery(),
equalTo(singleStatementResult.normalizedQuery()));
+assertThat(result.originalQuery(),
containsString(singleStatementResult.originalQuery()));
+}
+}
+
+@Test
+void originalQueryMatchesTheWayItIsSpecifiedInScript() {
+ParserService service = new ParserServiceImpl();
+
+@SuppressWarnings("ConcatenationWithEmptyString")
+String script = ""
++ "-- simple comment before first statement \n"
++ "seLECT * FROM Table_1; -- simple comment after first\n"
++ "/* multiline\n"
++ "comment\n"
++ "before second */ \n"
++ "select /*+ USE_INDEX(table_2_idx)*/ Table_2.* \n"
++ " FROM table_2; /* multiline\n"
++ "comment"
++ "after second */";
+
+List results = service.parseScript(script);
+
+for (ParsedResult result : results) {
+assertThat(
+script,
+containsString(result.originalQuery())
+);
+}
Review Comment:
Let's check all strings are valid and we don't see partial comments after
semicolon like` FROM table_2;/*`
Or better add a test for this case.
--
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]
Re: [PR] IGNITE-25092 Sql. Running queries view should return query string in valid format [ignite-3]
AMashenkov commented on code in PR #5603:
URL: https://github.com/apache/ignite-3/pull/5603#discussion_r2041955775
##
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/sql/ParserServiceImplTest.java:
##
@@ -142,7 +143,52 @@ void parseMultiStatementQuery() {
assertThat(parsedTree, notNullValue());
assertThat(parsedTree.toString(),
equalTo(singleStatementResult.parsedTree().toString()));
assertThat(result.normalizedQuery(),
equalTo(singleStatementResult.normalizedQuery()));
-assertThat(result.originalQuery(),
equalTo(singleStatementResult.normalizedQuery()));
+assertThat(result.originalQuery(),
containsString(singleStatementResult.originalQuery()));
+}
+}
+
+@Test
+void originalQueryMatchesTheWayItIsSpecifiedInScript() {
+ParserService service = new ParserServiceImpl();
+
+@SuppressWarnings("ConcatenationWithEmptyString")
+String script = ""
++ "-- simple comment before first statement \n"
++ "seLECT * FROM Table_1; -- simple comment after first\n"
++ "/* multiline\n"
++ "comment\n"
++ "before second */ \n"
++ "select /*+ USE_INDEX(table_2_idx)*/ Table_2.* \n"
++ " FROM table_2; /* multiline\n"
++ "comment"
++ "after second */";
+
+List results = service.parseScript(script);
+
+for (ParsedResult result : results) {
+assertThat(
+script,
+containsString(result.originalQuery())
+);
+}
Review Comment:
Let's check all strings are valid and we don't see partial comments after
semicolon like` FROM table_2;/*`
--
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]
Re: [PR] IGNITE-25092 Sql. Running queries view should return query string in valid format [ignite-3]
AMashenkov commented on code in PR #5603:
URL: https://github.com/apache/ignite-3/pull/5603#discussion_r2041936870
##
modules/sql-engine/src/main/codegen/includes/parserImpls.ftl:
##
@@ -787,3 +787,39 @@ Boolean SqlKillNoWait():
return noWait;
}
}
+
+/**
+ * Parses an EXPLAIN PLAN statement.
+ */
+SqlNode SqlIgniteExplain() :
+{
+SqlNode stmt;
+SqlExplainLevel detailLevel = SqlExplainLevel.EXPPLAN_ATTRIBUTES;
+SqlExplain.Depth depth;
+Span s;
+final SqlExplainFormat format;
+}
+{
+ { s = span(); }
+[ detailLevel = ExplainDetailLevel() ]
+depth = ExplainDepth()
+(
+LOOKAHEAD(2)
+ { format = SqlExplainFormat.XML; }
+|
+LOOKAHEAD(2)
+ { format = SqlExplainFormat.JSON; }
+|
+ { format = SqlExplainFormat.DOT; }
Review Comment:
```suggestion
LOOKAHEAD(2)
{ format = SqlExplainFormat.DOT; }
```
--
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]
