github-actions[bot] commented on code in PR #67784:
URL: https://github.com/apache/doris/pull/67784#discussion_r4027760510
##########
fe/fe-core/src/main/java/org/apache/doris/tablefunction/CdcStreamTableValuedFunction.java:
##########
@@ -226,6 +226,8 @@ public List<Column> getTableColumns() throws
AnalysisException {
throw new AnalysisException("Table does not exist: " + table);
}
List<Column> columns = new
ArrayList<>(jdbcClient.getColumnsFromJdbc(database, table));
+ // Use the CDC transport schema, not the external JDBC catalog's
timestamp mapping.
+ columns.forEach(column ->
column.setType(StreamingJobUtils.getCdcTimestampType(column.getType())));
Review Comment:
[P1] Keep CDC binary columns on the JSON transport type
The authoritative JDBC mappings now return VARBINARY for MySQL binary/BLOB
and PostgreSQL bytea, but this TVF's CDC producer returns `byte[]` to Jackson,
so the HTTP JSON contains Base64 text. The BE VARBINARY JSON reader copies that
string verbatim rather than decoding it; for example, source bytes `DE AD BE
EF` become the ASCII bytes of `3q2+7w==`. The auto-created CDC-table path
already normalizes VARBINARY to STRING. Please apply the same CDC schema
normalization here (or decode Base64 before advertising VARBINARY) and cover
snapshot and incremental binary rows.
##########
be/src/exprs/function/cast/cast_to_datetimev2_impl.hpp:
##########
@@ -658,16 +659,39 @@ inline bool
CastToDatetimeV2::from_string_strict_mode_internal(
// minute
SET_PARAMS_RET_FALSE_IFN((consume_digit<UInt32, 2>(ptr, end,
part[1])),
"invalid minute offset '{}'",
std::string {ptr, end});
- SET_PARAMS_RET_FALSE_IFN((part[1] == 0 || part[1] == 30 ||
part[1] == 45),
- "invalid minute offset '{}'",
part[1]);
+ if constexpr (type == DataTimeCastEnumType::TIMESTAMP_TZ) {
+ // TIMESTAMPTZ output preserves historical offsets,
including seconds and
+ // non-quarter-hour minutes. Keep the legacy DATETIME
parser unchanged.
+ SET_PARAMS_RET_FALSE_IFN(part[1] < 60, "invalid minute
offset '{}'", part[1]);
+ if (ptr < end && *ptr == ':') {
+ ++ptr;
+ SET_PARAMS_RET_FALSE_IFN(
+ (consume_digit<UInt32, 2>(ptr, end,
second_offset)),
+ "invalid second offset '{}'", std::string
{ptr, end});
+ SET_PARAMS_RET_FALSE_IFN(second_offset < 60, "invalid
second offset '{}'",
+ second_offset);
+ }
+ } else {
+ SET_PARAMS_RET_FALSE_IFN((part[1] == 0 || part[1] == 30 ||
part[1] == 45),
+ "invalid minute offset '{}'",
part[1]);
+ }
}
- SET_PARAMS_RET_FALSE_IFN(part[0] != 14 || part[1] == 0, "invalid
timezone offset '{}'",
- combine_tz_offset(sign, part[0],
part[1]));
-
- SET_PARAMS_RET_FALSE_IFN(TimezoneUtils::find_cctz_time_zone(
- combine_tz_offset(sign, part[0],
part[1]), parsed_tz),
+ SET_PARAMS_RET_FALSE_IFN(part[0] != 14 || (part[1] == 0 &&
second_offset == 0),
"invalid timezone offset '{}'",
combine_tz_offset(sign, part[0],
part[1]));
+
+ if (second_offset != 0) {
+ SET_PARAMS_RET_FALSE_IFN(sign != '-' || part[0] <= 12,
"invalid hour offset '{}'",
Review Comment:
[P2] Reject totals below the -12:00 offset boundary
This new guard checks only the hour component, so `-12:00:01` is accepted
and converted through `fixed_time_zone`; the newly relaxed minute path likewise
admits `-12:01` through the shared normalizer. Doris defines the supported
numeric range as `-12:00` through `+14:00`, and the positive side already
requires exact `+14:00:00`. Please validate the signed total before either
branch so an hour of 12 requires zero minutes and seconds, mirror it in the
fallback parser, and add strict/non-strict just-past-boundary cases.
--
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]
