SOLR-15576: Allow filtering on ISO-8601 formatted timestamp literals in SQL WHERE clause #247
Conversation
…in SQL WHERE clause
| terms = "\"" + terms + "\""; | ||
| wrappedQuotes = true; |
There was a problem hiding this comment.
Out of scope here, but #236 has me thinking about what if there are quotes inside of the terms.
There was a problem hiding this comment.
if terms has a quote, the query breaks ... I think we should just fix this here vs. opening another PR though
There was a problem hiding this comment.
@madrob This is a good catch and I just fixed in this PR but could break out to its own if you feel strongly about not fixing it here ;-)
There was a problem hiding this comment.
I was imagining that it would be more work than it ended up actually being
| @@ -283,8 +283,9 @@ public void open() throws IOException { | |||
| resultSet = statement.executeQuery(sqlQuery); | |||
| resultSet.setFetchSize(fetchSize); | |||
| } catch (SQLException e) { | |||
| throw new IOException(String.format(Locale.ROOT, "Failed to execute sqlQuery '%s' against JDBC connection '%s'.\n" | |||
| + e.getMessage(), sqlQuery, connectionUrl), e); | |||
| // don't embed the exception message in the format template as wildcard '%' will throw off the formatter | |||
There was a problem hiding this comment.
I'm confused here, why does exception message have a %, but sqlQuery wouldn't?
edit: I'm not saying the code is wrong, but the comment could be little more clear
There was a problem hiding this comment.
No actually you're right, if the SQL has a %, then it will confuse the formatter too. This was just something I found while debugging another issue and it looked like it was the exception message that caused the issue but the sql query would too. Will escape any percents in the query / error message with %% and add a test case
There was a problem hiding this comment.
Wait, no ... now I'm confusing myself ;-) The reason the exception message and not the sqlQuery was messing up the formatter is the code (before I changed it) was appending the exception message to the formatter template whereas the sqlQuery is injected as a %s parameter. I think the better fix here is to just inject the exception message as a %s parameter as well
…in SQL WHERE clause (apache#247)
https://issues.apache.org/jira/browse/SOLR-15576
Description
Minor improvement for robustness of the SQL backend to allow filtering with ISO-8601 timestamp formatted literals.
Solution
Just needed to fix the proper handling of RexNode types in the translateBinary call. Also found a bug where the left and right operands were being swapped but that's only valid for equals operator. I mean we could also adapt the operator to the swap but that seems awkward, so leaving that as an open issue for now.
Tests
Added new unit test
Checklist
Please review the following and check all that apply:
mainbranch../gradlew check.