Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
vgmartinez wants to merge 3 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9d59c60
fixed parse properties
vgmartinez Feb 10, 2016
810c14e
add more tests for parse prefix
vgmartinez Feb 10, 2016
4859cac
fix test for parse propertyKey
vgmartinez Feb 16, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 26 additions & 14 deletions jdbc/src/main/java/org/apache/zeppelin/jdbc/JDBCInterpreter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@

import org.apache.zeppelin.interpreter.Interpreter;
import org.apache.zeppelin.interpreter.InterpreterContext;
import org.apache.zeppelin.interpreter.InterpreterException;
import org.apache.zeppelin.interpreter.InterpreterPropertyBuilder;
import org.apache.zeppelin.interpreter.InterpreterResult;
import org.apache.zeppelin.interpreter.InterpreterResult.Code;
Expand Down Expand Up @@ -172,6 +173,9 @@ public void open() {

public Connection getConnection(String propertyKey) throws ClassNotFoundException, SQLException {
Connection connection = null;
if (propertyKey == null || propertiesMap.get(propertyKey) == null) {
return null;
}
if (propertyKeyUnusedConnectionListMap.containsKey(propertyKey)) {
ArrayList<Connection> connectionList = propertyKeyUnusedConnectionListMap.get(propertyKey);
if (0 != connectionList.size()) {
Expand Down Expand Up @@ -206,6 +210,10 @@ public Statement getStatement(String propertyKey, String paragraphId)
} else {
connection = getConnection(propertyKey);
}

if (connection == null) {
return null;
}

Statement statement = connection.createStatement();
if (isStatementClosed(statement)) {
Expand Down Expand Up @@ -260,6 +268,10 @@ private InterpreterResult executeSql(String propertyKey, String sql,
try {

Statement statement = getStatement(propertyKey, paragraphId);

if (statement == null) {
return new InterpreterResult(Code.ERROR, "Prefix not found.");
}
statement.setMaxRows(getMaxResult());

StringBuilder msg = null;
Expand Down Expand Up @@ -344,12 +356,10 @@ public InterpreterResult interpret(String cmd, InterpreterContext contextInterpr
logger.info("Run SQL command '{}'", cmd);
String propertyKey = getPropertyKey(cmd);

if (null != propertyKey) {
if (null != propertyKey && !propertyKey.equals(DEFAULT_KEY)) {
cmd = cmd.substring(propertyKey.length() + 2);
} else {
propertyKey = DEFAULT_KEY;
}

cmd = cmd.trim();

logger.info("PropertyKey: {}, SQL command: '{}'", propertyKey, cmd);
Expand All @@ -371,17 +381,19 @@ public void cancel(InterpreterContext context) {
}

public String getPropertyKey(String cmd) {
int firstLineIndex = cmd.indexOf("\n");
if (-1 == firstLineIndex) {
firstLineIndex = cmd.length();
}
int configStartIndex = cmd.indexOf("(");
int configLastIndex = cmd.indexOf(")");
if (configStartIndex != -1 && configLastIndex != -1
&& configLastIndex < firstLineIndex && configLastIndex < firstLineIndex) {
return cmd.substring(configStartIndex + 1, configLastIndex);
boolean firstLineIndex = cmd.startsWith("(");
Copy link
Member

@felixcheung felixcheung Jan 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@vgmartinez vgmartinez Jan 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

@cjmatta cjmatta Jan 26, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@cjmatta cjmatta Jan 26, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


if (firstLineIndex) {
int configStartIndex = cmd.indexOf("(");
int configLastIndex = cmd.indexOf(")");
if (configStartIndex != -1 && configLastIndex != -1) {
return cmd.substring(configStartIndex + 1, configLastIndex);
Copy link
Member

@felixcheung felixcheung Feb 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it a problem if

%jdbc)default(

Or

%jdbc()

Or

%jdbc(
select ....

Or

%jdbc(
default)
select ...
?

Copy link
Member

@felixcheung felixcheung Feb 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you be able to address these cases? @vgmartinez

} else {
return null;
}
} else {
return DEFAULT_KEY;
}
return null;
}

@Override
Expand Down
47 changes: 47 additions & 0 deletions jdbc/src/test/java/org/apache/zeppelin/jdbc/JDBCInterpreterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,53 @@ public void setUp() throws Exception {
);
}


@Test
public void testForParsePropertyKey() throws IOException {
JDBCInterpreter t = new JDBCInterpreter(new Properties());

assertEquals(t.getPropertyKey("(fake) select max(cant) from test_table where id >= 2452640"),
"fake");

assertEquals(t.getPropertyKey("() select max(cant) from test_table where id >= 2452640"),
"");

assertEquals(t.getPropertyKey(")fake( select max(cant) from test_table where id >= 2452640"),
"default");

// when you use a %jdbc(prefix1), prefix1 is the propertyKey as form part of the cmd string
assertEquals(t.getPropertyKey("(prefix1)\n select max(cant) from test_table where id >= 2452640"),
"prefix1");

assertEquals(t.getPropertyKey("(prefix2) select max(cant) from test_table where id >= 2452640"),
"prefix2");

// when you use a %jdbc, prefix is the default
assertEquals(t.getPropertyKey("select max(cant) from test_table where id >= 2452640"),
"default");
}

@Test
public void testForMapPrefix() throws SQLException, IOException {
Properties properties = new Properties();
properties.setProperty("common.max_count", "1000");
properties.setProperty("common.max_retry", "3");
properties.setProperty("default.driver", "org.h2.Driver");
properties.setProperty("default.url", getJdbcConnection());
properties.setProperty("default.user", "");
properties.setProperty("default.password", "");
JDBCInterpreter t = new JDBCInterpreter(properties);
t.open();

String sqlQuery = "(fake) select * from test_table";

InterpreterResult interpreterResult = t.interpret(sqlQuery, new InterpreterContext("", "1", "","", null,null,null,null,null,null));

// if prefix not found return ERROR and Prefix not found.
assertEquals(InterpreterResult.Code.ERROR, interpreterResult.code());
assertEquals("Prefix not found.", interpreterResult.message());
}

Copy link
Member

@jongyoul jongyoul Feb 10, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about adding what felix mentioned into test cases?

Copy link
Contributor Author

@vgmartinez vgmartinez Feb 10, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Test
public void testDefaultProperties() throws SQLException {
JDBCInterpreter jdbcInterpreter = new JDBCInterpreter(new Properties());
Expand Down