Skip to content

MongoDB: Introduce a config flag to determine whether to skip using collation or not when querying #26304

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sherifkayad
Copy link

@sherifkayad sherifkayad commented Jul 30, 2025

MongoDB: Introduce a config flag to determine whether to skip using collation or not when querying

Description

MongoDB: Introduce a config flag to determine whether to skip using collation or not when querying. This is mainly to address the issue reported by #15911

This PR introduces a config flag mongodb.skip-collation (defaults to false) and based on it, the MongoSession decides whether to use a collation or not.

Additional context and related issues

#15911

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(x) Release notes are required, with the following suggested text: MongoDB: Introduce a config flag to determine whether to skip using collation or not when querying

Copy link

cla-bot bot commented Jul 30, 2025

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

@github-actions github-actions bot added docs mongodb MongoDB connector labels Jul 30, 2025
@sherifkayad sherifkayad force-pushed the feat/config-property-to-allow-skipping-collation-use branch from b219b01 to 0bcb9e3 Compare July 30, 2025 09:50
Copy link

cla-bot bot commented Jul 30, 2025

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

Copy link
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.

Can we use a localstatck docker container for testing this change?

@sherifkayad
Copy link
Author

Can we use a localstatck docker container for testing this change?

@ebyhr I think probably one could do that. Or maybe if you give me an easy way to build a trino docker image locally for the MR I can test against one of my DocumentDB instances in AWS

@sherifkayad sherifkayad force-pushed the feat/config-property-to-allow-skipping-collation-use branch from 0bcb9e3 to 836afa3 Compare July 30, 2025 11:44
Copy link

cla-bot bot commented Jul 30, 2025

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

@sherifkayad sherifkayad force-pushed the feat/config-property-to-allow-skipping-collation-use branch from 836afa3 to ad8a07f Compare July 30, 2025 11:52
Copy link

cla-bot bot commented Jul 30, 2025

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

@sherifkayad sherifkayad force-pushed the feat/config-property-to-allow-skipping-collation-use branch from ad8a07f to 10f5e51 Compare July 31, 2025 09:52
Copy link

cla-bot bot commented Jul 31, 2025

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

@sherifkayad
Copy link
Author

@ebyhr I signed and sent out the CLA yesterday .. however, it seems like the document is still not on file .. any clue how fast that process could be? .. also do you think we can easily build a Trino Docker Image with that change such that I can test it locally?

@chenjian2664
Copy link
Contributor

@sherifkayad CLA usually take few days, no worries

@ebyhr
Copy link
Member

ebyhr commented Aug 1, 2025

do you think we can easily build a Trino Docker Image with that change such that I can test it locally?

Please try adding dockerized tests first. We can't ensure future changes if don't add automated tests.

@sherifkayad
Copy link
Author

sherifkayad commented Aug 1, 2025

do you think we can easily build a Trino Docker Image with that change such that I can test it locally?

Please try adding dockerized tests first. We can't ensure future changes if don't add automated tests.

@ebyhr I believe that DocumentDB is a premium feature of localstack => https://docs.localstack.cloud/aws/services/docdb/ .. I am not sure that a test with localstack would be possible.

@ebyhr
Copy link
Member

ebyhr commented Aug 5, 2025

Thanks for the confirmation. Manual testing is fine, then.

FindIterable<Document> iterable = collection.find(filter).projection(projection).collation(SIMPLE_COLLATION);
FindIterable<Document> iterable = collection.find(filter).projection(projection);

if (!skipCollation) {
Copy link
Member

Choose a reason for hiding this comment

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

MongoDB doesn't expose whether collation is supported or not, right? I'm asking because we'd like to avoid using configuration properties as much as possible. Ideally, this skip should be determined automatically rather than relying on a flag.

Copy link
Author

Choose a reason for hiding this comment

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

@ebyhr I tried searching quickly for that and it doesn't seem to be the case that collation is a capability you can simply query for its presence or not .. as far as I have read the only way would be to attempt a try / catch query with collation to see if it's supported or not .. which in my view is not really an elegant solution

Copy link
Author

Choose a reason for hiding this comment

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

@ebyhr may I ask how we can proceed with that PR here?

@sherifkayad sherifkayad force-pushed the feat/config-property-to-allow-skipping-collation-use branch from 10f5e51 to 9579083 Compare August 5, 2025 20:34
Copy link

cla-bot bot commented Aug 5, 2025

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

@sherifkayad sherifkayad force-pushed the feat/config-property-to-allow-skipping-collation-use branch from 9579083 to 76aed9f Compare August 6, 2025 14:32
Copy link

cla-bot bot commented Aug 6, 2025

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

…ollation or not when querying

Signed-off-by: Sherif Ayad <[email protected]>
@sherifkayad sherifkayad force-pushed the feat/config-property-to-allow-skipping-collation-use branch from 76aed9f to 1e51c10 Compare August 7, 2025 09:13
Copy link

cla-bot bot commented Aug 7, 2025

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs mongodb MongoDB connector
Development

Successfully merging this pull request may close these issues.

3 participants