-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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
fix(sql): unable to filter text with quotes #17881
Conversation
Codecov Report
@@ Coverage Diff @@
## master #17881 +/- ##
=======================================
Coverage 66.05% 66.05%
=======================================
Files 1591 1591
Lines 62423 62423
Branches 6287 6287
=======================================
Hits 41233 41233
Misses 19568 19568
Partials 1622 1622
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looking into this. I went through the git blame logs, and it appears this change was added in #4651, so a very long time ago. In addition, it was originally prefixed with the comment "backward compatibility with previous components", so I'm not entirely sure we need this anymore - maybe just handle the special cases '' and "", but even those seem dubious. I wonder if @betodealmeida or @hughhhh happen to still remember this case? Also, the unit tests should probably not be implemented using DruidColumn, as the whole native Druid connector is in the process of being deprecated. Ping me if you want to discuss an alternative testing solution.
Hi @villebro . I have discussed this issue with @stephenLYZ. This issue requires more consideration.
There is a virtual dataset (for Postgresql) for test this issue. SELECT
'"text in double quotes"' as name
UNION
SELECT '''text in singlequotes''' as name
UNION
SELECT 'double quotes " in text' as name
UNION
SELECT 'single quotes '' in text' as name |
@stephenLYZ are there more tests to be added to this PR before final review? CC @zhaoyongjie |
ebd49c5
to
7d23b25
Compare
), | ||
database=get_example_database(), | ||
) | ||
TableColumn(column_name="foo", type="VARCHAR(255)", table=table) | ||
SqlMetric(metric_name="count", expression="count(*)", table=table) | ||
yield table |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
table wasn't persisted so we no need delete command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - good test (+115 +61) to functional code change ratio (+1 -1) 😄
@villebro @stephenLYZ While this fix is definitely a good direction, can we make sure to add this to After deploying this fix, we found out that a lot of our users had been wrapping strings in quotes, so a ton of important charts broke. I think the previous behavior made some people think that you had to wrap string filter values in quotes. |
@serenajiang thanks for flagging this and apologies for the inconvenience. This was a good reminder to always flag breaking changes in |
@serenajiang @villebro |
SUMMARY
This PR fixes the problem that the double/single quote will be dropped in SQL query when using filters.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before
After
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION