-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Support predicate pushdown on char and decimal type in mongodb #18382
Conversation
43a8cf7
to
f59b142
Compare
@@ -341,7 +341,11 @@ public Object[][] predicatePushdownProvider() | |||
{"smallint '2'"}, | |||
{"integer '3'"}, | |||
{"bigint '4'"}, | |||
{"DECIMAL '3.14'"}, |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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))")
There was a problem hiding this comment.
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
plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/TypeUtils.java
Outdated
Show resolved
Hide resolved
f59b142
to
fc03635
Compare
plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/MongoSession.java
Outdated
Show resolved
Hide resolved
fc03635
to
07b6236
Compare
plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/TypeUtils.java
Outdated
Show resolved
Hide resolved
07b6236
to
083de79
Compare
assertThat(query("SELECT * FROM " + table.getName() + " WHERE col = " + predicateValue)) | ||
.returnsEmptyResult(); |
There was a problem hiding this comment.
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}
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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
083de79
to
914ad26
Compare
plugin/trino-mongodb/src/test/java/io/trino/plugin/mongodb/TestMongoConnectorTest.java
Outdated
Show resolved
Hide resolved
assertThat(query("SELECT * FROM " + table.getName() + " WHERE col = " + predicateValue)) | ||
.returnsEmptyResult(); |
There was a problem hiding this comment.
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
assertThat(query("SELECT * FROM " + table.getName() + " WHERE col = " + predicateValue)) | ||
.returnsEmptyResult(); |
There was a problem hiding this comment.
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 ?
ba49729
to
fea3190
Compare
This PR LGTM, the only thing I missed why pushdown not happens with higher precision values #18382 (comment) |
Thanks @praveen2111, @ebyhr, @vlad-lyutenko for the review. Addressed comments |
@vlad-lyutenko, I have added a comment. isFullyPushdown works when there is
|
plugin/trino-mongodb/src/test/java/io/trino/plugin/mongodb/TestMongoConnectorTest.java
Outdated
Show resolved
Hide resolved
plugin/trino-mongodb/src/test/java/io/trino/plugin/mongodb/TestMongoConnectorTest.java
Outdated
Show resolved
Hide resolved
fea3190
to
7d75688
Compare
Thanks, @ebyhr for the review. Addressed comments. |
plugin/trino-mongodb/src/test/java/io/trino/plugin/mongodb/TestMongoConnectorTest.java
Outdated
Show resolved
Hide resolved
7d75688
to
5b050ea
Compare
@Praveen2112, AC |
Thanks for working on this. |
Release notes
(X) Release notes are required, with the following suggested text: