[ZEPPELIN-2279] excluded comments from SQL by tinkoff-dwh · Pull Request #2158 · apache/zeppelin

tinkoff-dwh · 2017-03-19T14:07:29Z

What is this PR for?

Exclusion comments (single-, multiline) from queries before execution. Comments don't need to execute query and sometimes there are errors.

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

tinkoff-dwh · 2017-03-19T14:11:47Z

biggest job is interrupted because of the limit in travis-ci (50 minutes) -> CI red

Leemoonsoo · 2017-03-20T16:47:54Z

@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?

tinkoff-dwh · 2017-03-20T18:34:14Z

@Leemoonsoo
done

Leemoonsoo · 2017-03-20T18:49:20Z

Looks like somehow this branch includes commits not part of this contribution.

If you rebase / merge correctly, they'll not be seen as commit of this PR. Could you reset to 6db3c46
and try again?

tinkoff-dwh · 2017-03-20T19:22:49Z

@Leemoonsoo
fix it.
CI green, https://travis-ci.org/tinkoff-dwh/zeppelin/builds/213118918

Leemoonsoo · 2017-03-20T21:52:48Z

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?

felixcheung · 2017-03-21T03:42:54Z

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

tinkoff-dwh · 2017-03-21T04:36:15Z

@felixcheung
example in jira

tinkoff-dwh · 2017-03-21T04:38:33Z

@Leemoonsoo
it works, thx!

Leemoonsoo · 2017-03-22T23:16:32Z

LGTM
Merge to master if no further comments.

felixcheung · 2017-03-23T02:13:04Z

Actually I want to clarify... Do you know why the specific example fails? The Postgres spec is pretty clear that it is an accept form of comment. https://www.postgresql.org/docs/8.0/static/sql-syntax.html#SQL-SYNTAX-COMMENTS Again my point is we can't possibly know what is and is not supported by all the possible JDBC sources - we should avoid hardcoding the logic here. #2158 (comment)

tinkoff-dwh · 2017-03-23T05:42:34Z

@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).

felixcheung · 2017-03-23T06:27:22Z

that's the thing, each of these implementation handles comments differently.
for instance, solr JDBC shares the same spec as presto (which is very common) - but it doesn't support empty comment /**/ or multiline comment /*\n*/

felixcheung · 2017-03-23T05:52:27Z

    if (StringUtils.isNotBlank(precode)) {
-      precode = StringUtils.trim(precode);
-      logger.info("Run SQL precode '{}'", precode);
+      List<String> precodeQueries = splitSqlQueries(precode);


why do we need to split this up?

Yes, there is no need to

felixcheung · 2017-03-23T06:28:20Z

+    t.open();
+
+    String sqlQuery = "/* ; */\n" +
+        "--select * from test_table\n" +


add a test for -- a ; b?

tinkoff-dwh · 2017-03-23T06:48:56Z

@felixcheung
I don't see a problem.

/**/ will not be processed
/\n/ will not be processed

tinkoff-dwh · 2017-03-29T05:48:27Z

Ready to review

Leemoonsoo · 2017-04-03T02:10:43Z

LGTM \cc @felixcheung

Leemoonsoo · 2017-04-04T20:11:23Z

Merge to master if no further discussions.

felixcheung · 2017-04-05T05:57:28Z

      character = sql.charAt(item);

+      if ((singleLineComment && (character.equals('\n') || item == sql.length() - 1))
+          || (multiLineComment && character.equals('/') && sql.charAt(item - 1) == '*')) {


should check if item == 0, for example

/* multi line / */

this is going to call sql.charAt(0 - 1)?

why sometimes character.equals() and sometimes charAt() ==?

item > 0 because first condition should be multiLineComment == true

conditions fixed

felixcheung · 2017-04-05T06:05:32Z

+        if (character.equals('/') && sql.charAt(item + 1) == '*') {
+          multiLineComment = true;
+          continue;
+        }


what about

-- /* comment select ...

is that a single line or multiline?

single because singleLineComment == true

if (singleLineComment || multiLineComment) { continue; }

### What is this PR for? Exclusion comments (single-, multiline) from queries before execution. Comments don't need to execute query and sometimes there are errors. ### 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 Author: Tinkoff DWH <tinkoff.dwh@gmail.com> Closes apache#2158 from tinkoff-dwh/ZEPPELIN-2279 and squashes the following commits: 3f7496e [Tinkoff DWH] [ZEPPELIN-2279] fix conditions, common format f48f7d6 [Tinkoff DWH] [ZEPPELIN-2279] improve test, revert precode execution 2cb94fa [Tinkoff DWH] Merge remote-tracking branch 'origin/master' into ZEPPELIN-2279 6db3c46 [Tinkoff DWH] [ZEPPELIN-2279] excluded comments from SQL

### What is this PR for? The original purpose of #2158 was correct processing of ';'. This was done via full removing comments from code. Unfortunately Apache Phoenix uses hooks in comments https://forcedotcom.github.io/phoenix/#hintml. Thus we should not delete comments in scripts. There was discussion about comment rules for different databases (solr). The right way is keep style. Thus analysts can copy queries to another tool and can get same results and errors. ### What type of PR is it? [Bug Fix] ### What is the Jira issue? * https://issues.apache.org/jira/browse/ZEPPELIN-3344 ### How should this be tested? * Unit tests pass: testSplitSqlQueryWithComments and testSplitSqlQuery ### Questions: * Does the licenses files need update? No * Is there breaking changes for older versions? No * Does this needs documentation? No Author: mebelousov <mebelousov@ya.ru> Closes #2876 from mebelousov/ZEPPELIN-3344 and squashes the following commits: 6980400 [mebelousov] ZEPPELIN-3344 Fix checkstyle, add tests for comments 83c8e8f [mebelousov] ZEPPELIN-3344 Rename test eed54c8 [mebelousov] ZEPPELIN-3344 Revert comments in JDBC interpreter (cherry picked from commit 8238b71) Signed-off-by: Jeff Zhang <zjffdu@apache.org>

### What is this PR for? The original purpose of #2158 was correct processing of ';'. This was done via full removing comments from code. Unfortunately Apache Phoenix uses hooks in comments https://forcedotcom.github.io/phoenix/#hintml. Thus we should not delete comments in scripts. There was discussion about comment rules for different databases (solr). The right way is keep style. Thus analysts can copy queries to another tool and can get same results and errors. ### What type of PR is it? [Bug Fix] ### What is the Jira issue? * https://issues.apache.org/jira/browse/ZEPPELIN-3344 ### How should this be tested? * Unit tests pass: testSplitSqlQueryWithComments and testSplitSqlQuery ### Questions: * Does the licenses files need update? No * Is there breaking changes for older versions? No * Does this needs documentation? No Author: mebelousov <mebelousov@ya.ru> Closes #2876 from mebelousov/ZEPPELIN-3344 and squashes the following commits: 6980400 [mebelousov] ZEPPELIN-3344 Fix checkstyle, add tests for comments 83c8e8f [mebelousov] ZEPPELIN-3344 Rename test eed54c8 [mebelousov] ZEPPELIN-3344 Revert comments in JDBC interpreter

### What is this PR for? The original purpose of apache#2158 was correct processing of ';'. This was done via full removing comments from code. Unfortunately Apache Phoenix uses hooks in comments https://forcedotcom.github.io/phoenix/#hintml. Thus we should not delete comments in scripts. There was discussion about comment rules for different databases (solr). The right way is keep style. Thus analysts can copy queries to another tool and can get same results and errors. ### What type of PR is it? [Bug Fix] ### What is the Jira issue? * https://issues.apache.org/jira/browse/ZEPPELIN-3344 ### How should this be tested? * Unit tests pass: testSplitSqlQueryWithComments and testSplitSqlQuery ### Questions: * Does the licenses files need update? No * Is there breaking changes for older versions? No * Does this needs documentation? No Author: mebelousov <mebelousov@ya.ru> Closes apache#2876 from mebelousov/ZEPPELIN-3344 and squashes the following commits: 6980400 [mebelousov] ZEPPELIN-3344 Fix checkstyle, add tests for comments 83c8e8f [mebelousov] ZEPPELIN-3344 Rename test eed54c8 [mebelousov] ZEPPELIN-3344 Revert comments in JDBC interpreter

[ZEPPELIN-2279] excluded comments from SQL

6db3c46

tinkoff-dwh force-pushed the ZEPPELIN-2279 branch from 1295a74 to 6db3c46 Compare March 20, 2017 18:57

Merge remote-tracking branch 'origin/master' into ZEPPELIN-2279

2cb94fa

tinkoff-dwh closed this Mar 21, 2017

tinkoff-dwh reopened this Mar 21, 2017

felixcheung reviewed Mar 23, 2017

View reviewed changes

[ZEPPELIN-2279] improve test, revert precode execution

f48f7d6

felixcheung requested changes Apr 5, 2017

View reviewed changes

[ZEPPELIN-2279] fix conditions, common format

3f7496e

felixcheung approved these changes Apr 6, 2017

View reviewed changes

asfgit closed this in f9830a7 Apr 8, 2017

tinkoff-dwh deleted the ZEPPELIN-2279 branch April 17, 2017 09:47

mebelousov mentioned this pull request Mar 16, 2018

ZEPPELIN-3344. Revert comments in queries in JDBC interpreter #2876

Closed

Conversation

tinkoff-dwh commented Mar 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is this PR for?

What type of PR is it?

What is the Jira issue?

How should this be tested?

Questions:

Uh oh!

tinkoff-dwh commented Mar 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Leemoonsoo commented Mar 20, 2017

Uh oh!

tinkoff-dwh commented Mar 20, 2017

Uh oh!

Leemoonsoo commented Mar 20, 2017

Uh oh!

tinkoff-dwh commented Mar 20, 2017

Uh oh!

Leemoonsoo commented Mar 20, 2017

Uh oh!

felixcheung commented Mar 21, 2017

Uh oh!

tinkoff-dwh commented Mar 21, 2017

Uh oh!

tinkoff-dwh commented Mar 21, 2017

Uh oh!

Leemoonsoo commented Mar 22, 2017

Uh oh!

felixcheung commented Mar 23, 2017 via email

Uh oh!

tinkoff-dwh commented Mar 23, 2017

Uh oh!

felixcheung commented Mar 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tinkoff-dwh commented Mar 23, 2017

Uh oh!

tinkoff-dwh commented Mar 29, 2017

Uh oh!

Leemoonsoo commented Apr 3, 2017

Uh oh!

Leemoonsoo commented Apr 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tinkoff-dwh Apr 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tinkoff-dwh commented Mar 19, 2017 •

edited

Loading

tinkoff-dwh commented Mar 19, 2017 •

edited

Loading

tinkoff-dwh Apr 5, 2017 •

edited

Loading