[jira] [Commented] (CALCITE-2453) Enhance the SQL parser in order to optionally support semicolon at the end of the sql statements
[
https://issues.apache.org/jira/browse/CALCITE-2453?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16823643#comment-16823643
]
Chunwei Lei commented on CALCITE-2453:
--
Thanks for your review, [~julianhyde]. PR is updated including:
* Add some javadoc in {{SqlParserTest}} and merge some tests.
* Use use {{assertThat}} rather than {{assertEquals}}.
I prefer to make {{checkList}} take {{List }}since it can use
{{check(String sql, String expected)}} simply.
> Enhance the SQL parser in order to optionally support semicolon at the end of
> the sql statements
>
>
> Key: CALCITE-2453
> URL: https://issues.apache.org/jira/browse/CALCITE-2453
> Project: Calcite
> Issue Type: Improvement
> Components: core
>Affects Versions: next
>Reporter: charbel yazbeck
>Assignee: Chunwei Lei
>Priority: Trivial
> Labels: pull-request-available
> Attachments:
> 0001-CALCITE-2453-Allowing-SQL-statements-to-optionally-e.patch
>
> Time Spent: 1h 20m
> Remaining Estimate: 0h
>
> Consists of adding [] in Parser.jj
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (CALCITE-2453) Enhance the SQL parser in order to optionally support semicolon at the end of the sql statements
[ https://issues.apache.org/jira/browse/CALCITE-2453?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16823641#comment-16823641 ] pengzhiwei commented on CALCITE-2453: - Hi [~Chunwei Lei], It works well for me in the case which mixed "select" with the "insert" according to you implement. Thanks! > Enhance the SQL parser in order to optionally support semicolon at the end of > the sql statements > > > Key: CALCITE-2453 > URL: https://issues.apache.org/jira/browse/CALCITE-2453 > Project: Calcite > Issue Type: Improvement > Components: core >Affects Versions: next >Reporter: charbel yazbeck >Assignee: Chunwei Lei >Priority: Trivial > Labels: pull-request-available > Attachments: > 0001-CALCITE-2453-Allowing-SQL-statements-to-optionally-e.patch > > Time Spent: 1h 20m > Remaining Estimate: 0h > > Consists of adding [] in Parser.jj -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (CALCITE-2453) Enhance the SQL parser in order to optionally support semicolon at the end of the sql statements
[
https://issues.apache.org/jira/browse/CALCITE-2453?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16823377#comment-16823377
]
Julian Hyde commented on CALCITE-2453:
--
* Need more javadoc in {{SqlParserTest}}. Explain the intent of your new tests.
They look very similar to existing tests. Likewise explain the intent of
{{sqlList}}.
* Please use {{assertThat}} rather than {{assertEquals}}. if it would make
things simpler, consider making {{checkList}} take {{List>}}
rather than {{List}}.
> Enhance the SQL parser in order to optionally support semicolon at the end of
> the sql statements
>
>
> Key: CALCITE-2453
> URL: https://issues.apache.org/jira/browse/CALCITE-2453
> Project: Calcite
> Issue Type: Improvement
> Components: core
>Affects Versions: next
>Reporter: charbel yazbeck
>Assignee: Chunwei Lei
>Priority: Trivial
> Labels: pull-request-available
> Attachments:
> 0001-CALCITE-2453-Allowing-SQL-statements-to-optionally-e.patch
>
> Time Spent: 1h 20m
> Remaining Estimate: 0h
>
> Consists of adding [] in Parser.jj
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (CALCITE-2453) Enhance the SQL parser in order to optionally support semicolon at the end of the sql statements
[ https://issues.apache.org/jira/browse/CALCITE-2453?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16822901#comment-16822901 ] Chunwei Lei commented on CALCITE-2453: -- Thanks [~pzw2018]. I updated the PR with more test cases which mixed the "select" statement with the "insert" statement. It works correctly. > Enhance the SQL parser in order to optionally support semicolon at the end of > the sql statements > > > Key: CALCITE-2453 > URL: https://issues.apache.org/jira/browse/CALCITE-2453 > Project: Calcite > Issue Type: Improvement > Components: core >Affects Versions: next >Reporter: charbel yazbeck >Assignee: Chunwei Lei >Priority: Trivial > Labels: pull-request-available > Attachments: > 0001-CALCITE-2453-Allowing-SQL-statements-to-optionally-e.patch > > Time Spent: 1h 20m > Remaining Estimate: 0h > > Consists of adding [] in Parser.jj -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (CALCITE-2453) Enhance the SQL parser in order to optionally support semicolon at the end of the sql statements
[ https://issues.apache.org/jira/browse/CALCITE-2453?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16822840#comment-16822840 ] pengzhiwei commented on CALCITE-2453: - Hi [~Chunwei Lei], can you add a test which mixed the "select" statement with the "insert" statement? In our previous implement similar to you PR, there is conflict between them. > Enhance the SQL parser in order to optionally support semicolon at the end of > the sql statements > > > Key: CALCITE-2453 > URL: https://issues.apache.org/jira/browse/CALCITE-2453 > Project: Calcite > Issue Type: Improvement > Components: core >Affects Versions: next >Reporter: charbel yazbeck >Assignee: Chunwei Lei >Priority: Trivial > Labels: pull-request-available > Attachments: > 0001-CALCITE-2453-Allowing-SQL-statements-to-optionally-e.patch > > Time Spent: 1h 20m > Remaining Estimate: 0h > > Consists of adding [] in Parser.jj -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (CALCITE-2453) Enhance the SQL parser in order to optionally support semicolon at the end of the sql statements
[
https://issues.apache.org/jira/browse/CALCITE-2453?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16822502#comment-16822502
]
Chunwei Lei commented on CALCITE-2453:
--
{quote}The first statement is parsed successfully and the second one failed.
Should we keep result of the first statement? Or should we consider it as a
failure?
{quote}
Sorry, I change my idea. I think it is better to consider it as a failure when
meeting an error statement which seems more reasonable.
Besides, I find PR#783 is not as good as expected since it treats some negative
test cases as success wrongly. For instance, the following sql
{code:java}
select * from emp where name like 'toto'; delete from emp; delete from emp
select * from dept
{code}
will be parsed successfully. As a result, I opened a new pull request:
[https://github.com/apache/calcite/pull/1177|https://github.com/apache/calcite/pull/1177/].
The major changes in the new PR are as follows:
* Correct the wrong definition in the .jj file
* Add some negative test cases
* Resolve conflicts
> Enhance the SQL parser in order to optionally support semicolon at the end of
> the sql statements
>
>
> Key: CALCITE-2453
> URL: https://issues.apache.org/jira/browse/CALCITE-2453
> Project: Calcite
> Issue Type: Improvement
> Components: core
>Affects Versions: next
>Reporter: charbel yazbeck
>Assignee: Chunwei Lei
>Priority: Trivial
> Labels: pull-request-available
> Attachments:
> 0001-CALCITE-2453-Allowing-SQL-statements-to-optionally-e.patch
>
> Time Spent: 1h 10m
> Remaining Estimate: 0h
>
> Consists of adding [] in Parser.jj
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (CALCITE-2453) Enhance the SQL parser in order to optionally support semicolon at the end of the sql statements
[
https://issues.apache.org/jira/browse/CALCITE-2453?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16820660#comment-16820660
]
Chunwei Lei commented on CALCITE-2453:
--
I am not sure that whether we should keep the result before the error
statement. For instance, when paring the sql as follows
{code:java}
select * from emp where name like 'toto'; delete from emp delete from emp{code}
the first statement is parsed successfully and the second one failed. Should we
keep result of the first statement? Or should we consider it as a failure?
> Enhance the SQL parser in order to optionally support semicolon at the end of
> the sql statements
>
>
> Key: CALCITE-2453
> URL: https://issues.apache.org/jira/browse/CALCITE-2453
> Project: Calcite
> Issue Type: Improvement
> Components: core
>Affects Versions: next
>Reporter: charbel yazbeck
>Assignee: Chunwei Lei
>Priority: Trivial
> Labels: pull-request-available
> Attachments:
> 0001-CALCITE-2453-Allowing-SQL-statements-to-optionally-e.patch
>
> Time Spent: 1h
> Remaining Estimate: 0h
>
> Consists of adding [] in Parser.jj
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (CALCITE-2453) Enhance the SQL parser in order to optionally support semicolon at the end of the sql statements
[ https://issues.apache.org/jira/browse/CALCITE-2453?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16820220#comment-16820220 ] Chunwei Lei commented on CALCITE-2453: -- Thanks [~julianhyde]. I reviewed the PR and I believe it is good except lack of some negative test cases. So I am going to add some negative test cases and rebase it as well. > Enhance the SQL parser in order to optionally support semicolon at the end of > the sql statements > > > Key: CALCITE-2453 > URL: https://issues.apache.org/jira/browse/CALCITE-2453 > Project: Calcite > Issue Type: Improvement > Components: core >Affects Versions: next >Reporter: charbel yazbeck >Assignee: Chunwei Lei >Priority: Trivial > Labels: pull-request-available > Attachments: > 0001-CALCITE-2453-Allowing-SQL-statements-to-optionally-e.patch > > Time Spent: 50m > Remaining Estimate: 0h > > Consists of adding [] in Parser.jj -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (CALCITE-2453) Enhance the SQL parser in order to optionally support semicolon at the end of the sql statements
[ https://issues.apache.org/jira/browse/CALCITE-2453?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16818307#comment-16818307 ] Julian Hyde commented on CALCITE-2453: -- I haven't had chance to review. [~Chunwei Lei], If you review the PR and it looks good, feel free to commit. As long as existing calls (e.g. from JDBC driver) continue to disallow trailing ';'. > Enhance the SQL parser in order to optionally support semicolon at the end of > the sql statements > > > Key: CALCITE-2453 > URL: https://issues.apache.org/jira/browse/CALCITE-2453 > Project: Calcite > Issue Type: Improvement > Components: core >Affects Versions: next >Reporter: charbel yazbeck >Priority: Trivial > Labels: pull-request-available > Attachments: > 0001-CALCITE-2453-Allowing-SQL-statements-to-optionally-e.patch > > > Consists of adding [] in Parser.jj -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (CALCITE-2453) Enhance the SQL parser in order to optionally support semicolon at the end of the sql statements
[ https://issues.apache.org/jira/browse/CALCITE-2453?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16816976#comment-16816976 ] Chunwei Lei commented on CALCITE-2453: -- Hi, [~julianhyde], [~charbel], any progress on it? I think it is a helpful feature worth to be done. > Enhance the SQL parser in order to optionally support semicolon at the end of > the sql statements > > > Key: CALCITE-2453 > URL: https://issues.apache.org/jira/browse/CALCITE-2453 > Project: Calcite > Issue Type: Improvement > Components: core >Affects Versions: next >Reporter: charbel yazbeck >Priority: Trivial > Labels: pull-request-available > Attachments: > 0001-CALCITE-2453-Allowing-SQL-statements-to-optionally-e.patch > > > Consists of adding [] in Parser.jj -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (CALCITE-2453) Enhance the SQL parser in order to optionally support semicolon at the end of the sql statements
[ https://issues.apache.org/jira/browse/CALCITE-2453?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16587275#comment-16587275 ] charbel yazbeck commented on CALCITE-2453: -- Hi Julian, are my last PR changes ok ? Any idea on when my PR will be merged? > Enhance the SQL parser in order to optionally support semicolon at the end of > the sql statements > > > Key: CALCITE-2453 > URL: https://issues.apache.org/jira/browse/CALCITE-2453 > Project: Calcite > Issue Type: Improvement > Components: core >Affects Versions: next >Reporter: charbel yazbeck >Assignee: Julian Hyde >Priority: Trivial > Attachments: > 0001-CALCITE-2453-Allowing-SQL-statements-to-optionally-e.patch > > > Consists of adding [] in Parser.jj -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (CALCITE-2453) Enhance the SQL parser in order to optionally support semicolon at the end of the sql statements
[ https://issues.apache.org/jira/browse/CALCITE-2453?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16574655#comment-16574655 ] charbel yazbeck commented on CALCITE-2453: -- Hi Julia, all your comments have been taken into account and PR adapted. > Enhance the SQL parser in order to optionally support semicolon at the end of > the sql statements > > > Key: CALCITE-2453 > URL: https://issues.apache.org/jira/browse/CALCITE-2453 > Project: Calcite > Issue Type: Improvement > Components: core >Affects Versions: next >Reporter: charbel yazbeck >Assignee: Julian Hyde >Priority: Trivial > Attachments: > 0001-CALCITE-2453-Allowing-SQL-statements-to-optionally-e.patch > > > Consists of adding [] in Parser.jj -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (CALCITE-2453) Enhance the SQL parser in order to optionally support semicolon at the end of the sql statements
[
https://issues.apache.org/jira/browse/CALCITE-2453?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16573851#comment-16573851
]
Julian Hyde commented on CALCITE-2453:
--
Reviewing [PR 783|https://github.com/apache/calcite/pull/783]:
* Can you make {{handleException}} return an exception, that the caller has to
throw, rather than returning void. (I don't like the 'return null;' in the
catch block that will never be reached.)
* Can you change {{checkStmts(String sql, String expected)}} to
{{checkList(String sql, List expected)}}? Tests are clearer if they
contain a list of individual strings.
* Might we worth adding a test where a semicolon occurs inside a comment or
character literal.
* In a sequence of statements, the semicolons between statements are necessary,
but is the semicolon after the last statement necessary? It don't care which,
but please be explicit in the method documentation.
These are minor points; your change looks good overall.
> Enhance the SQL parser in order to optionally support semicolon at the end of
> the sql statements
>
>
> Key: CALCITE-2453
> URL: https://issues.apache.org/jira/browse/CALCITE-2453
> Project: Calcite
> Issue Type: Improvement
> Components: core
>Affects Versions: next
>Reporter: charbel yazbeck
>Assignee: Julian Hyde
>Priority: Trivial
> Attachments:
> 0001-CALCITE-2453-Allowing-SQL-statements-to-optionally-e.patch
>
>
> Consists of adding [] in Parser.jj
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (CALCITE-2453) Enhance the SQL parser in order to optionally support semicolon at the end of the sql statements
[ https://issues.apache.org/jira/browse/CALCITE-2453?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16572960#comment-16572960 ] charbel yazbeck commented on CALCITE-2453: -- Hi Julian, sorry about the patch and the tests, I created the Jira issue too early, before setting my env. I just created a PR taking into account the creation of 2 new productions that you describe. > Enhance the SQL parser in order to optionally support semicolon at the end of > the sql statements > > > Key: CALCITE-2453 > URL: https://issues.apache.org/jira/browse/CALCITE-2453 > Project: Calcite > Issue Type: Improvement > Components: core >Affects Versions: next >Reporter: charbel yazbeck >Assignee: Julian Hyde >Priority: Trivial > Attachments: > 0001-CALCITE-2453-Allowing-SQL-statements-to-optionally-e.patch > > > Consists of adding [] in Parser.jj -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (CALCITE-2453) Enhance the SQL parser in order to optionally support semicolon at the end of the sql statements
[ https://issues.apache.org/jira/browse/CALCITE-2453?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16571936#comment-16571936 ] Julian Hyde commented on CALCITE-2453: -- In future please provide a PR not a patch; you must include tests. CALCITE-2310 proposes a sequence of statements, separated by semicolons. I think that there should be new productions in the parser for 1 a sequence of statements, and 2 a statement followed by a semicolon. Not what you've done here, which is to silently eat up a trailing semicolon. That would fix this and 2310. > Enhance the SQL parser in order to optionally support semicolon at the end of > the sql statements > > > Key: CALCITE-2453 > URL: https://issues.apache.org/jira/browse/CALCITE-2453 > Project: Calcite > Issue Type: Improvement > Components: core >Affects Versions: next >Reporter: charbel yazbeck >Assignee: Julian Hyde >Priority: Trivial > Attachments: > 0001-CALCITE-2453-Allowing-SQL-statements-to-optionally-e.patch > > > Consists of adding [] in Parser.jj -- This message was sent by Atlassian JIRA (v7.6.3#76005)
