Skip to content

[opt](jdbc) Support Oracle BINARY_FLOAT and BINARY_DOUBLE types - #68200

Open
zhangstar333 wants to merge 1 commit into
apache:masterfrom
zhangstar333:jdbc-oracle-type
Open

zhangstar333 wants to merge 1 commit into
apache:masterfrom
zhangstar333:jdbc-oracle-type

Conversation

@zhangstar333

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Problem Summary:
Support Oracle BINARY_FLOAT and BINARY_DOUBLE type mapping

Release note

None

Check List (For Author)

  • Test

    • Regression test
    • Unit Test
    • Manual test (add detailed scripts or steps below)
    • No need to test or manual test. Explain why:
      • This is a refactor/code format and no logic has been changed.
      • Previous test can cover this change.
      • No code files have been changed.
      • Other reason
  • Behavior changed:

    • No.
    • Yes.
  • Does this need documentation?

    • No.
    • Yes.

Check List (For Reviewer who merge this PR)

  • Confirm the release note
  • Confirm test cases
  • Confirm document
  • Add branch pick label

@zhangstar333

Copy link
Copy Markdown
Contributor Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Requesting changes for three correctness issues.

Review status: capped/incomplete. The third and final allowed convergence round surfaced the non-finite predicate issue below. Every discovered candidate was adjudicated, but the review contract forbids a fourth round, so this review cannot claim convergence.

Critical checkpoints:

  • Goal and test proof: not achieved. Normal type='jdbc' catalogs execute the separate plugin mapper, which remains unsupported; the added unit test directly proves only the inactive legacy switch. No Oracle end-to-end schema/read/write/predicate result was supplied.
  • Scope and parallel paths: the local switch edit is small, but it misses the active plugin metadata path and its downstream write and predicate behavior.
  • Concurrency, locking, lifecycle/static initialization, persistence/EditLog, transaction atomicity, and observability: no new applicable issue found.
  • Configuration and compatibility: existing FLOAT/DOUBLE descriptor transport and old/new Oracle scan getters are compatible. The sink, however, lacks the Oracle/remote-type information needed for native binary binding. No new storage-format or rolling-upgrade issue was found.
  • Special conditions: NULL and finite read materialization are already supported; NaN/infinity predicate serialization is not.
  • Performance: the mapper itself is negligible, while Oracle documents avoidable conversion work in the current NUMBER-format write binding.
  • User focus: no additional focus was provided.

Validation was static-only as required by the review prompt; I did not run builds or tests, and author/CI claims were not treated as independent validation. Reviewed exact head 097a9331263580ca645df028c07b8994fa888369.

precision = scale;
}
return createDecimalOrStringType(precision, scale);
case "BINARY_FLOAT":

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[P1] Update the Oracle connector mapper used by JDBC catalogs

A normal type='jdbc' catalog is routed through JdbcConnectorProvider to JdbcOracleConnectorClient, and JdbcConnectorMetadata#getTableSchema calls that class's jdbcTypeToConnectorType. Its switch still has only FLOAT and falls through to UNSUPPORTED for both BINARY_FLOAT and BINARY_DOUBLE; this legacy JdbcOracleClient mapper is not on the catalog path (its remaining production callers are streaming helpers whose source enum excludes Oracle). Consequently this unit test can pass while DESC/queries on an Oracle catalog still reject these columns. Please add the two mappings to JdbcOracleConnectorClient and cover that production mapper, ideally with the existing Oracle JDBC regression suite.

}
return createDecimalOrStringType(precision, scale);
case "BINARY_FLOAT":
return Type.FLOAT;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[P1] Do not push bare NaN/Infinity tokens to Oracle

After the production mapping is fixed, these types make non-finite predicates reachable. The connector currently unwraps CAST, carries the Java Double into JdbcQueryBuilder, and literalToSql renders it with toString(), so for example binary_double_col < CAST('Infinity' AS DOUBLE) becomes "BD" < Infinity. Oracle accepts the supplied BINARY_FLOAT_NAN/BINARY_DOUBLE_NAN and BINARY_FLOAT_INFINITY/BINARY_DOUBLE_INFINITY constants, not bare NaN/Infinity tokens (Oracle literals). Please render the type-appropriate Oracle literal (including negative infinity) or decline this conjunct from pushdown so BE evaluates it locally, and add query-builder/regression coverage.

return createDecimalOrStringType(precision, scale);
case "BINARY_FLOAT":
return Type.FLOAT;
case "BINARY_DOUBLE":

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[P1] Preserve the binary format on Oracle JDBC writes

Once these cases are added to the production connector mapper, the columns also become writable through the JDBC catalog. The sink currently binds every Doris FLOAT/DOUBLE with standard PreparedStatement.setFloat/setDouble, and its writer parameters carry neither the remote Oracle type nor even table_type. Oracle documents that these standard setters send NUMBER-format bits by default; converting those bits back into BINARY_FLOAT/BINARY_DOUBLE is not bit-exact and can corrupt values (OraclePreparedStatement). Please preserve enough Oracle/type information to use native binary setters, or enable SetFloatAndDoubleUseBinary for Oracle writer connections, and cover a binary-float INSERT/readback round trip.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant