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

Document predicate pushdown support for string-type columns in SQL Se… #16366

Merged
merged 1 commit into from
Apr 3, 2023

Conversation

Jessie212
Copy link
Contributor

…rver

Description

Document predicate pushdown support for string-type columns.

Additional context and related issues

#15714

Release notes

(x) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

@cla-bot cla-bot bot added the cla-signed label Mar 3, 2023
@Jessie212 Jessie212 requested a review from hashhar March 3, 2023 15:39
@Jessie212
Copy link
Contributor Author

@hashhar I need your help with this.

docs/src/main/sphinx/connector/sqlserver.rst Outdated Show resolved Hide resolved
docs/src/main/sphinx/connector/sqlserver.rst Outdated Show resolved Hide resolved

.. example

To enable pushdown for case-sensitive columns, change the collation setting to a
Copy link
Member

Choose a reason for hiding this comment

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

I have no idea what that means. What setting do you change from what to what?

Copy link
Contributor

@vlad-lyutenko vlad-lyutenko Mar 9, 2023

Choose a reason for hiding this comment

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

In SqlServer every textual column have a Collation.
Collation it's kind of set of rules how to treat text symbols in this column during different operations (like compare during sorting).
For example collation Latin1_General_CI_AS - means for this column we translate bytes to symbols using Latin byte to symbol translation and CI - mean Case Insensitivity and other rules.
So our code is working only if collation is Case Sensitive for column.

Copy link
Contributor

@vlad-lyutenko vlad-lyutenko Mar 9, 2023

Choose a reason for hiding this comment

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

Case sensitive collation means that in Collation name there is part like CS or BIN or BIN2

for example: Latin1_General_CS_AS

@github-actions github-actions bot added the docs label Mar 3, 2023
@Jessie212 Jessie212 force-pushed the jt/predicate-pushdown-sqlserver branch from 1080326 to cff1341 Compare March 10, 2023 18:33
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.

What do you think about wording like:

The connector supports pushdown of predicates on ``VARCHAR`` and
``NVARCHAR`` columns if the underlying columns in SQL Server use a
case-sensitive collation. The following operators are pushed down:

- ``=``
- ``<>``
- ``IN``
- ``NOT IN``

If the column in SQL Server uses a case-insensitive collation then no
operators are pushed down to preserve correct results. To enable
pushdown on such columns you must change the collation to be a
case-sensitive one in SQL Server. See https://learn.microsoft.com/en-us/sql/relational-databases/collations/collation-and-unicode-support?view=sql-server-ver16

The TL;DR is:

  • SQL Server column is case-sensitive
    • =, !=, IN and NOT IN can be pushed down
    • <, >, BETWEEN etc. cannot be pushed down
  • SQL Server column is case-insensitive
    • No pushdown at all unless you change the collation in SQL Server to be a case-sensitive one

@Jessie212
Copy link
Contributor Author

Thanks for taking time to explain, @hashhar.

@Jessie212 Jessie212 force-pushed the jt/predicate-pushdown-sqlserver branch from cff1341 to 3ecefe1 Compare March 13, 2023 17:43
@Jessie212 Jessie212 force-pushed the jt/predicate-pushdown-sqlserver branch from 3ecefe1 to 76b7ca6 Compare March 14, 2023 17:33
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 % comment

docs/src/main/sphinx/connector/sqlserver.rst Outdated Show resolved Hide resolved
Comment on lines 453 to 454
Operators are not pushed down and correct results are not preserved for columns
using a case-insensitive collation.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Operators are not pushed down and correct results are not preserved for columns
using a case-insensitive collation.
Operators are not pushed down for columns using a case-insensitive collation
to preserve correct results.

the current sentence reads the opposite (i.e. results are incorrect)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the "to preserve correct results" that needs to be changed to sound less awkward. Since I'm having trouble improving the flow of the sentence without changing the meaning, please help me understand this process better.

For columns using a case-insensitive collation, operators are not pushed down, which preserves correct results. Is that right or wrong?

Copy link
Member

Choose a reason for hiding this comment

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

For columns using a case-insensitive collation, operators are not pushed down, which preserves correct results.

Yes, this is the behaviour and intention.

@Jessie212 Jessie212 force-pushed the jt/predicate-pushdown-sqlserver branch from 76b7ca6 to 4efb32e Compare March 20, 2023 17:32
Copy link
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

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

Looks good.

@hashhar hashhar merged commit e5ffe5a into trinodb:master Apr 3, 2023
@github-actions github-actions bot added this to the 412 milestone Apr 3, 2023
@Jessie212 Jessie212 deleted the jt/predicate-pushdown-sqlserver branch April 3, 2023 15:46
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.

5 participants