You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2021/01/21 17:26:35 UTC
[GitHub] [calcite] wangjie-fourth opened a new pull request #2332: [CALCITE-4474] SqlSimpleParser inner Tokenizer ignore comment in some special case
wangjie-fourth opened a new pull request #2332:
URL: https://github.com/apache/calcite/pull/2332
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2332: [CALCITE-4474] SqlSimpleParser inner Tokenizer should not recognize the sql of TokenType.ID or some keywords in some case
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2332:
URL: https://github.com/apache/calcite/pull/2332#discussion_r563275831
##########
File path: core/src/test/java/org/apache/calcite/sql/test/SqlAdvisorTest.java
##########
@@ -1585,4 +1587,47 @@ private void testSimpleParserQuotedIdImpl() {
assertSimplify(sql, simplified);
assertComplete(sql, EXPR_KEYWORDS, Collections.singletonList("TABLE(a)"), DEPT_COLUMNS);
}
+
+
+ @Test public void testFilterComment() {
+ // SqlSimpleParser.Tokenizer#nextToken() lines 401 - 423
+ // is used to recognize the sql of TokenType.ID or some keywords
+ // if a certain segment of characters is continuously composed of Token,
+ // the function of this code may be wrong
+ // E.g :
+ // (1)select * from a where price> 10.0--comment
+ // 【10.0--comment】should be recognize as TokenType.ID("10.0") and TokenType.COMMENT
+ // but it recognize as TokenType.ID("10.0--comment")
+ // (2)select * from a where column_b='/* this is not comment */'
+ // 【/* this is not comment */】should be recognize as
+ // TokenType.SQID("/* this is not comment */"), but it was not
+
+ final String baseOriginSql = "select * from a ";
+ final String baseResultSql = "SELECT * FROM a ";
+ String originSql;
+
+ // when SqlSimpleParser.Tokenizer#nextToken() method parse sql,
+ // ignore the "--" after 10.0, this is a comment,
+ // but Tokenizer#nextToken() does not recognize it
+ originSql = baseOriginSql + "where price > 10.0-- this is comment "
+ + System.lineSeparator() + " -- comment ";
+ filterCommentHelper(originSql, baseResultSql + "WHERE price > 10.0");
+
+ originSql = baseOriginSql + "where column_b='/* this is not comment */'";
+ filterCommentHelper(originSql, baseResultSql + "WHERE column_b= '/* this is not comment */'");
+
+ originSql = baseOriginSql + "where column_b='2021 --this is not comment'";
+ filterCommentHelper(originSql, baseResultSql + "WHERE column_b= '2021 --this is not comment'");
+
+ originSql = baseOriginSql + "where column_b='2021--this is not comment'";
+ filterCommentHelper(originSql, baseResultSql + "WHERE column_b= '2021--this is not comment'");
+ }
+
+ private void filterCommentHelper(String originSql, String expectedSql) {
+ SqlSimpleParser simpleParser =
+ new SqlSimpleParser("_suggest_", SqlParser.Config.DEFAULT);
+
+ String actualSql = simpleParser.simplifySql(originSql);
+ assertThat(originSql, actualSql, equalTo(expectedSql));
Review comment:
Please add clarification to the message, for instance: `"simpleParser.simplifySql(" + originSql + ")"`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] amaliujia closed pull request #2332: [CALCITE-4474] SqlSimpleParser inner Tokenizer should not recognize the sql of TokenType.ID or some keywords in some case
Posted by GitBox <gi...@apache.org>.
amaliujia closed pull request #2332:
URL: https://github.com/apache/calcite/pull/2332
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] amaliujia commented on pull request #2332: [CALCITE-4474] SqlSimpleParser inner Tokenizer ignore comment in some special case
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #2332:
URL: https://github.com/apache/calcite/pull/2332#issuecomment-765610785
Overall LGTM. Left a comment on the test 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] amaliujia edited a comment on pull request #2332: [CALCITE-4474] SqlSimpleParser inner Tokenizer should not recognize the sql of TokenType.ID or some keywords in some case
Posted by GitBox <gi...@apache.org>.
amaliujia edited a comment on pull request #2332:
URL: https://github.com/apache/calcite/pull/2332#issuecomment-766506646
Thanks for all the code review so far! If there is no more comments within a day, I will merge this PR.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] wangjie-fourth commented on a change in pull request #2332: [CALCITE-4474] SqlSimpleParser inner Tokenizer should not recognize the sql of TokenType.ID or some keywords in some case
Posted by GitBox <gi...@apache.org>.
wangjie-fourth commented on a change in pull request #2332:
URL: https://github.com/apache/calcite/pull/2332#discussion_r563278334
##########
File path: core/src/test/java/org/apache/calcite/sql/test/SqlAdvisorTest.java
##########
@@ -1585,4 +1587,47 @@ private void testSimpleParserQuotedIdImpl() {
assertSimplify(sql, simplified);
assertComplete(sql, EXPR_KEYWORDS, Collections.singletonList("TABLE(a)"), DEPT_COLUMNS);
}
+
+
+ @Test public void testFilterComment() {
+ // SqlSimpleParser.Tokenizer#nextToken() lines 401 - 423
+ // is used to recognize the sql of TokenType.ID or some keywords
+ // if a certain segment of characters is continuously composed of Token,
+ // the function of this code may be wrong
+ // E.g :
+ // (1)select * from a where price> 10.0--comment
+ // 【10.0--comment】should be recognize as TokenType.ID("10.0") and TokenType.COMMENT
+ // but it recognize as TokenType.ID("10.0--comment")
+ // (2)select * from a where column_b='/* this is not comment */'
+ // 【/* this is not comment */】should be recognize as
+ // TokenType.SQID("/* this is not comment */"), but it was not
+
+ final String baseOriginSql = "select * from a ";
+ final String baseResultSql = "SELECT * FROM a ";
+ String originSql;
+
+ // when SqlSimpleParser.Tokenizer#nextToken() method parse sql,
+ // ignore the "--" after 10.0, this is a comment,
+ // but Tokenizer#nextToken() does not recognize it
+ originSql = baseOriginSql + "where price > 10.0-- this is comment "
+ + System.lineSeparator() + " -- comment ";
+ filterCommentHelper(originSql, baseResultSql + "WHERE price > 10.0");
+
+ originSql = baseOriginSql + "where column_b='/* this is not comment */'";
+ filterCommentHelper(originSql, baseResultSql + "WHERE column_b= '/* this is not comment */'");
+
+ originSql = baseOriginSql + "where column_b='2021 --this is not comment'";
+ filterCommentHelper(originSql, baseResultSql + "WHERE column_b= '2021 --this is not comment'");
+
+ originSql = baseOriginSql + "where column_b='2021--this is not comment'";
+ filterCommentHelper(originSql, baseResultSql + "WHERE column_b= '2021--this is not comment'");
+ }
+
+ private void filterCommentHelper(String originSql, String expectedSql) {
+ SqlSimpleParser simpleParser =
+ new SqlSimpleParser("_suggest_", SqlParser.Config.DEFAULT);
+
+ String actualSql = simpleParser.simplifySql(originSql);
+ assertThat(originSql, actualSql, equalTo(expectedSql));
Review comment:
thanks, it is done
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] wangjie-fourth commented on pull request #2332: [CALCITE-4474] SqlSimpleParser inner Tokenizer should not recognize the sql of TokenType.ID or some keywords in some case
Posted by GitBox <gi...@apache.org>.
wangjie-fourth commented on pull request #2332:
URL: https://github.com/apache/calcite/pull/2332#issuecomment-765927111
@amaliujia @julianhyde @vlsi
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2332: [CALCITE-4474] SqlSimpleParser inner Tokenizer should not recognize the sql of TokenType.ID or some keywords in some case
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2332:
URL: https://github.com/apache/calcite/pull/2332#discussion_r563275789
##########
File path: core/src/test/java/org/apache/calcite/sql/test/SqlAdvisorTest.java
##########
@@ -1585,4 +1587,47 @@ private void testSimpleParserQuotedIdImpl() {
assertSimplify(sql, simplified);
assertComplete(sql, EXPR_KEYWORDS, Collections.singletonList("TABLE(a)"), DEPT_COLUMNS);
}
+
+
+ @Test public void testFilterComment() {
+ // SqlSimpleParser.Tokenizer#nextToken() lines 401 - 423
+ // is used to recognize the sql of TokenType.ID or some keywords
+ // if a certain segment of characters is continuously composed of Token,
+ // the function of this code may be wrong
+ // E.g :
+ // (1)select * from a where price> 10.0--comment
+ // 【10.0--comment】should be recognize as TokenType.ID("10.0") and TokenType.COMMENT
+ // but it recognize as TokenType.ID("10.0--comment")
+ // (2)select * from a where column_b='/* this is not comment */'
+ // 【/* this is not comment */】should be recognize as
+ // TokenType.SQID("/* this is not comment */"), but it was not
+
+ final String baseOriginSql = "select * from a ";
+ final String baseResultSql = "SELECT * FROM a ";
+ String originSql;
+
+ // when SqlSimpleParser.Tokenizer#nextToken() method parse sql,
+ // ignore the "--" after 10.0, this is a comment,
+ // but Tokenizer#nextToken() does not recognize it
+ originSql = baseOriginSql + "where price > 10.0-- this is comment "
+ + System.lineSeparator() + " -- comment ";
+ filterCommentHelper(originSql, baseResultSql + "WHERE price > 10.0");
+
+ originSql = baseOriginSql + "where column_b='/* this is not comment */'";
+ filterCommentHelper(originSql, baseResultSql + "WHERE column_b= '/* this is not comment */'");
+
+ originSql = baseOriginSql + "where column_b='2021 --this is not comment'";
+ filterCommentHelper(originSql, baseResultSql + "WHERE column_b= '2021 --this is not comment'");
+
+ originSql = baseOriginSql + "where column_b='2021--this is not comment'";
+ filterCommentHelper(originSql, baseResultSql + "WHERE column_b= '2021--this is not comment'");
+ }
+
+ private void filterCommentHelper(String originSql, String expectedSql) {
Review comment:
Please rename the method so it is clear what it does.
In most cases, `helper` in the method name does not add much value.
For instance: `assertSimplifySql`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] amaliujia edited a comment on pull request #2332: [CALCITE-4474] SqlSimpleParser inner Tokenizer should not recognize the sql of TokenType.ID or some keywords in some case
Posted by GitBox <gi...@apache.org>.
amaliujia edited a comment on pull request #2332:
URL: https://github.com/apache/calcite/pull/2332#issuecomment-766506646
Thanks for all the code review so far! If there is no more comments within a day, I will merge this PR.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] amaliujia commented on a change in pull request #2332: [CALCITE-4474] SqlSimpleParser inner Tokenizer ignore comment in some special case
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #2332:
URL: https://github.com/apache/calcite/pull/2332#discussion_r562275975
##########
File path: core/src/test/java/org/apache/calcite/sql/advise/SqlSimpleParserTest.java
##########
@@ -0,0 +1,42 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.calcite.sql.advise;
+
+import org.apache.calcite.sql.parser.SqlParser;
+
+import org.junit.jupiter.api.Test;
+
+import static org.hamcrest.CoreMatchers.equalTo;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+/**
+ * A <code>SqlSimpleParserTest</code> is a unit-test for
+ * {@link org.apache.calcite.sql.advise.SqlSimpleParser}.
+ */
+public class SqlSimpleParserTest {
Review comment:
Per the suggestion in JIRA, can you move this test to SqlAdvisorTest
##########
File path: core/src/test/java/org/apache/calcite/sql/advise/SqlSimpleParserTest.java
##########
@@ -0,0 +1,42 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.calcite.sql.advise;
+
+import org.apache.calcite.sql.parser.SqlParser;
+
+import org.junit.jupiter.api.Test;
+
+import static org.hamcrest.CoreMatchers.equalTo;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+/**
+ * A <code>SqlSimpleParserTest</code> is a unit-test for
+ * {@link org.apache.calcite.sql.advise.SqlSimpleParser}.
+ */
+public class SqlSimpleParserTest {
Review comment:
Per the suggestion in JIRA, can you move this test to SqlAdvisorTest?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] amaliujia commented on a change in pull request #2332: [CALCITE-4474] SqlSimpleParser inner Tokenizer ignore comment in some special case
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #2332:
URL: https://github.com/apache/calcite/pull/2332#discussion_r562832978
##########
File path: core/src/test/java/org/apache/calcite/sql/test/SqlAdvisorTest.java
##########
@@ -1585,4 +1587,17 @@ private void testSimpleParserQuotedIdImpl() {
assertSimplify(sql, simplified);
assertComplete(sql, EXPR_KEYWORDS, Collections.singletonList("TABLE(a)"), DEPT_COLUMNS);
}
+
+ private static SqlSimpleParser simpleParser =
+ new SqlSimpleParser("_suggest_", SqlParser.Config.DEFAULT);
+
+ @Test public void testFilterComment() {
+ // when SqlSimpleParser.Tokenizer#nextToken() method parse sql,
+ // ignore the "--" after 10.0, this is a comment,
+ // but Tokenizer#nextToken() does not recognize it
+ String sqlWithComment = "select count(*) from a where price > 10.0-- this is comment n"
Review comment:
The last `n` should be `\n`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] wangjie-fourth commented on pull request #2332: [CALCITE-4474] SqlSimpleParser inner Tokenizer should not recognize the sql of TokenType.ID or some keywords in some case
Posted by GitBox <gi...@apache.org>.
wangjie-fourth commented on pull request #2332:
URL: https://github.com/apache/calcite/pull/2332#issuecomment-765930179
@amaliujia @julianhyde @vlsi Hi,thanks for your review, I have done what your said.Is there anything else I need to do?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2332: [CALCITE-4474] SqlSimpleParser inner Tokenizer should not recognize the sql of TokenType.ID or some keywords in some case
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2332:
URL: https://github.com/apache/calcite/pull/2332#discussion_r563098592
##########
File path: core/src/test/java/org/apache/calcite/sql/test/SqlAdvisorTest.java
##########
@@ -1585,4 +1587,47 @@ private void testSimpleParserQuotedIdImpl() {
assertSimplify(sql, simplified);
assertComplete(sql, EXPR_KEYWORDS, Collections.singletonList("TABLE(a)"), DEPT_COLUMNS);
}
+
+
+ @Test public void testFilterComment() {
+ // SqlSimpleParser.Tokenizer#nextToken() lines 401 - 423
+ // is used to recognize the sql of TokenType.ID or some keywords
+ // if a certain segment of characters is continuously composed of Token,
+ // the function of this code may be wrong
+ // E.g :
+ // (1)select * from a where price> 10.0--comment
+ // 【10.0--comment】should be recognize as TokenType.ID("10.0") and TokenType.COMMENT
+ // but it recognize as TokenType.ID("10.0--comment")
+ // (2)select * from a where column_b='/* this is not comment */'
+ // 【/* this is not comment */】should be recognize as
+ // TokenType.SQID("/* this is not comment */"), but it was not
+
+ SqlSimpleParser simpleParser =
+ new SqlSimpleParser("_suggest_", SqlParser.Config.DEFAULT);
+
+ final String baseOriginSql = "select * from a ";
+ final String baseResultSql = "SELECT * FROM a ";
+ String originSql;
+ String actualSql;
+
+ // when SqlSimpleParser.Tokenizer#nextToken() method parse sql,
+ // ignore the "--" after 10.0, this is a comment,
+ // but Tokenizer#nextToken() does not recognize it
+ originSql = baseOriginSql + "where price > 10.0-- this is comment \n"
+ + " -- comment ";
+ actualSql = simpleParser.simplifySql(originSql);
+ assertThat(actualSql, equalTo(baseResultSql + "WHERE price > 10.0"));
+
+ originSql = baseOriginSql + "where column_b='/* this is not comment */'";
+ actualSql = simpleParser.simplifySql(originSql);
+ assertThat(actualSql, equalTo(baseResultSql + "WHERE column_b= '/* this is not comment */'"));
Review comment:
Please factor this out to a helper method that would take `originSql`, and `expectedSql`.
Then the method should add `originSql`into the assertion message.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] wangjie-fourth commented on a change in pull request #2332: [CALCITE-4474] SqlSimpleParser inner Tokenizer should not recognize the sql of TokenType.ID or some keywords in some case
Posted by GitBox <gi...@apache.org>.
wangjie-fourth commented on a change in pull request #2332:
URL: https://github.com/apache/calcite/pull/2332#discussion_r563278326
##########
File path: core/src/test/java/org/apache/calcite/sql/test/SqlAdvisorTest.java
##########
@@ -1585,4 +1587,47 @@ private void testSimpleParserQuotedIdImpl() {
assertSimplify(sql, simplified);
assertComplete(sql, EXPR_KEYWORDS, Collections.singletonList("TABLE(a)"), DEPT_COLUMNS);
}
+
+
+ @Test public void testFilterComment() {
+ // SqlSimpleParser.Tokenizer#nextToken() lines 401 - 423
+ // is used to recognize the sql of TokenType.ID or some keywords
+ // if a certain segment of characters is continuously composed of Token,
+ // the function of this code may be wrong
+ // E.g :
+ // (1)select * from a where price> 10.0--comment
+ // 【10.0--comment】should be recognize as TokenType.ID("10.0") and TokenType.COMMENT
+ // but it recognize as TokenType.ID("10.0--comment")
+ // (2)select * from a where column_b='/* this is not comment */'
+ // 【/* this is not comment */】should be recognize as
+ // TokenType.SQID("/* this is not comment */"), but it was not
+
+ final String baseOriginSql = "select * from a ";
+ final String baseResultSql = "SELECT * FROM a ";
+ String originSql;
+
+ // when SqlSimpleParser.Tokenizer#nextToken() method parse sql,
+ // ignore the "--" after 10.0, this is a comment,
+ // but Tokenizer#nextToken() does not recognize it
+ originSql = baseOriginSql + "where price > 10.0-- this is comment "
+ + System.lineSeparator() + " -- comment ";
+ filterCommentHelper(originSql, baseResultSql + "WHERE price > 10.0");
+
+ originSql = baseOriginSql + "where column_b='/* this is not comment */'";
+ filterCommentHelper(originSql, baseResultSql + "WHERE column_b= '/* this is not comment */'");
+
+ originSql = baseOriginSql + "where column_b='2021 --this is not comment'";
+ filterCommentHelper(originSql, baseResultSql + "WHERE column_b= '2021 --this is not comment'");
+
+ originSql = baseOriginSql + "where column_b='2021--this is not comment'";
+ filterCommentHelper(originSql, baseResultSql + "WHERE column_b= '2021--this is not comment'");
+ }
+
+ private void filterCommentHelper(String originSql, String expectedSql) {
Review comment:
thanks, it is done
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] amaliujia closed pull request #2332: [CALCITE-4474] SqlSimpleParser inner Tokenizer should not recognize the sql of TokenType.ID or some keywords in some case
Posted by GitBox <gi...@apache.org>.
amaliujia closed pull request #2332:
URL: https://github.com/apache/calcite/pull/2332
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] julianhyde commented on a change in pull request #2332: [CALCITE-4474] SqlSimpleParser inner Tokenizer ignore comment in some special case
Posted by GitBox <gi...@apache.org>.
julianhyde commented on a change in pull request #2332:
URL: https://github.com/apache/calcite/pull/2332#discussion_r563033587
##########
File path: core/src/test/java/org/apache/calcite/sql/test/SqlAdvisorTest.java
##########
@@ -1585,4 +1587,17 @@ private void testSimpleParserQuotedIdImpl() {
assertSimplify(sql, simplified);
assertComplete(sql, EXPR_KEYWORDS, Collections.singletonList("TABLE(a)"), DEPT_COLUMNS);
}
+
+ private static SqlSimpleParser simpleParser =
+ new SqlSimpleParser("_suggest_", SqlParser.Config.DEFAULT);
Review comment:
Make this a local variable, not a static field. Static fields use memory after the test has finished, and may not be reentrant.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] amaliujia commented on pull request #2332: [CALCITE-4474] SqlSimpleParser inner Tokenizer should not recognize the sql of TokenType.ID or some keywords in some case
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #2332:
URL: https://github.com/apache/calcite/pull/2332#issuecomment-766506646
Thanks for all the code review so far! If there is no more comments with a day, I will merge this PR.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] amaliujia commented on pull request #2332: [CALCITE-4474] SqlSimpleParser inner Tokenizer should not recognize the sql of TokenType.ID or some keywords in some case
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #2332:
URL: https://github.com/apache/calcite/pull/2332#issuecomment-766294197
@wangjie-fourth
Thanks. I will give another pass soon! If there is not major comment I will merge this PR (for minors I can update directly).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] amaliujia commented on pull request #2332: [CALCITE-4474] SqlSimpleParser inner Tokenizer should not recognize the sql of TokenType.ID or some keywords in some case
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #2332:
URL: https://github.com/apache/calcite/pull/2332#issuecomment-767261160
Fixed via https://github.com/apache/calcite/commit/a16a47351aa25caf538c1955edd171ec871569f7
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] amaliujia commented on a change in pull request #2332: [CALCITE-4474] SqlSimpleParser inner Tokenizer ignore comment in some special case
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #2332:
URL: https://github.com/apache/calcite/pull/2332#discussion_r562275975
##########
File path: core/src/test/java/org/apache/calcite/sql/advise/SqlSimpleParserTest.java
##########
@@ -0,0 +1,42 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.calcite.sql.advise;
+
+import org.apache.calcite.sql.parser.SqlParser;
+
+import org.junit.jupiter.api.Test;
+
+import static org.hamcrest.CoreMatchers.equalTo;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+/**
+ * A <code>SqlSimpleParserTest</code> is a unit-test for
+ * {@link org.apache.calcite.sql.advise.SqlSimpleParser}.
+ */
+public class SqlSimpleParserTest {
Review comment:
Per the suggestion in JIRA, can you move this test to SqlAdvisorTest
##########
File path: core/src/test/java/org/apache/calcite/sql/advise/SqlSimpleParserTest.java
##########
@@ -0,0 +1,42 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.calcite.sql.advise;
+
+import org.apache.calcite.sql.parser.SqlParser;
+
+import org.junit.jupiter.api.Test;
+
+import static org.hamcrest.CoreMatchers.equalTo;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+/**
+ * A <code>SqlSimpleParserTest</code> is a unit-test for
+ * {@link org.apache.calcite.sql.advise.SqlSimpleParser}.
+ */
+public class SqlSimpleParserTest {
Review comment:
Per the suggestion in JIRA, can you move this test to SqlAdvisorTest?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] amaliujia commented on pull request #2332: [CALCITE-4474] SqlSimpleParser inner Tokenizer should not recognize the sql of TokenType.ID or some keywords in some case
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #2332:
URL: https://github.com/apache/calcite/pull/2332#issuecomment-766506646
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] wangjie-fourth removed a comment on pull request #2332: [CALCITE-4474] SqlSimpleParser inner Tokenizer should not recognize the sql of TokenType.ID or some keywords in some case
Posted by GitBox <gi...@apache.org>.
wangjie-fourth removed a comment on pull request #2332:
URL: https://github.com/apache/calcite/pull/2332#issuecomment-765927111
@amaliujia @julianhyde @vlsi
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org