-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Fix failure reading FLOAT type in Query table function in Oracle
#27880
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
Fix failure reading FLOAT type in Query table function in Oracle
#27880
Conversation
5ff6b71 to
19052a6
Compare
plugin/trino-oracle/src/main/java/io/trino/plugin/oracle/OracleClient.java
Show resolved
Hide resolved
plugin/trino-oracle/src/main/java/io/trino/plugin/oracle/OracleClient.java
Outdated
Show resolved
Hide resolved
plugin/trino-oracle/src/main/java/io/trino/plugin/oracle/OracleClient.java
Outdated
Show resolved
Hide resolved
plugin/trino-oracle/src/main/java/io/trino/plugin/oracle/OracleClient.java
Outdated
Show resolved
Hide resolved
| Optional.of(caseSensitive)); | ||
| } | ||
|
|
||
| private static boolean isAllowedNumber(int precision, int columnSize) |
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 this doesn't compare columnSize and precision values?
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.
Updated the logic to follow
trino/plugin/trino-oracle/src/main/java/io/trino/plugin/oracle/OracleClient.java
Lines 507 to 520 in 2d36105
| if (precision < scale) { | |
| if (roundingMode == RoundingMode.UNNECESSARY) { | |
| break; | |
| } | |
| scale = min(Decimals.MAX_PRECISION, scale); | |
| precision = scale; | |
| } | |
| else if (numberDefaultScale.isPresent() && precision == PRECISION_OF_UNSPECIFIED_NUMBER) { | |
| precision = Decimals.MAX_PRECISION; | |
| scale = numberDefaultScale.get(); | |
| } | |
| else if (precision > Decimals.MAX_PRECISION || actualPrecision <= 0) { | |
| break; | |
| } |
The purpose of the method is to check if it is able to be handled as the
NUMBER - as we could always correct interpret the type and with less "lossy"If it not, we use it actual type - FLOAT
plugin/trino-oracle/src/test/java/io/trino/plugin/oracle/BaseOracleConnectorTest.java
Outdated
Show resolved
Hide resolved
plugin/trino-oracle/src/main/java/io/trino/plugin/oracle/OracleClient.java
Show resolved
Hide resolved
plugin/trino-oracle/src/test/java/io/trino/plugin/oracle/BaseOracleConnectorTest.java
Outdated
Show resolved
Hide resolved
80c4b57 to
974aefb
Compare
plugin/trino-oracle/src/main/java/io/trino/plugin/oracle/OracleClient.java
Outdated
Show resolved
Hide resolved
plugin/trino-oracle/src/test/java/io/trino/plugin/oracle/BaseOracleConnectorTest.java
Outdated
Show resolved
Hide resolved
plugin/trino-oracle/src/main/java/io/trino/plugin/oracle/OracleClient.java
Show resolved
Hide resolved
| return columns.build(); | ||
| } | ||
|
|
||
| private static JdbcTypeHandle getJdbcTypeHandle(ConnectorSession session, ResultSetMetaData metadata, int column) |
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: columnIndex
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 want to make consistency with
trino/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java
Line 295 in 195d536
| for (int column = 1; column <= metadata.getColumnCount(); column++) { |
column as the name
| { | ||
| ImmutableList.Builder<JdbcColumnHandle> columns = ImmutableList.builder(); | ||
| for (int column = 1; column <= metadata.getColumnCount(); column++) { | ||
| JdbcTypeHandle jdbcTypeHandle = getJdbcTypeHandle(session, metadata, column); |
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.
Should we introduce getJdbcTypeHandle as an extension point so that connectors could implement their logic without mimicing the boilder plate code ?
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.
Currently only Oracle has the requirements, so seems not so urgent to do such extension, wdyt?
plugin/trino-oracle/src/main/java/io/trino/plugin/oracle/OracleClient.java
Show resolved
Hide resolved
ca69056 to
28178dc
Compare
Description
Reading
FLOATinQuerytable function in Oracle will fail with:The return types in
Querytable function are derived from a prepared query https://github.com/trinodb/trino/blob/ce0492437e72d9e7483ef0028a941b333f82e488/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/ptf/Query.java#L99C8-L106Oracle will declare type to NUMBER for the
FLOATtype.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.
(x) Release notes are required, with the following suggested text: