You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2021/08/03 20:23:12 UTC
[GitHub] [solr] thelabdude opened a new pull request #247: SOLR-15576: Allow filtering on ISO-8601 formatted timestamp literals in SQL WHERE clause
thelabdude opened a new pull request #247:
URL: https://github.com/apache/solr/pull/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
# Tests
Added new unit test
# Checklist
Please review the following and check all that apply:
- [ ] I have reviewed the guidelines for [How to Contribute](https://wiki.apache.org/solr/HowToContribute) and my code conforms to the standards described there to the best of my ability.
- [ ] I have created a Jira issue and added the issue ID to my pull request title.
- [ ] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
- [ ] I have developed this patch against the `main` branch.
- [ ] I have run `./gradlew check`.
- [ ] I have added tests for my changes.
- [ ] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] thelabdude commented on a change in pull request #247: SOLR-15576: Allow filtering on ISO-8601 formatted timestamp literals in SQL WHERE clause
Posted by GitBox <gi...@apache.org>.
thelabdude commented on a change in pull request #247:
URL: https://github.com/apache/solr/pull/247#discussion_r682995862
##########
File path: solr/core/src/java/org/apache/solr/handler/sql/SolrFilter.java
##########
@@ -278,6 +265,24 @@ protected String translateComparison(RexNode node) {
}
}
+ private String toEqualsClause(String key, RexLiteral value, RexNode node) {
+ SqlTypeName fieldTypeName = ((RexCall) node).getOperands().get(0).getType().getSqlTypeName();
+ String terms = toSolrLiteralForEquals(value, fieldTypeName).trim();
+
+ boolean wrappedQuotes = false;
+ if (!terms.startsWith("(") && !terms.startsWith("[") && !terms.startsWith("{")) {
+ terms = "\"" + terms + "\"";
+ wrappedQuotes = true;
Review 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 ;-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] thelabdude commented on a change in pull request #247: SOLR-15576: Allow filtering on ISO-8601 formatted timestamp literals in SQL WHERE clause
Posted by GitBox <gi...@apache.org>.
thelabdude commented on a change in pull request #247:
URL: https://github.com/apache/solr/pull/247#discussion_r682971005
##########
File path: solr/core/src/java/org/apache/solr/handler/sql/SolrFilter.java
##########
@@ -278,6 +265,24 @@ protected String translateComparison(RexNode node) {
}
}
+ private String toEqualsClause(String key, RexLiteral value, RexNode node) {
+ SqlTypeName fieldTypeName = ((RexCall) node).getOperands().get(0).getType().getSqlTypeName();
+ String terms = toSolrLiteralForEquals(value, fieldTypeName).trim();
+
+ boolean wrappedQuotes = false;
+ if (!terms.startsWith("(") && !terms.startsWith("[") && !terms.startsWith("{")) {
+ terms = "\"" + terms + "\"";
+ wrappedQuotes = true;
Review comment:
if terms has a quote, the query breaks ... I think we should just fix this here vs. opening another PR though
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] madrob commented on a change in pull request #247: SOLR-15576: Allow filtering on ISO-8601 formatted timestamp literals in SQL WHERE clause
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #247:
URL: https://github.com/apache/solr/pull/247#discussion_r683476249
##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/io/stream/JDBCStream.java
##########
@@ -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
Review 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] madrob commented on a change in pull request #247: SOLR-15576: Allow filtering on ISO-8601 formatted timestamp literals in SQL WHERE clause
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #247:
URL: https://github.com/apache/solr/pull/247#discussion_r682942562
##########
File path: solr/core/src/java/org/apache/solr/handler/sql/SolrFilter.java
##########
@@ -278,6 +265,24 @@ protected String translateComparison(RexNode node) {
}
}
+ private String toEqualsClause(String key, RexLiteral value, RexNode node) {
+ SqlTypeName fieldTypeName = ((RexCall) node).getOperands().get(0).getType().getSqlTypeName();
+ String terms = toSolrLiteralForEquals(value, fieldTypeName).trim();
+
+ boolean wrappedQuotes = false;
+ if (!terms.startsWith("(") && !terms.startsWith("[") && !terms.startsWith("{")) {
+ terms = "\"" + terms + "\"";
+ wrappedQuotes = true;
Review comment:
Out of scope here, but #236 has me thinking about what if there are quotes inside of the terms.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] madrob commented on a change in pull request #247: SOLR-15576: Allow filtering on ISO-8601 formatted timestamp literals in SQL WHERE clause
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #247:
URL: https://github.com/apache/solr/pull/247#discussion_r683476249
##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/io/stream/JDBCStream.java
##########
@@ -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
Review comment:
I'm confused here, why does exception message have a %, but sqlQuery wouldn't?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] thelabdude merged pull request #247: SOLR-15576: Allow filtering on ISO-8601 formatted timestamp literals in SQL WHERE clause
Posted by GitBox <gi...@apache.org>.
thelabdude merged pull request #247:
URL: https://github.com/apache/solr/pull/247
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] madrob commented on a change in pull request #247: SOLR-15576: Allow filtering on ISO-8601 formatted timestamp literals in SQL WHERE clause
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #247:
URL: https://github.com/apache/solr/pull/247#discussion_r683000680
##########
File path: solr/core/src/java/org/apache/solr/handler/sql/SolrFilter.java
##########
@@ -278,6 +265,24 @@ protected String translateComparison(RexNode node) {
}
}
+ private String toEqualsClause(String key, RexLiteral value, RexNode node) {
+ SqlTypeName fieldTypeName = ((RexCall) node).getOperands().get(0).getType().getSqlTypeName();
+ String terms = toSolrLiteralForEquals(value, fieldTypeName).trim();
+
+ boolean wrappedQuotes = false;
+ if (!terms.startsWith("(") && !terms.startsWith("[") && !terms.startsWith("{")) {
+ terms = "\"" + terms + "\"";
+ wrappedQuotes = true;
Review comment:
I was imagining that it would be more work than it ended up actually being
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] thelabdude commented on a change in pull request #247: SOLR-15576: Allow filtering on ISO-8601 formatted timestamp literals in SQL WHERE clause
Posted by GitBox <gi...@apache.org>.
thelabdude commented on a change in pull request #247:
URL: https://github.com/apache/solr/pull/247#discussion_r682971005
##########
File path: solr/core/src/java/org/apache/solr/handler/sql/SolrFilter.java
##########
@@ -278,6 +265,24 @@ protected String translateComparison(RexNode node) {
}
}
+ private String toEqualsClause(String key, RexLiteral value, RexNode node) {
+ SqlTypeName fieldTypeName = ((RexCall) node).getOperands().get(0).getType().getSqlTypeName();
+ String terms = toSolrLiteralForEquals(value, fieldTypeName).trim();
+
+ boolean wrappedQuotes = false;
+ if (!terms.startsWith("(") && !terms.startsWith("[") && !terms.startsWith("{")) {
+ terms = "\"" + terms + "\"";
+ wrappedQuotes = true;
Review comment:
if terms has a quote, the query breaks ... I think we should just fix this here vs. opening another PR though
##########
File path: solr/core/src/java/org/apache/solr/handler/sql/SolrFilter.java
##########
@@ -278,6 +265,24 @@ protected String translateComparison(RexNode node) {
}
}
+ private String toEqualsClause(String key, RexLiteral value, RexNode node) {
+ SqlTypeName fieldTypeName = ((RexCall) node).getOperands().get(0).getType().getSqlTypeName();
+ String terms = toSolrLiteralForEquals(value, fieldTypeName).trim();
+
+ boolean wrappedQuotes = false;
+ if (!terms.startsWith("(") && !terms.startsWith("[") && !terms.startsWith("{")) {
+ terms = "\"" + terms + "\"";
+ wrappedQuotes = true;
Review 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 ;-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] thelabdude commented on a change in pull request #247: SOLR-15576: Allow filtering on ISO-8601 formatted timestamp literals in SQL WHERE clause
Posted by GitBox <gi...@apache.org>.
thelabdude commented on a change in pull request #247:
URL: https://github.com/apache/solr/pull/247#discussion_r683505909
##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/io/stream/JDBCStream.java
##########
@@ -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
Review 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] thelabdude commented on a change in pull request #247: SOLR-15576: Allow filtering on ISO-8601 formatted timestamp literals in SQL WHERE clause
Posted by GitBox <gi...@apache.org>.
thelabdude commented on a change in pull request #247:
URL: https://github.com/apache/solr/pull/247#discussion_r683537838
##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/io/stream/JDBCStream.java
##########
@@ -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
Review 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] madrob commented on a change in pull request #247: SOLR-15576: Allow filtering on ISO-8601 formatted timestamp literals in SQL WHERE clause
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #247:
URL: https://github.com/apache/solr/pull/247#discussion_r682942562
##########
File path: solr/core/src/java/org/apache/solr/handler/sql/SolrFilter.java
##########
@@ -278,6 +265,24 @@ protected String translateComparison(RexNode node) {
}
}
+ private String toEqualsClause(String key, RexLiteral value, RexNode node) {
+ SqlTypeName fieldTypeName = ((RexCall) node).getOperands().get(0).getType().getSqlTypeName();
+ String terms = toSolrLiteralForEquals(value, fieldTypeName).trim();
+
+ boolean wrappedQuotes = false;
+ if (!terms.startsWith("(") && !terms.startsWith("[") && !terms.startsWith("{")) {
+ terms = "\"" + terms + "\"";
+ wrappedQuotes = true;
Review comment:
Out of scope here, but #236 has me thinking about what if there are quotes inside of the terms.
##########
File path: solr/core/src/java/org/apache/solr/handler/sql/SolrFilter.java
##########
@@ -278,6 +265,24 @@ protected String translateComparison(RexNode node) {
}
}
+ private String toEqualsClause(String key, RexLiteral value, RexNode node) {
+ SqlTypeName fieldTypeName = ((RexCall) node).getOperands().get(0).getType().getSqlTypeName();
+ String terms = toSolrLiteralForEquals(value, fieldTypeName).trim();
+
+ boolean wrappedQuotes = false;
+ if (!terms.startsWith("(") && !terms.startsWith("[") && !terms.startsWith("{")) {
+ terms = "\"" + terms + "\"";
+ wrappedQuotes = true;
Review comment:
I was imagining that it would be more work than it ended up actually being
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org