Skip to content

Added a fix for BQ connector to correctly treat single quote in query criteria#7869

Merged
ebyhr merged 1 commit intotrinodb:masterfrom
ayushbilala:bq-single-quote-bug
May 17, 2021
Merged

Added a fix for BQ connector to correctly treat single quote in query criteria#7869
ebyhr merged 1 commit intotrinodb:masterfrom
ayushbilala:bq-single-quote-bug

Conversation

@ayushbilala
Copy link
Copy Markdown
Contributor

Fix for #7784.

Added a change to properly rewrite filter values containing the ANSI SQL single quote Escape sequence into the one that BigQuery is happy with.

Example:
'Looney''s Lane' string in Presto query filter will be changed to 'Looney\'s Lane' in order to make the query work.

@cla-bot
Copy link
Copy Markdown

cla-bot bot commented May 9, 2021

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Ayush Bilala.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email email@example.com
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@ayushbilala ayushbilala requested review from ebyhr and hashhar and removed request for ebyhr and hashhar May 9, 2021 18:07
Copy link
Copy Markdown
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

Thanks for sending a PR.

It'd be nice to also handle other possible cases. It can be part of a follow-up if needed.
Please also change the commit message to describe the fix instead of referencing a GitHub issue. You can comment in the PR description as Fixes #7784 instead.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It'd be nice to handle all possible characters that need escaping.

If possible can you also add support for the ones listed at https://cloud.google.com/bigquery/docs/reference/standard-sql/lexical#string_and_bytes_literals along with test-cases?

Copy link
Copy Markdown
Contributor Author

@ayushbilala ayushbilala May 9, 2021

Choose a reason for hiding this comment

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

Sure, let me take a look and figure out what does it actually take to add escaping for all possible characters.
If it is too much effort, I'll probably take it up as a follow-up.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@hashhar Adding support for other literals doesn't look straightforward. Is it fine if we go ahead with this one and I'll do some more research and raise a separate issue for other characters?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please create an issue with your findings and add a TODO comment here so that we don't forget.

cc: @ebyhr

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure, I am on it.

Copy link
Copy Markdown
Contributor Author

@ayushbilala ayushbilala May 12, 2021

Choose a reason for hiding this comment

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

Created an issue for adding support for all characters #7900.
Added a TODO comment.

Copy link
Copy Markdown
Member

@ebyhr ebyhr left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this. Understood remained patterns will be handled in #7900.

Left only minor comments. Please squash commits into one and update the commit title as
Escape single quote in BigQuery string condition

Copy link
Copy Markdown
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

@ayushbilala Please squash the commits and amend commit message as suggested by @ebyhr in #7869 (review).

@ebyhr I've verified TestBigQueryIntegrationSmokeTest passes.

@ebyhr ebyhr merged commit c540a8e into trinodb:master May 17, 2021
@ebyhr
Copy link
Copy Markdown
Member

ebyhr commented May 17, 2021

Merged, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

3 participants