Skip to content
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.

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

Fixed internal error in pinot queries caused due to question mark (?) in query parameter. #73

Merged
merged 5 commits into from
Apr 25, 2021
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -81,13 +81,19 @@ private PinotClient(String pathType, String path) {
}

public ResultSetGroup executeQuery(String statement, Params params) {
/*
PreparedStatement preparedStatement = buildPreparedStatement(statement, params);
return preparedStatement.execute();
*/
return connection.execute(new Request(SQL_FORMAT, resolveStatement(statement, params)));
}

public Future<ResultSetGroup> executeQueryAsync(String statement, Params params) {
/*
PreparedStatement preparedStatement = buildPreparedStatement(statement, params);
return preparedStatement.executeAsync();
*/
return connection.executeAsync(new Request(SQL_FORMAT, resolveStatement(statement, params)));
}

private PreparedStatement buildPreparedStatement(String statement, Params params) {
Expand All @@ -102,5 +108,47 @@ private PreparedStatement buildPreparedStatement(String statement, Params params
.forEach((i, b) -> preparedStatement.setString(i, Hex.encodeHexString(b.toByteArray())));
return preparedStatement;
}

@VisibleForTesting
/*
* Pinot PreparedStatement creates invalid query if one of the parameters has '?' in its value.

Choose a reason for hiding this comment

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

nice documentation 💯

* Sample pinot query : select * from table where team in (?, ?, ?).. Now say parameters are:
* 'abc', 'pqr with (?)' and 'xyz'..
*
* Now, on executing PreparedStatement:fillStatementWithParameters method on this will return
* select * from table where team in ('abc', 'pqr with ('xyz')', ?) -- which is clearly wrong
* (what we wanted was select * from table where team in ('abc', 'pqr with (?)', 'xyz'))..
*
* The reason is the usage of replaceFirst iteration in the pinot PreparedStatement method..
*
* Hence written this custom method to resolve the query statement rather than relying on Pinot's
* library method.
* This is a temporary fix and can be reverted when the Pinot issue gets resolved
* Raised an issue in incubator-pinot github repo: apache/incubator-pinot#6834
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting find!

*/
String resolveStatement(String query, Params params) {
singhalprerana marked this conversation as resolved.
Show resolved Hide resolved

Choose a reason for hiding this comment

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

What happens in this method when the number of ? don't match the number of params?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if number of params are more, we drop the extras and if it's less, we replace ? with empty string.. the loop takes care of this..

String[] queryParts = query.split("\\?");
singhalprerana marked this conversation as resolved.
Show resolved Hide resolved

String[] parameters = new String[queryParts.length];
params.getStringParams().forEach((i, p) -> parameters[i] = getStringParam(p));
params.getIntegerParams().forEach((i, p) -> parameters[i] = String.valueOf(p));
params.getLongParams().forEach((i, p) -> parameters[i] = String.valueOf(p));
params.getDoubleParams().forEach((i, p) -> parameters[i] = String.valueOf(p));
params.getFloatParams().forEach((i, p) -> parameters[i] = String.valueOf(p));
params
.getByteStringParams()
.forEach((i, p) -> parameters[i] = getStringParam(Hex.encodeHexString(p.toByteArray())));

StringBuilder sb = new StringBuilder();
for (int i = 0; i < queryParts.length; i++) {
sb.append(queryParts[i]);
sb.append(parameters[i] != null ? parameters[i] : "");
}
return sb.toString();
}

private String getStringParam(String value) {
singhalprerana marked this conversation as resolved.
Show resolved Hide resolved
return "'" + value.replace("'", "''") + "'";
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,4 +22,19 @@ public void testPinotQuery() {
Assertions.assertEquals(q2, q3);
Assertions.assertEquals(q3, q2);
}

@Test
public void testResolveStatement() {
PinotClientFactory.PinotClient pinotClient = new PinotClientFactory.PinotClient(null);
String statement =
pinotClient.resolveStatement(
"select * from table where team in (?, ?, ?)",
Params.newBuilder()
.addStringParam("abc")
.addStringParam("pqr with (?)")
.addStringParam("xyz")
.build());

Assertions.assertEquals("select * from table where team in ('abc', 'pqr with (?)', 'xyz')", statement);
}
}