github-actions[bot] commented on code in PR #68873:
URL: https://github.com/apache/doris/pull/68873#discussion_r4237013943
##########
fe/be-java-extensions/jdbc-scanner/src/main/java/org/apache/doris/jdbc/OracleJdbcExecutor.java:
##########
@@ -50,8 +52,27 @@ public OracleJdbcExecutor(byte[] thriftParams) throws
Exception {
@Override
protected void setTimestampTz(int parameterIndex, LocalDateTime value)
throws SQLException {
- // An unzoned TIMESTAMP bind is session-local for both Oracle TZ and
LOCAL TIME ZONE columns.
- preparedStatement.setObject(parameterIndex,
value.atOffset(ZoneOffset.UTC), Types.TIMESTAMP_WITH_TIMEZONE);
+ // Older Oracle drivers reject JDBC type 2014; the sink SQL parses an
explicit UTC offset.
+ preparedStatement.setString(parameterIndex,
value.format(TIMESTAMP_TZ_FORMATTER) + " +00:00");
+ }
+
+ @Override
+ protected void setTimestampTzNull(int parameterIndex) throws SQLException {
+ preparedStatement.setNull(parameterIndex, Types.VARCHAR);
+ }
+
+ @Override
+ protected void initializeStatement(Connection conn, JdbcDataSourceConfig
config, String sql) throws SQLException {
+ // ALTER SESSION alone leaves the driver's TSLTZ zone unset;
initialize every borrowed connection.
+ try {
+ Connection physical = conn.unwrap(Connection.class);
+ Class<?> oracleConnection =
Class.forName("oracle.jdbc.OracleConnection", true,
+ physical.getClass().getClassLoader());
Review Comment:
[P1] Preserve OceanBase Oracle-mode connections. `OCEANBASE_ORACLE` is
dispatched to this executor, but its supported `com.oceanbase.jdbc.Driver`
connection does not implement `oracle.jdbc.OracleConnection`. This
unconditional load/unwrap therefore fails before every scan or write statement
(even for tables without TIMESTAMPTZ). Apply this Oracle-driver-specific setter
only to actual Oracle connections and use a supported session-zone path for
OceanBase.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonUtil.java:
##########
@@ -812,9 +812,10 @@ private static String
serializePartitionValue(org.apache.paimon.types.DataType t
return null;
}
return Double.toString((Double) value);
- // case binary:
- // case varbinary: should not supported, because if return string
with utf8,
- // the data maybe be corrupted
+ case BINARY:
+ case VARBINARY:
+ // Native readers reconstruct omitted partition columns from
lossless hex literals.
+ return value == null ? null : "0x" +
com.google.common.io.BaseEncoding.base16().encode((byte[]) value);
case DATE:
Review Comment:
[P1] Decode binary partition hex in the V1 ORC reader. For a native Paimon
ORC-only scan with `enable_file_scanner_v2=false`, this sends `0x00FF80`
through `columns_from_path`, but V1's ORC reader uses the partition filler and
VARBINARY JSON deserializer, which inserts those eight ASCII bytes instead of
`00 FF 80`. V2 uses `from_string` and decodes correctly. Make the V1 partition
fill decode the typed hex and add a V1 readback test.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/jdbc/source/JdbcScanNode.java:
##########
@@ -213,13 +219,154 @@ private String getTvfQuery() {
if (!projectsTimestamps) {
return query;
}
- String source = query.trim();
- if (source.endsWith(";")) {
- source = source.substring(0, source.length() - 1);
+ boolean mysql = jdbcType == TOdbcTableType.MYSQL || jdbcType ==
TOdbcTableType.OCEANBASE;
+ if (mysql && noBackslashEscapes == null) {
+ // Resolve only for wrapped MySQL queries and reuse the result for
EXPLAIN and scan serialization.
+ org.apache.doris.datasource.jdbc.JdbcExternalCatalog catalog =
+ (org.apache.doris.datasource.jdbc.JdbcExternalCatalog)
org.apache.doris.catalog.Env
+
.getCurrentEnv().getCatalogMgr().getCatalog(catalogId);
+ noBackslashEscapes =
catalog.getJdbcClient().isNoBackslashEscapes();
+ }
+ String source = stripTerminalDelimiter(query.trim(), mysql &&
noBackslashEscapes);
+ // WITH SESSION must stay outside the derived table; terminate
trailing line comments with a newline.
+ int start = jdbcType == TOdbcTableType.TRINO ?
trinoSessionQueryStart(source) : 0;
+ return source.substring(0, start) + "SELECT " + Joiner.on(",
").join(columns)
+ + " FROM (" + source.substring(start) + "\n)
doris_jdbc_source";
+ }
+
+ private String stripTerminalDelimiter(String sql, boolean
noBackslashEscapes) {
+ boolean mysql = jdbcType == TOdbcTableType.MYSQL || jdbcType ==
TOdbcTableType.OCEANBASE;
+ List<Integer> delimiters = new java.util.ArrayList<>();
+ for (int i = 0; i < sql.length();) {
+ char c = sql.charAt(i);
+ if (Character.isWhitespace(c)) {
+ i++;
+ // MySQL's second dash needs a following whitespace/control
character; --1 is arithmetic.
+ } else if ((sql.startsWith("--", i) && (!mysql || (i + 2 <
sql.length()
+ && (Character.isWhitespace(sql.charAt(i + 2)) ||
Character.isISOControl(sql.charAt(i + 2))))))
+ || (c == '#'
+ && (jdbcType == TOdbcTableType.MYSQL || jdbcType ==
TOdbcTableType.OCEANBASE))) {
+ while (i < sql.length() && sql.charAt(i) != '\n' &&
sql.charAt(i) != '\r') {
+ i++;
+ }
+ } else if (sql.startsWith("/*", i)) {
+ int depth = 1;
+ i += 2;
+ while (i < sql.length() && depth > 0) {
+ if (sql.startsWith("/*", i)) {
+ depth++;
+ i += 2;
+ } else if (sql.startsWith("*/", i)) {
+ depth--;
+ i += 2;
+ } else {
+ i++;
+ }
+ }
+ } else if (c == '\'' || c == '"' || c == '`') {
+ char quote = c;
+ for (i++; i < sql.length(); i++) {
+ // SQL mode belongs to the remote connection, not the
Doris session.
+ if (sql.charAt(i) == '\\' && !(mysql && noBackslashEscapes)
+ && quote != '`' && jdbcType !=
TOdbcTableType.TRINO && jdbcType != TOdbcTableType.PRESTO) {
+ i++;
+ } else if (sql.charAt(i) == quote) {
+ if (i + 1 < sql.length() && sql.charAt(i + 1) ==
quote) {
+ i++;
+ } else {
+ i++;
+ break;
+ }
+ }
+ }
+ delimiters.clear();
+ } else {
+ if (c == ';') {
+ delimiters.add(i);
+ } else {
+ delimiters.clear();
+ }
+ i++;
+ }
+ }
+ // A statement delimiter may precede trailing comments, which must
stay outside SQL literals.
+ // Remove only the terminal delimiter run; interior delimiters still
fail as multi-statements.
+ if (delimiters.isEmpty()) {
+ return sql;
+ }
+ StringBuilder result = new StringBuilder(sql);
+ for (int i = delimiters.size() - 1; i >= 0; i--) {
+ result.deleteCharAt(delimiters.get(i));
}
- return "SELECT " + Joiner.on(", ").join(columns) + " FROM (" + source
+ ") doris_jdbc_source";
+ return result.toString();
}
+ private static int trinoSessionQueryStart(String sql) {
+ int depth = 0;
+ int prefixWords = 0;
+ for (int i = 0; i < sql.length();) {
+ char c = sql.charAt(i);
+ if (c == '\'' || c == '"') {
+ char quote = c;
+ for (i++; i < sql.length(); i++) {
+ if (sql.charAt(i) == quote) {
+ if (i + 1 < sql.length() && sql.charAt(i + 1) ==
quote) {
+ i++;
+ } else {
+ i++;
+ break;
+ }
+ }
+ }
+ } else if (sql.startsWith("--", i)) {
+ int newline = sql.indexOf('\n', i + 2);
+ i = newline < 0 ? sql.length() : newline + 1;
+ } else if (sql.startsWith("/*", i)) {
+ int comments = 1;
+ i += 2;
+ while (i < sql.length() && comments > 0) {
+ if (sql.startsWith("/*", i)) {
+ comments++;
Review Comment:
[P2] Match the remote block-comment rule in both wrapper scanners. Trino
ends `/* note /* marker */` at its first `*/`, so `WITH SESSION time_zone='UTC'
/* note /* marker */ SELECT ts FROM t` is a valid root query. This loop counts
the inner `/*` as nested, never sees the later `SELECT`, and returns 0;
`getTvfQuery` then places `WITH SESSION` inside `FROM (...)`, where it is
invalid. The same nesting assumption in `stripTerminalDelimiter` can hide a
trailing semicolon. Stop at the first `*/` in both scanners and add both TVF
cases.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/jdbc/source/JdbcScanNode.java:
##########
@@ -213,13 +219,154 @@ private String getTvfQuery() {
if (!projectsTimestamps) {
return query;
}
- String source = query.trim();
- if (source.endsWith(";")) {
- source = source.substring(0, source.length() - 1);
+ boolean mysql = jdbcType == TOdbcTableType.MYSQL || jdbcType ==
TOdbcTableType.OCEANBASE;
+ if (mysql && noBackslashEscapes == null) {
+ // Resolve only for wrapped MySQL queries and reuse the result for
EXPLAIN and scan serialization.
+ org.apache.doris.datasource.jdbc.JdbcExternalCatalog catalog =
+ (org.apache.doris.datasource.jdbc.JdbcExternalCatalog)
org.apache.doris.catalog.Env
+
.getCurrentEnv().getCatalogMgr().getCatalog(catalogId);
+ noBackslashEscapes =
catalog.getJdbcClient().isNoBackslashEscapes();
+ }
+ String source = stripTerminalDelimiter(query.trim(), mysql &&
noBackslashEscapes);
+ // WITH SESSION must stay outside the derived table; terminate
trailing line comments with a newline.
+ int start = jdbcType == TOdbcTableType.TRINO ?
trinoSessionQueryStart(source) : 0;
+ return source.substring(0, start) + "SELECT " + Joiner.on(",
").join(columns)
+ + " FROM (" + source.substring(start) + "\n)
doris_jdbc_source";
+ }
+
+ private String stripTerminalDelimiter(String sql, boolean
noBackslashEscapes) {
+ boolean mysql = jdbcType == TOdbcTableType.MYSQL || jdbcType ==
TOdbcTableType.OCEANBASE;
+ List<Integer> delimiters = new java.util.ArrayList<>();
+ for (int i = 0; i < sql.length();) {
+ char c = sql.charAt(i);
+ if (Character.isWhitespace(c)) {
+ i++;
+ // MySQL's second dash needs a following whitespace/control
character; --1 is arithmetic.
+ } else if ((sql.startsWith("--", i) && (!mysql || (i + 2 <
sql.length()
+ && (Character.isWhitespace(sql.charAt(i + 2)) ||
Character.isISOControl(sql.charAt(i + 2))))))
+ || (c == '#'
+ && (jdbcType == TOdbcTableType.MYSQL || jdbcType ==
TOdbcTableType.OCEANBASE))) {
+ while (i < sql.length() && sql.charAt(i) != '\n' &&
sql.charAt(i) != '\r') {
+ i++;
+ }
+ } else if (sql.startsWith("/*", i)) {
+ int depth = 1;
+ i += 2;
+ while (i < sql.length() && depth > 0) {
+ if (sql.startsWith("/*", i)) {
+ depth++;
+ i += 2;
+ } else if (sql.startsWith("*/", i)) {
+ depth--;
+ i += 2;
+ } else {
+ i++;
+ }
+ }
+ } else if (c == '\'' || c == '"' || c == '`') {
+ char quote = c;
+ for (i++; i < sql.length(); i++) {
+ // SQL mode belongs to the remote connection, not the
Doris session.
+ if (sql.charAt(i) == '\\' && !(mysql && noBackslashEscapes)
+ && quote != '`' && jdbcType !=
TOdbcTableType.TRINO && jdbcType != TOdbcTableType.PRESTO) {
Review Comment:
[P2] Handle `ANSI_QUOTES` when scanning double quotes. With MySQL
`sql_mode=ANSI_QUOTES`, `SELECT ts, 1 AS "x\" FROM t;` is valid: the alias ends
in a literal backslash. This branch skips its closing `"` because
`NO_BACKSLASH_ESCAPES` is absent, so `stripTerminalDelimiter` leaves the
semicolon and `getTvfQuery` places `... FROM t;` inside the derived table.
Timestamp TVF scans then fail. Read the identifier-quoting mode as well and
test this case.
##########
fe/be-java-extensions/jdbc-scanner/src/main/java/org/apache/doris/jdbc/TrinoJdbcExecutor.java:
##########
@@ -42,13 +42,22 @@
public class TrinoJdbcExecutor extends BaseJdbcExecutor {
private static final DateTimeFormatter TIMESTAMP_TZ_WRITE_FORMATTER =
DateTimeFormatter.ofPattern("uuuu-MM-dd HH:mm:ss.SSSSSS");
+ private static final DateTimeFormatter ARRAY_TIMESTAMP_FORMATTER = new
java.time.format.DateTimeFormatterBuilder()
+ .appendPattern("uuuu-MM-dd HH:mm:ss")
+ .appendFraction(java.time.temporal.ChronoField.NANO_OF_SECOND, 0,
9, true)
+ .appendLiteral(' ').appendZoneOrOffsetId().toFormatter();
public TrinoJdbcExecutor(byte[] thriftParams) throws Exception {
super(thriftParams);
}
@Override
protected void setTimestampTz(int parameterIndex, LocalDateTime value)
throws SQLException {
+ if (config != null && config.getTableType() ==
org.apache.doris.thrift.TOdbcTableType.PRESTO) {
+ // PrestoDB requires VARCHAR plus a server-side cast; its driver
rejects JDBC type 2014.
Review Comment:
[P2] Exercise the Presto branch in the server integration tests. Both
`testPrestoTimestampWriteRoundTrip` and
`testPrestoUtcProjectionPreservesNamedZoneOverlap` create partial
`TrinoJdbcExecutor` mocks with `config == null`, so this `PRESTO` branch and
the new scalar `getTimestamp` branch are skipped. The write helper also uses
raw `?` instead of the new FE `CAST` placeholder. Set the test executor's table
type to PRESTO and use the production cast so these tests validate the driver
path this change adds.
##########
fe/be-java-extensions/jdbc-scanner/src/main/java/org/apache/doris/jdbc/OracleJdbcExecutor.java:
##########
@@ -50,8 +52,27 @@ public OracleJdbcExecutor(byte[] thriftParams) throws
Exception {
@Override
protected void setTimestampTz(int parameterIndex, LocalDateTime value)
throws SQLException {
- // An unzoned TIMESTAMP bind is session-local for both Oracle TZ and
LOCAL TIME ZONE columns.
- preparedStatement.setObject(parameterIndex,
value.atOffset(ZoneOffset.UTC), Types.TIMESTAMP_WITH_TIMEZONE);
+ // Older Oracle drivers reject JDBC type 2014; the sink SQL parses an
explicit UTC offset.
+ preparedStatement.setString(parameterIndex,
value.format(TIMESTAMP_TZ_FORMATTER) + " +00:00");
+ }
Review Comment:
[P2] Update the Oracle write integration test for the text bind.
`testOracleTimestampWriteRoundTrip` still prepares raw `INSERT ... VALUES (?)`
and calls this setter, while only `JdbcTable.getInsertSql` now wraps the
placeholder in `TO_TIMESTAMP_TZ(..., 'YYYY-MM-DD HH24:MI:SS.FF6 TZH:TZM')`.
With a different `NLS_TIMESTAMP_TZ_FORMAT`, the conditional test's implicit
conversion fails and never validates the production write path. Use the
explicit cast in that test for both TZ and local TZ columns.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
