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

Support predicate pushdown on char and decimal type in mongodb #18382

Conversation

krvikash
Copy link
Contributor

@krvikash krvikash commented Jul 24, 2023

Release notes

(X) Release notes are required, with the following suggested text:

# Mongo
* Support predicate pushdown on `char` and `decimal` type. ({issue}`18382`)

@cla-bot cla-bot bot added the cla-signed label Jul 24, 2023
@github-actions github-actions bot added the mongodb MongoDB connector label Jul 24, 2023
@krvikash krvikash force-pushed the krvikash/mongodb-support-predicate-pushdown-for-char-varbinary-decimal-type branch 2 times, most recently from 43a8cf7 to f59b142 Compare July 24, 2023 06:58
@krvikash krvikash marked this pull request as ready for review July 24, 2023 07:00
@krvikash krvikash self-assigned this Jul 24, 2023
@@ -341,7 +341,11 @@ public Object[][] predicatePushdownProvider()
{"smallint '2'"},
{"integer '3'"},
{"bigint '4'"},
{"DECIMAL '3.14'"},
Copy link
Member

Choose a reason for hiding this comment

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

Apart from this - can we update the tests in TestMongoTypeMapping ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed offline, we wanted to fix below two cases. But Decimal128 supports 34 digits precision. and below tests still failing with the current change.

// TODO: Fix NumberFormatException: Conversion to Decimal128 would require inexact rounding
//                .addRoundTrip("decimal(38, 0)", "CAST('27182818284590452353602874713526624977' AS decimal(38, 0))", createDecimalType(38, 0), "CAST('27182818284590452353602874713526624977' AS decimal(38, 0))")
//                .addRoundTrip("decimal(38, 0)", "CAST('-27182818284590452353602874713526624977' AS decimal(38, 0))", createDecimalType(38, 0), "CAST('-27182818284590452353602874713526624977' AS decimal(38, 0))")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed offline. To ensure 38 precision on the filter should not error, I have added testHighPrecisionDecimalPredicate

@krvikash krvikash force-pushed the krvikash/mongodb-support-predicate-pushdown-for-char-varbinary-decimal-type branch from f59b142 to fc03635 Compare July 24, 2023 08:53
@krvikash krvikash force-pushed the krvikash/mongodb-support-predicate-pushdown-for-char-varbinary-decimal-type branch from fc03635 to 07b6236 Compare July 24, 2023 11:00
@krvikash krvikash changed the title Support predicate pushdown on char, varbinary, and decimal type in mongodb Support predicate pushdown on char and decimal type in mongodb Jul 24, 2023
@krvikash krvikash force-pushed the krvikash/mongodb-support-predicate-pushdown-for-char-varbinary-decimal-type branch from 07b6236 to 083de79 Compare July 25, 2023 07:08
Comment on lines 365 to 406
assertThat(query("SELECT * FROM " + table.getName() + " WHERE col = " + predicateValue))
.returnsEmptyResult();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case, I was not able to apply pushdown condition.

This is the Explain of the "SELECT * FROM " + table.getName() + " WHERE col = " + predicateValue

Trino version: testversion
Fragment 0 [SINGLE]
    Output layout: [col]
    Output partitioning: SINGLE []
    Output[columnNames = [col]]
    │   Layout: [col:decimal(34,0)]
    │   Estimates: {rows: 0 (0B), cpu: 0, memory: 0B, network: 0B}
    └─ Values[]
           Layout: [col:decimal(34,0)]
           Estimates: {rows: 0 (0B), cpu: 0, memory: 0B, network: 0B}

Copy link
Contributor

Choose a reason for hiding this comment

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

I am little but missed here, why pushdown is not happening here?
I thought Trino rounds it or pass it as it is and mongo just not able to exact match value with higher pression.
But why pushdown is not happening?

Copy link
Member

Choose a reason for hiding this comment

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

Why is the table scan is replaced with a values node ? Is it due to different precision on both side of expression ? Can we add a comment for it

Copy link
Member

Choose a reason for hiding this comment

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

Can we add additional test case for 34 precision decimal value ?

Copy link
Contributor Author

@krvikash krvikash Aug 4, 2023

Choose a reason for hiding this comment

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

Why is the table scan is replaced with a values node ?

I am little but missed here, why pushdown is not happening here?

I am not sure about this. It seems that when the value is 38 precision then it is getting converted into null.

This query does not even reach MongoSession#execute

"SELECT * FROM " + table.getName() + " WHERE col = " + predicateValue

Comment on lines 365 to 406
assertThat(query("SELECT * FROM " + table.getName() + " WHERE col = " + predicateValue))
.returnsEmptyResult();
Copy link
Member

Choose a reason for hiding this comment

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

Why is the table scan is replaced with a values node ? Is it due to different precision on both side of expression ? Can we add a comment for it

Comment on lines 365 to 406
assertThat(query("SELECT * FROM " + table.getName() + " WHERE col = " + predicateValue))
.returnsEmptyResult();
Copy link
Member

Choose a reason for hiding this comment

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

Can we add additional test case for 34 precision decimal value ?

@krvikash krvikash force-pushed the krvikash/mongodb-support-predicate-pushdown-for-char-varbinary-decimal-type branch from ba49729 to fea3190 Compare August 4, 2023 09:39
@vlad-lyutenko
Copy link
Contributor

This PR LGTM, the only thing I missed why pushdown not happens with higher precision values #18382 (comment)

@krvikash
Copy link
Contributor Author

krvikash commented Aug 4, 2023

Thanks @praveen2111, @ebyhr, @vlad-lyutenko for the review. Addressed comments

@krvikash
Copy link
Contributor Author

krvikash commented Aug 4, 2023

This PR LGTM, the only thing I missed why pushdown not happens with higher precision values #18382 (comment)

@vlad-lyutenko, I have added a comment. isFullyPushdown works when there is TableScanNode. But due to precision mismatch in Column type and Predicate value, PushPredicateIntoTableScan#pushFilterIntoTableScan returns ValueNode which can not verify using isFullyPushdown.

            assertThat(query("SELECT * FROM " + table.getName() + " WHERE col %s %s ".formatted(operator, predicateValue)))
                    // With EQUAL operator when column type precision is less than the predicate value's precision,
                    // PushPredicateIntoTableScan#pushFilterIntoTableScan returns ValuesNode. So It not possible to verify isFullyPushedDown.
                    .returnsEmptyResult();

@krvikash krvikash force-pushed the krvikash/mongodb-support-predicate-pushdown-for-char-varbinary-decimal-type branch from fea3190 to 7d75688 Compare August 7, 2023 06:25
@krvikash
Copy link
Contributor Author

krvikash commented Aug 7, 2023

Thanks, @ebyhr for the review. Addressed comments.

@krvikash krvikash force-pushed the krvikash/mongodb-support-predicate-pushdown-for-char-varbinary-decimal-type branch from 7d75688 to 5b050ea Compare August 7, 2023 11:31
@krvikash
Copy link
Contributor Author

krvikash commented Aug 7, 2023

@Praveen2112, AC

@Praveen2112 Praveen2112 merged commit c76d4be into trinodb:master Aug 8, 2023
47 checks passed
@Praveen2112
Copy link
Member

Thanks for working on this.

@krvikash krvikash deleted the krvikash/mongodb-support-predicate-pushdown-for-char-varbinary-decimal-type branch August 8, 2023 10:44
@github-actions github-actions bot added this to the 423 milestone Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed mongodb MongoDB connector
Development

Successfully merging this pull request may close these issues.

None yet

4 participants