-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Allow complex expression rewrites with approximate numeric constant #21892
base: master
Are you sure you want to change the base?
Allow complex expression rewrites with approximate numeric constant #21892
Conversation
WIll update once the test failures are fixed. |
1ffef64
to
76c3f16
Compare
try (TestTable testTable = new TestTable( | ||
getQueryRunner()::execute, | ||
"test_complex_expression_pushdown_", | ||
"AS SELECT 1 as a_int, REAL '3.14' as a_real, DOUBLE '1.9891e30' as a_double")) { |
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.
add a test with NaN stored in the remote database (for systems that do support storing NaN)
and queries col < 1
, col > 1
, col > -inf
, col < +inf
.
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'll address it as a follow-up PR
5516763
to
c42ccec
Compare
Enable them for MariaDB, MySQL, Oracle and Ignite connectors
c42ccec
to
c99fe17
Compare
c99fe17
to
5f4033d
Compare
Type type = constant.getType(); | ||
Object value = constant.getValue(); | ||
if (value == null) { | ||
// TODO we could handle NULL values too |
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.
nit: should we add ticket number for the todo?
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.
NULL literal is rarely practical for pushdown, since most of the time this means the encompassing expression is NULL and should be simplified in the trino engine.
i am sure there are cases where it's different (like trino format
function), but can't of anything non Trino specific yet.
"complex_expression_", | ||
"AS SELECT 1 as a_int, REAL '3.14' as a_real, DOUBLE '1.9891e30' as a_double")) { | ||
assertThat(query("SELECT a_int FROM " + testTable.getName() + " WHERE a_real > REAL '3.12' OR a_double > DOUBLE '1.9890e30'")) | ||
.isFullyPushedDown(); |
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 testcase with at least either Double.MIN_VALUE or Double.MIN_VALUE and Float.MIN_VALUE or Float.MIN_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.
good point
with at least either Double.MIN_VALUE or Double.MIN_VALUE
not "or". all these should be tested
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.
MIN_VALUE or MAX_VALUE as a part of data or constant or both ?
Description
Currently
RewriteExactNumericConstant
doesn't support rewriting expressions if they are ofDOUBLE
orREAL
type, so we are generalizing it toRewriteNumericConstant
.Additional context and related issues
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.
( ) Release notes are required, with the following suggested text:
This would affect the JDBC connectors in general so should implement it for each of them or do we have a generic options for JDBC ?