You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@zeppelin.apache.org by tinkoff-dwh <gi...@git.apache.org> on 2017/03/19 14:07:35 UTC

[GitHub] zeppelin pull request #2158: [ZEPPELIN-2279] excluded comments from SQL

GitHub user tinkoff-dwh opened a pull request:

    https://github.com/apache/zeppelin/pull/2158

    [ZEPPELIN-2279] excluded comments from SQL

    ### What is this PR for?
    Exclusion comments (single-, multiline) from queries before execution. Comments don't need to execute  query and sometimes there are mistakes. 
    
    
    ### What type of PR is it?
    Bug Fix | Improvement
    
    
    ### What is the Jira issue?
    https://issues.apache.org/jira/browse/ZEPPELIN-2279
    
    ### How should this be tested?
    ```
    /* ; */
    select 1;
    -- text select 1
    /* bla 
    bla
    bla*/
    select 1; -- text
    ```
    
    ### Questions:
    * Does the licenses files need update? no
    * Is there breaking changes for older versions? no
    * Does this needs documentation? no

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/tinkoff-dwh/zeppelin ZEPPELIN-2279

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/zeppelin/pull/2158.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #2158
    
----
commit 6db3c464350cc112f45085bd7d82d7dfdc3e370f
Author: Tinkoff DWH <ti...@gmail.com>
Date:   2017-03-19T07:39:07Z

    [ZEPPELIN-2279] excluded comments from SQL

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zeppelin issue #2158: [ZEPPELIN-2279] excluded comments from SQL

Posted by Leemoonsoo <gi...@git.apache.org>.

Github user Leemoonsoo commented on the issue:

    https://github.com/apache/zeppelin/pull/2158
  
    Merge to master if no further discussions.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zeppelin issue #2158: [ZEPPELIN-2279] excluded comments from SQL

Posted by felixcheung <gi...@git.apache.org>.

Github user felixcheung commented on the issue:

    https://github.com/apache/zeppelin/pull/2158
  
    could you elaborate on what errors would come from comment in SQL?
    generally we should avoid trying to pre-parse the paragraph text since we don't necessarily know the SQL dialect the interpreter can handle, or not


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zeppelin issue #2158: [ZEPPELIN-2279] excluded comments from SQL

Posted by tinkoff-dwh <gi...@git.apache.org>.

Github user tinkoff-dwh commented on the issue:

    https://github.com/apache/zeppelin/pull/2158
  
    @felixcheung 
    I don't see a problem. 
    1. /**/ will not be processed 
    2. /*\n*/ will not be processed 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zeppelin issue #2158: [ZEPPELIN-2279] excluded comments from SQL

Posted by tinkoff-dwh <gi...@git.apache.org>.

Github user tinkoff-dwh commented on the issue:

    https://github.com/apache/zeppelin/pull/2158
  
    longest job is interrupted because of the limit in travis-ci (50 minutes) -> CI red


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zeppelin issue #2158: [ZEPPELIN-2279] excluded comments from SQL

Posted by Leemoonsoo <gi...@git.apache.org>.

Github user Leemoonsoo commented on the issue:

    https://github.com/apache/zeppelin/pull/2158
  
    I fixed Jenkins build command the problem taking wrong commit hash when it's got merge commit.
    Now Jenkins will take correct commits. @tinkoff-dwh Could you try close / re-open this PR and see if Jenkins check the travis correctly?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zeppelin issue #2158: [ZEPPELIN-2279] excluded comments from SQL

Posted by masyan <gi...@git.apache.org>.

Github user masyan commented on the issue:

    https://github.com/apache/zeppelin/pull/2158
  
    @felixcheung 
    falls because ";" is delimiter query and after split generated two queries ("/* ", "*/ select ...")
    
    I relied on the standards for SQL89, 92 ... if you're talking about nosql databases, the jdbc driver are implemented in the form of wrappers (SQL queries, mongodb, aerospike).  
    



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zeppelin issue #2158: [ZEPPELIN-2279] excluded comments from SQL

Posted by Leemoonsoo <gi...@git.apache.org>.

Github user Leemoonsoo commented on the issue:

    https://github.com/apache/zeppelin/pull/2158
  
    @tinkoff-dwh Thanks for contribution.
    
    CI failure on exceeding timelimit has fixed on master branch. Can you try rebase this branch and see if CI becomes green?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zeppelin issue #2158: [ZEPPELIN-2279] excluded comments from SQL

Posted by tinkoff-dwh <gi...@git.apache.org>.

Github user tinkoff-dwh commented on the issue:

    https://github.com/apache/zeppelin/pull/2158
  
    Ready to review


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zeppelin issue #2158: [ZEPPELIN-2279] excluded comments from SQL

Posted by tinkoff-dwh <gi...@git.apache.org>.

Github user tinkoff-dwh commented on the issue:

    https://github.com/apache/zeppelin/pull/2158
  
    @felixcheung 
    example in jira


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zeppelin issue #2158: [ZEPPELIN-2279] excluded comments from SQL

Posted by tinkoff-dwh <gi...@git.apache.org>.

Github user tinkoff-dwh commented on the issue:

    https://github.com/apache/zeppelin/pull/2158
  
    @Leemoonsoo 
    it works, thx!


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zeppelin issue #2158: [ZEPPELIN-2279] excluded comments from SQL

Posted by tinkoff-dwh <gi...@git.apache.org>.

Github user tinkoff-dwh commented on the issue:

    https://github.com/apache/zeppelin/pull/2158
  
    @felixcheung
    falls because ";" is delimiter query and after split generated two queries ("/* ", "*/ select ...")
    
    I relied on the standards for SQL89, 92 ... if you're talking about nosql databases, the jdbc driver are implemented in the form of wrappers (SQL queries, mongodb, aerospike).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zeppelin issue #2158: [ZEPPELIN-2279] excluded comments from SQL

Posted by Leemoonsoo <gi...@git.apache.org>.

Github user Leemoonsoo commented on the issue:

    https://github.com/apache/zeppelin/pull/2158
  
    LGTM \cc @felixcheung 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zeppelin issue #2158: [ZEPPELIN-2279] excluded comments from SQL

Posted by felixcheung <gi...@git.apache.org>.

Github user felixcheung commented on the issue:

    https://github.com/apache/zeppelin/pull/2158
  
    that's the thing, each of these implementation handles comments differently.
    for instance, solr JDBC shares the same spec as [presto](https://github.com/prestodb/presto/blob/master/presto-parser/src/main/antlr4/com/facebook/presto/sql/parser/SqlBase.g4) (which is very common) - but it doesn't support empty comment `/**/` or multiline comment `/*\n*/`


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] zeppelin issue #2158: [ZEPPELIN-2279] excluded comments from SQL

Posted by tinkoff-dwh <gi...@git.apache.org>.

Github user tinkoff-dwh commented on the issue:

    https://github.com/apache/zeppelin/pull/2158
  
    @Leemoonsoo 
    done


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---