[Zeppelin-628 ] Fix parse propertyKey in interpreter name for JDBC #667

vgmartinez · 2016-01-22T16:51:45Z

What is this PR for?

Fix bug
https://issues.apache.org/jira/browse/ZEPPELIN-628

Todos

How should this be tested?

run a query that contains (something)...eg

%jdbc
select max(ss_promo_sk), ss_customer_sk from qhive.tpcds_orc_500.store_sales where ss_sold_date_sk >= 2452640 and ss_customer_sk > 3 and ss_customer_sk < 20 group by ss_customer_sk

It is ok if the propertyKey is default:

PropertyKey: default, SQL command: 'select max(ss_promo_sk), ss_customer_sk from qhive.tpcds_orc_500.store_sales where ss_sold_date_sk >= 2452640 and ss_customer_sk > 3 and ss_customer_sk < 20 group by ss_customer_sk'

Questions:

Does the licenses files need update? no
Is there breaking changes for older versions? no
Does this needs documentation? no

felixcheung · 2016-01-22T21:21:48Z

thanks for addressing this. could we add some tests for this?

vgmartinez · 2016-01-22T22:06:57Z

Hi @felixcheung, thanks for review...
add the test, is enough? or you think is necessary something more?

felixcheung · 2016-01-22T23:44:27Z

jdbc/src/main/java/org/apache/zeppelin/jdbc/JDBCInterpreter.java

maybe add trim() because the string could be %jdbc (redshift) select * from...

I think it is not necessary because when I write this query the interpreter what comes is not the name of the interpreter...eg

%jdbc (redshift) select * from...

to string is applied trim in paragraph class...and the interpreter receives this way without spaces

(redshift) select * from...

and finally on line 353 is applied trim to the query....what do you think?

This presupposes that the end-user will be using the first line as a declaration of the interpreter:

%jdbc (redshift)\n

What about in cases where the user has selected a default interpreter for each paragraph and doesn't need to specify the %<interpreter> prefix? When a user does that then any query with a function will error.

Oops, I see you're checking for startsWith, nevermind.

felixcheung · 2016-01-22T23:46:08Z

in additional to getPropertyKey() is there a place where it parses the rest of the query? that might need to be changed too?

vgmartinez · 2016-01-23T10:42:24Z

I think there is no to change any more...I'm sorry for my english...;)

felixcheung · 2016-01-24T00:27:12Z

ok, I'll try to test this out a bit in 1-2 days

vgmartinez · 2016-01-31T10:28:48Z

Hi @felixcheung, could you check the bug...?

felixcheung · 2016-02-01T07:32:25Z

I've tested it a bit, it seems to work - to reproduce the bug, do I need to have multiple, or valid propertyKey?

For example, I type a random one and it does not complain:

%jdbc (blah)
select max(code) as ok, min(title) as t from films2 group by code, did,  kind;

vgmartinez · 2016-02-01T22:13:12Z

the problem was when they used the in this way %jdbc and had the query something like this select max(code) as ok, min(title) as t from films2 group by code, did, kind
Because trying to parse what was within bracket.
I think is ready...

felixcheung · 2016-02-02T05:27:16Z

jdbc/src/main/java/org/apache/zeppelin/jdbc/JDBCInterpreter.java

Is it a problem if

%jdbc)default(

Or

%jdbc()

Or

%jdbc( select ....

Or

%jdbc( default) select ... ?

Would you be able to address these cases? @vgmartinez

felixcheung · 2016-02-02T05:30:51Z

Tested the current broken behavior, thanks for fixing this.
Added a comment on parsing.
Also, I think it's an issue when the return value from getPropertyKey is invalid, like %jdbc (blah) but that could be fixed separately.

jongyoul · 2016-02-02T15:12:34Z

@felixcheung I don't think current implementation is good. but I think it's better handle it with different issue. Does it make sense?

felixcheung · 2016-02-06T19:12:00Z

property key not matching a valid profile is probably a fix would be great to make separately - would you be interested in that, @vgmartinez ?
let's fix the parsing and merge this, since I think it is affecting a bunch of users.

vgmartinez · 2016-02-06T19:45:04Z

Hi @felixcheung @jongyoul
I propose message of error when user set a not valid prefix, similar to when interpreter not found...I think is responsibility of user set a valid prefix...

something like this:

the problem here is that you always have to put the prefix in default or not...
what do you think guys...?

jongyoul · 2016-02-09T05:01:06Z

@vgmartinez I think we need to support default interpreter without setting any default word. In other words, users should be able to use default jdbc by typing just %jdbc.

felixcheung · 2016-02-10T08:25:22Z

agreed, I think %jdbc should work.
can't we check only when a prefix is there? say %jdbc() or %jdbc (something_invalid) should both fail, but %jdbc is ok?

vgmartinez · 2016-02-10T08:40:33Z

Hi @felixcheung @jongyoul
I tested it and looks like this:

I rebase and push...review please...;)

jongyoul · 2016-02-10T13:45:37Z

@vgmartinez it looks like that you merge master into your branch. Please check it and rebase your PR from master again.

vgmartinez · 2016-02-10T14:06:05Z

@jongyoul...Done

jongyoul · 2016-02-10T16:29:07Z

jdbc/src/test/java/org/apache/zeppelin/jdbc/JDBCInterpreterTest.java

+    assertEquals(t.getPropertyKey("(redshift)\n select max(cant) from test_table where id >= 2452640"),
+        "redshift");
+  }
+


How about adding what felix mentioned into test cases?

ok...add more tests cases....

vgmartinez · 2016-02-10T17:23:00Z

@jongyoul
I have added more tests....

felixcheung · 2016-02-16T03:53:30Z

@vgmartinez could you rebase to latest on master to get Travis tests to run again?

jongyoul · 2016-02-16T04:09:28Z

jdbc/src/test/java/org/apache/zeppelin/jdbc/JDBCInterpreterTest.java

+
+    // if return null is that prefix not found
+    assertEquals(t.getPropertyKey("(fake) select max(cant) from test_table where id >= 2452640"),
+        null);


@vgmartinez I saw the failure and found out the below:

------------------------------------------------------- T E S T S ------------------------------------------------------- Running org.apache.zeppelin.jdbc.JDBCInterpreterTest SLF4J: Class path contains multiple SLF4J bindings. SLF4J: Found binding in [jar:file:/home/travis/build/apache/incubator-zeppelin/zeppelin-interpreter/target/zeppelin-interpreter-0.6.0-incubating-SNAPSHOT.jar!/org/slf4j/impl/StaticLoggerBinder.class] SLF4J: Found binding in [jar:file:/home/travis/build/apache/incubator-zeppelin/zeppelin-interpreter/target/lib/slf4j-log4j12-1.7.10.jar!/org/slf4j/impl/StaticLoggerBinder.class] SLF4J: Found binding in [jar:file:/home/travis/.m2/repository/org/slf4j/slf4j-log4j12/1.7.10/slf4j-log4j12-1.7.10.jar!/org/slf4j/impl/StaticLoggerBinder.class] SLF4J: See http://www.slf4j.org/codes.html#multiple_bindings for an explanation. SLF4J: Actual binding is of type [org.slf4j.impl.Log4jLoggerFactory] log4j:WARN No appenders could be found for logger (org.apache.zeppelin.jdbc.JDBCInterpreter). log4j:WARN Please initialize the log4j system properly. log4j:WARN See http://logging.apache.org/log4j/1.2/faq.html#noconfig for more info. Tests run: 4, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.474 sec <<< FAILURE! - in org.apache.zeppelin.jdbc.JDBCInterpreterTest testForParsePropertyKey(org.apache.zeppelin.jdbc.JDBCInterpreterTest) Time elapsed: 0.339 sec <<< FAILURE! java.lang.AssertionError: expected:<fake> but was:<null> at org.junit.Assert.fail(Assert.java:88) at org.junit.Assert.failNotEquals(Assert.java:743) at org.junit.Assert.assertEquals(Assert.java:118) at org.junit.Assert.assertEquals(Assert.java:144) at org.apache.zeppelin.jdbc.JDBCInterpreterTest.testForParsePropertyKey(JDBCInterpreterTest.java:78)

It needs to fix the test case. It should return fake actually.

vgmartinez · 2016-02-16T09:12:51Z

hi @felixcheung @jongyoul...I have added more test and fix the other

jongyoul · 2016-02-16T16:53:30Z

LGTM

felixcheung · 2016-02-16T21:58:23Z

looks good, merging if no more comments.

### What is this PR for? Fix bug https://issues.apache.org/jira/browse/ZEPPELIN-628 ### Todos ### How should this be tested? run a query that contains (something)...eg ``` %jdbc select max(ss_promo_sk), ss_customer_sk from qhive.tpcds_orc_500.store_sales where ss_sold_date_sk >= 2452640 and ss_customer_sk > 3 and ss_customer_sk < 20 group by ss_customer_sk ``` It is ok if the **propertyKey** is default: ``` PropertyKey: default, SQL command: 'select max(ss_promo_sk), ss_customer_sk from qhive.tpcds_orc_500.store_sales where ss_sold_date_sk >= 2452640 and ss_customer_sk > 3 and ss_customer_sk < 20 group by ss_customer_sk' ``` ### Questions: Does the licenses files need update? no Is there breaking changes for older versions? no Does this needs documentation? no Author: vgmartinez <viktor.manuel.garcia@gmail.com> Closes apache#667 from vgmartinez/bug_628 and squashes the following commits: 4859cac [vgmartinez] fix test for parse propertyKey 810c14e [vgmartinez] add more tests for parse prefix 9d59c60 [vgmartinez] fixed parse properties

vgmartinez changed the title ~~Zeppelin-628 ] Fix parse propertyKey in interpreter name~~ [Zeppelin-628 ] Fix parse propertyKey in interpreter name for JDBC Jan 22, 2016

felixcheung reviewed Jan 22, 2016
View reviewed changes

felixcheung reviewed Feb 2, 2016
View reviewed changes

jongyoul mentioned this pull request Feb 9, 2016

Zeppelin 639 #684

Closed

vgmartinez force-pushed the bug_628 branch from bde087f to ba97e3f Compare February 10, 2016 14:05

jongyoul reviewed Feb 10, 2016
View reviewed changes

jongyoul reviewed Feb 16, 2016
View reviewed changes

vgmartinez added 2 commits February 16, 2016 09:28

fixed parse properties

9d59c60

add more tests for parse prefix

810c14e

vgmartinez force-pushed the bug_628 branch from ffc1135 to 810c14e Compare February 16, 2016 08:29

fix test for parse propertyKey

4859cac

felixcheung mentioned this pull request Feb 16, 2016

[Zeppelin-628 ] Fix parse propertyKey in interpreter name for Hive #668

Closed

asfgit closed this in a283dfa Feb 17, 2016

[Zeppelin-628 ] Fix parse propertyKey in interpreter name for JDBC #667

[Zeppelin-628 ] Fix parse propertyKey in interpreter name for JDBC #667

Uh oh!

Conversation

vgmartinez commented Jan 22, 2016

What is this PR for?

Todos

How should this be tested?

Questions:

Uh oh!

felixcheung commented Jan 22, 2016

Uh oh!

vgmartinez commented Jan 22, 2016

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!

felixcheung commented Jan 22, 2016

Uh oh!

vgmartinez commented Jan 23, 2016

Uh oh!

felixcheung commented Jan 24, 2016

Uh oh!

vgmartinez commented Jan 31, 2016

Uh oh!

felixcheung commented Feb 1, 2016

Uh oh!

vgmartinez commented Feb 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixcheung commented Feb 2, 2016

Uh oh!

jongyoul commented Feb 2, 2016

Uh oh!

felixcheung commented Feb 6, 2016

Uh oh!

vgmartinez commented Feb 6, 2016

Uh oh!

jongyoul commented Feb 9, 2016

Uh oh!

felixcheung commented Feb 10, 2016

Uh oh!

vgmartinez commented Feb 10, 2016

Uh oh!

jongyoul commented Feb 10, 2016

Uh oh!

vgmartinez commented Feb 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vgmartinez commented Feb 10, 2016

Uh oh!

felixcheung commented Feb 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vgmartinez commented Feb 16, 2016

Uh oh!

jongyoul commented Feb 16, 2016

Uh oh!

felixcheung commented Feb 16, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants