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

Smoke test varchar value with backslash #14125

Conversation

findepi
Copy link
Member

@findepi findepi commented Sep 14, 2022

No description provided.

@findepi findepi added test no-release-notes This pull request does not require release notes entry labels Sep 14, 2022
@cla-bot cla-bot bot added the cla-signed label Sep 14, 2022
@findepi
Copy link
Member Author

findepi commented Sep 15, 2022

CI #13946 and apparent maven central problems

@findepi
Copy link
Member Author

findepi commented Sep 15, 2022

CI #14141

@findepi findepi force-pushed the findepi/smoke-test-varchar-value-with-backslash-1020c6 branch from 913b027 to 3e5d84a Compare September 16, 2022 13:46
Copy link
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.

LGTM

This is testing correctness, regardless of whether the pushdown happens.
This covers a case where pushdown cannot happen with a `TupleDomain`,
but can be achieved via `ConnectorExpression`.
@findepi findepi force-pushed the findepi/smoke-test-varchar-value-with-backslash-1020c6 branch from 3e5d84a to cca820a Compare September 16, 2022 20:38
@findepi findepi merged commit b88d183 into trinodb:master Sep 16, 2022
@findepi findepi deleted the findepi/smoke-test-varchar-value-with-backslash-1020c6 branch September 16, 2022 20:38
@github-actions github-actions bot added this to the 397 milestone Sep 16, 2022
@hashhar
Copy link
Member

hashhar commented Sep 22, 2022

@findepi This seems to fail for BigQuery on master. Can you PTAL?

@findepi
Copy link
Member Author

findepi commented Sep 22, 2022

@hashhar didn't notice that, thanks for pointing out.
Looks like we even released 397 without me noticing that the master is red :/

Seems we re-discovered #7900

@findepi
Copy link
Member Author

findepi commented Sep 22, 2022

Actually -- #7869 uses backslash to escape apostrophes, so we knew backslashes are meaningful and they themselves require escaping ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed no-release-notes This pull request does not require release notes entry test
Development

Successfully merging this pull request may close these issues.

None yet

2 participants