Gabriel39 commented on code in PR #68532:
URL: https://github.com/apache/doris/pull/68532#discussion_r4120375303
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogProperty.java:
##########
@@ -103,37 +105,43 @@ public Map<String, String> getProperties() {
}
/**
- * @return true if varbinary mapping is enabled, false otherwise
+ * @return true because external binary types always retain their binary
semantics
*/
Review Comment:
This is the mixed-version rolling-upgrade compatibility window that was
explicitly excluded from the agreed scope. The finding is valid: until a new
master journals the migration, old and new FEs may expose different catalog
schemas. This PR intentionally makes logical binary/instant mappings mandatory
on new FEs; it does not add read fencing or retain the legacy mapping policy on
followers.
The limitation is documented in the PR description. This thread is closed as
an explicit scope decision, not as a claim that mixed-version compatibility is
fixed. The two DST correctness findings are addressed separately in this update.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonWriteBinding.java:
##########
@@ -194,13 +196,14 @@ private static String canonicalPartitionValue(
return value;
}
- // Doris writes an LTZ literal as civil time in the session zone.
Paimon 1.3
- // parses the string accepted by withOverwrite in the FE JVM default
zone.
- // Translate the same instant into that zone so the overwrite
predicate and
- // the row written by the JNI writer identify the same typed partition.
- LocalDateTime sessionValue = LocalDateTime.parse(
- value.replace(' ', 'T'),
DateTimeFormatter.ISO_LOCAL_DATE_TIME);
- return sessionValue.atZone(TimeUtils.getDorisZoneId())
+ // The write boundary now carries UTC instants. Do not parse its
display string as
+ // session-local time: that loses the offset and makes a DST fold
ambiguous again.
+ ZonedDateTime instant = literal instanceof TimestampTzLiteral
+ ? ((TimestampTzLiteral)
literal).toJavaDateType().atZone(ZoneId.of("UTC"))
+ : LocalDateTime.parse(value.replace(' ', 'T'),
DateTimeFormatter.ISO_LOCAL_DATE_TIME)
+ .atZone(TimeUtils.getDorisZoneId());
Review Comment:
Fixed in 901952c15f4. Static LTZ overwrite keys are now rejected when their
local representation has multiple valid offsets in the FE JVM zone. Both
instants of a DST fold are rejected because Paimon's offset-free withOverwrite
string cannot distinguish them. Appends retain their typed values and do not
use this overwrite filter.
Added separate failing-before-fix tests for 2023-11-05 08:30Z and 09:30Z in
an America/Los_Angeles JVM. Replaced the lossy expected-string assertion with
an actual Paimon TypeUtils.castFromString round trip against the original
instant: both values remain valid with a UTC JVM, and non-overlap values remain
valid in Los Angeles. Related Paimon binding and transaction tests pass.
##########
fe/be-java-extensions/jdbc-scanner/src/main/java/org/apache/doris/jdbc/TrinoJdbcExecutor.java:
##########
@@ -58,6 +60,11 @@ protected void initializeBlock(int columnCount, String[]
replaceStringList, int
@Override
protected Object getColumnValue(int columnIndex, ColumnType type, String[]
replaceStringList) throws SQLException {
switch (type.getType()) {
+ case TIMESTAMPTZ: {
+ // JNI carries instants as UTC components, not the source
zone's wall clock.
Review Comment:
Fixed in 901952c15f4. JdbcScanNode now projects Trino instant timestamps
with at_timezone(value, 'UTC') before JDBC decoding, recursively using
transform for arrays. This applies to catalog scans and query TVF wrappers;
local timestamp columns are unchanged.
Added failing-before-fix scalar/nested-array projection tests, plus executor
coverage for both overlap instants, nulls, empty arrays and negative fractional
timestamps. A standalone check using the real Trino JDBC 435 parsers reproduced
the named-zone overlap loss and passed 18 UTC-decoding checks across three JVM
zones for scalar and array timestamp paths.
152 targeted FE tests, 17 JDBC unit tests and both Checkstyle runs passed.
An opt-in live Trino integration test was added for the server projection plus
driver/executor path, but was not run locally because no Trino server
connection was configured.
--
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]
