github-actions[bot] commented on code in PR #68148:
URL: https://github.com/apache/doris/pull/68148#discussion_r4046745836
##########
be/src/format/parquet/parquet_predicate.h:
##########
@@ -211,6 +212,55 @@ class ParquetPredicate {
RowRange row_group_range;
};
+ // An adjusted-to-UTC INT64 timestamp mapped to DATETIMEV2 is displayed in
local civil time, so
+ // its converted min/max are only a usable bound when the UTC interval
contains no backward
+ // clock transition. Mirror the unit/adjust derivation in
TimestampConverter::init and defer the
+ // transition check to the shared v2 helper. Returns true (usable) for
anything that is not such
+ // a timestamp: a non-adjusted timestamp is shown in UTC (no transitions),
and INT96 is handled
+ // by its own singleton rule before this is reached.
+ static bool adjusted_utc_timestamp_range_is_monotonic(const FieldSchema*
col_schema,
+ const std::string&
encoded_min,
+ const std::string&
encoded_max,
+ const
cctz::time_zone& ctz) {
+ if (col_schema->parquet_schema.type != tparquet::Type::type::INT64) {
+ return true;
+ }
+ const auto& schema = col_schema->parquet_schema;
+ bool adjusted = false;
+ int64_t units_per_second = 0;
+ if (schema.__isset.logicalType &&
schema.logicalType.__isset.TIMESTAMP) {
+ const auto& ts = schema.logicalType.TIMESTAMP;
+ adjusted = ts.isAdjustedToUTC;
+ if (ts.unit.__isset.MILLIS) {
+ units_per_second = 1000;
+ } else if (ts.unit.__isset.MICROS) {
+ units_per_second = 1000000;
+ } else if (ts.unit.__isset.NANOS) {
+ units_per_second = 1000000000;
+ }
+ } else if (schema.__isset.converted_type) {
+ // Legacy TIMESTAMP_MILLIS / TIMESTAMP_MICROS carry instant
(UTC-normalized) semantics.
+ if (schema.converted_type ==
tparquet::ConvertedType::TIMESTAMP_MILLIS) {
+ adjusted = true;
+ units_per_second = 1000;
+ } else if (schema.converted_type ==
tparquet::ConvertedType::TIMESTAMP_MICROS) {
+ adjusted = true;
+ units_per_second = 1000000;
+ }
+ }
+ if (!adjusted || units_per_second == 0) {
Review Comment:
Do not treat an adjusted TIMESTAMP with no recognized TimeUnit as usable. A
present but empty (or forward-unknown) `TimeUnit` union is still mapped to
DATETIMEV2; `ConvertParams` leaves `second_mask=1` and keeps the session
timezone, whereas this branch returns before checking transitions. Raw seconds
`[1636263000,1636266600]` therefore recreate the New York rollback false-prune
this change is meant to fence. Please share the converter's resolved unit
semantics or conservatively reject these statistics, and add the malformed-unit
case.
##########
be/src/format/parquet/parquet_predicate.h:
##########
@@ -211,6 +212,55 @@ class ParquetPredicate {
RowRange row_group_range;
};
+ // An adjusted-to-UTC INT64 timestamp mapped to DATETIMEV2 is displayed in
local civil time, so
+ // its converted min/max are only a usable bound when the UTC interval
contains no backward
+ // clock transition. Mirror the unit/adjust derivation in
TimestampConverter::init and defer the
+ // transition check to the shared v2 helper. Returns true (usable) for
anything that is not such
+ // a timestamp: a non-adjusted timestamp is shown in UTC (no transitions),
and INT96 is handled
+ // by its own singleton rule before this is reached.
+ static bool adjusted_utc_timestamp_range_is_monotonic(const FieldSchema*
col_schema,
+ const std::string&
encoded_min,
+ const std::string&
encoded_max,
+ const
cctz::time_zone& ctz) {
+ if (col_schema->parquet_schema.type != tparquet::Type::type::INT64) {
+ return true;
+ }
+ const auto& schema = col_schema->parquet_schema;
+ bool adjusted = false;
+ int64_t units_per_second = 0;
+ if (schema.__isset.logicalType &&
schema.logicalType.__isset.TIMESTAMP) {
+ const auto& ts = schema.logicalType.TIMESTAMP;
+ adjusted = ts.isAdjustedToUTC;
+ if (ts.unit.__isset.MILLIS) {
+ units_per_second = 1000;
+ } else if (ts.unit.__isset.MICROS) {
+ units_per_second = 1000000;
+ } else if (ts.unit.__isset.NANOS) {
+ units_per_second = 1000000000;
+ }
+ } else if (schema.__isset.converted_type) {
+ // Legacy TIMESTAMP_MILLIS / TIMESTAMP_MICROS carry instant
(UTC-normalized) semantics.
+ if (schema.converted_type ==
tparquet::ConvertedType::TIMESTAMP_MILLIS) {
+ adjusted = true;
+ units_per_second = 1000;
+ } else if (schema.converted_type ==
tparquet::ConvertedType::TIMESTAMP_MICROS) {
+ adjusted = true;
+ units_per_second = 1000000;
+ }
+ }
+ if (!adjusted || units_per_second == 0) {
+ return true;
+ }
+ if (encoded_min.size() < sizeof(int64_t) || encoded_max.size() <
sizeof(int64_t)) {
+ return true;
+ }
+ const auto raw_min = *reinterpret_cast<const
int64_t*>(encoded_min.data());
+ const auto raw_max = *reinterpret_cast<const
int64_t*>(encoded_max.data());
+ return format::utc_timestamp_range_is_monotonic(
Review Comment:
Reject the raw inversion before flooring or taking the adjustment exemption.
For example, MICROS `raw_min=1500000` and `raw_max=1000000` both floor to
second 1, so this returns usable even though conversion publishes
`min=...01.500000` and `max=...01.000000`. All footer/page predicate and
expr-zone-map callers then trust that inverted interval and can prune a
matching value. The v2 path checks raw `min_value > max_value` first; please do
the same here (including non-adjusted/TIMESTAMPTZ paths) and cover the
same-second case.
##########
be/src/format/parquet/parquet_predicate.h:
##########
@@ -211,6 +212,55 @@ class ParquetPredicate {
RowRange row_group_range;
};
+ // An adjusted-to-UTC INT64 timestamp mapped to DATETIMEV2 is displayed in
local civil time, so
+ // its converted min/max are only a usable bound when the UTC interval
contains no backward
+ // clock transition. Mirror the unit/adjust derivation in
TimestampConverter::init and defer the
+ // transition check to the shared v2 helper. Returns true (usable) for
anything that is not such
+ // a timestamp: a non-adjusted timestamp is shown in UTC (no transitions),
and INT96 is handled
+ // by its own singleton rule before this is reached.
+ static bool adjusted_utc_timestamp_range_is_monotonic(const FieldSchema*
col_schema,
+ const std::string&
encoded_min,
+ const std::string&
encoded_max,
+ const
cctz::time_zone& ctz) {
+ if (col_schema->parquet_schema.type != tparquet::Type::type::INT64) {
+ return true;
+ }
+ const auto& schema = col_schema->parquet_schema;
+ bool adjusted = false;
+ int64_t units_per_second = 0;
+ if (schema.__isset.logicalType &&
schema.logicalType.__isset.TIMESTAMP) {
+ const auto& ts = schema.logicalType.TIMESTAMP;
+ adjusted = ts.isAdjustedToUTC;
+ if (ts.unit.__isset.MILLIS) {
+ units_per_second = 1000;
+ } else if (ts.unit.__isset.MICROS) {
+ units_per_second = 1000000;
+ } else if (ts.unit.__isset.NANOS) {
+ units_per_second = 1000000000;
+ }
+ } else if (schema.__isset.converted_type) {
+ // Legacy TIMESTAMP_MILLIS / TIMESTAMP_MICROS carry instant
(UTC-normalized) semantics.
+ if (schema.converted_type ==
tparquet::ConvertedType::TIMESTAMP_MILLIS) {
+ adjusted = true;
+ units_per_second = 1000;
+ } else if (schema.converted_type ==
tparquet::ConvertedType::TIMESTAMP_MICROS) {
+ adjusted = true;
+ units_per_second = 1000000;
+ }
+ }
+ if (!adjusted || units_per_second == 0) {
+ return true;
+ }
+ if (encoded_min.size() < sizeof(int64_t) || encoded_max.size() <
sizeof(int64_t)) {
Review Comment:
This length check is too late to protect external statistics: the INT64
switch arm has already dereferenced both strings as eight-byte values before
this helper runs, and the INT96 arm has already copied caller-controlled
lengths under release-disabled `DCHECK`s. Short INT64 bounds can read out of
bounds; equal 11-byte INT96 bounds are mislaid but pass raw equality; oversized
INT96 bounds can overrun the 24-byte destination. Validate exact physical
widths at the start of `parse_min_max_value` and return `DataQualityError`
before any load/copy.
##########
be/test/format/parquet/parquet_expr_test.cpp:
##########
@@ -2388,4 +2389,90 @@ TEST_F(ParquetExprTest,
test_bloom_filter_reused_after_first_load) {
EXPECT_EQ(2, loader_calls);
}
+namespace {
+
+std::string encode_i64(int64_t v) {
+ return std::string(reinterpret_cast<const char*>(&v), sizeof(v));
+}
+
+// #pragma pack(1) ParquetInt96 is {int64 lo (nanos in day); int32 hi (days
from julian epoch)}.
+std::string encode_int96(int64_t nanos_in_day, int32_t julian_day) {
+ std::string out;
+ out.append(reinterpret_cast<const char*>(&nanos_in_day),
sizeof(nanos_in_day));
+ out.append(reinterpret_cast<const char*>(&julian_day), sizeof(julian_day));
+ return out;
+}
+
+FieldSchema make_int64_utc_timestamp_schema(bool adjusted_to_utc) {
+ FieldSchema fs;
+ fs.parquet_schema.__set_type(tparquet::Type::INT64);
+ tparquet::MicroSeconds micros;
+ tparquet::TimeUnit unit;
+ unit.__set_MICROS(micros);
+ tparquet::TimestampType ts;
+ ts.__set_isAdjustedToUTC(adjusted_to_utc);
+ ts.__set_unit(unit);
+ tparquet::LogicalType lt;
+ lt.__set_TIMESTAMP(ts);
+ fs.parquet_schema.__set_logicalType(lt);
+ fs.data_type =
DataTypeFactory::instance().create_data_type(TYPE_DATETIMEV2, false, 0, 6);
+ return fs;
+}
+
+} // namespace
+
+// An adjusted-to-UTC timestamp whose min/max straddle a backward clock
transition is not a usable
+// bound: an interior instant can map outside the converted civil range. New
York falls back at
+// 2021-11-07 06:00 UTC.
+TEST_F(ParquetExprTest, ParseMinMaxRejectsTimestampRangeCrossingClockRollback)
{
+ cctz::time_zone ny;
+ ASSERT_TRUE(cctz::load_time_zone("America/New_York", &ny));
+ auto schema = make_int64_utc_timestamp_schema(/*adjusted_to_utc=*/true);
+ constexpr int64_t kUsPerSec = 1000000;
+
+ auto parse = [&](int64_t min_sec, int64_t max_sec, const cctz::time_zone&
tz) {
+ Field min_field;
+ Field max_field;
+ return ParquetPredicate::parse_min_max_value(&schema,
encode_i64(min_sec * kUsPerSec),
+ encode_i64(max_sec *
kUsPerSec), tz,
+ &min_field, &max_field);
+ };
+
+ // 05:30 .. 06:30 UTC crosses the 06:00 UTC rollback.
+ EXPECT_FALSE(parse(1636263000, 1636266600, ny).ok());
+ // Noon that day, no transition in range.
+ EXPECT_TRUE(parse(1636286400, 1636290000, ny).ok());
+ // Spring-forward on 2021-03-14 07:00 UTC must not be rejected for
crossing the gap.
+ EXPECT_TRUE(parse(1615703400, 1615707000, ny).ok());
+ // The same rollback range is fine when the timestamp is displayed in UTC
(not adjusted).
+ auto utc_schema =
make_int64_utc_timestamp_schema(/*adjusted_to_utc=*/false);
+ Field lo;
+ Field hi;
+ EXPECT_TRUE(
+ ParquetPredicate::parse_min_max_value(&utc_schema,
encode_i64(1636263000 * kUsPerSec),
+ encode_i64(1636266600 *
kUsPerSec), ny, &lo, &hi)
+ .ok());
+}
+
+// INT96 stats are only trustworthy when min == max (PARQUET-1065). This is
the page-index path,
+// which does not go through read_column_stats.
+TEST_F(ParquetExprTest, ParseMinMaxAppliesInt96SingletonRule) {
+ FieldSchema fs;
+ fs.parquet_schema.__set_type(tparquet::Type::INT96);
+ fs.data_type =
DataTypeFactory::instance().create_data_type(TYPE_DATETIMEV2, false, 0, 6);
+
+ const int32_t julian_2021_11_07 = ParquetInt96::JULIAN_EPOCH_OFFSET_DAYS +
18938;
+ auto a = encode_int96(12L * 3600 * 1000000000, julian_2021_11_07);
Review Comment:
Please make the unequal INT96 case distinguish raw-byte inequality from
decoded-value inequality. The current bounds differ by an hour, so a regression
to comparing decoded DATETIMEV2 values would still pass. Two encodings on the
same Julian day that differ only below one microsecond (for example
nanos-of-day `N+1` and `N+2`) decode to the same Doris value but must still be
rejected under the raw singleton rule.
--
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]
