github-actions[bot] commented on code in PR #68148:
URL: https://github.com/apache/doris/pull/68148#discussion_r4055906685
##########
be/src/format/parquet/parquet_predicate.h:
##########
@@ -211,11 +212,116 @@ 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 physical_stat_width_ok(const FieldSchema* col_schema, const
std::string& value) {
+ switch (col_schema->parquet_schema.type) {
+ case tparquet::Type::type::BOOLEAN:
Review Comment:
Width 1 is not enough to make a BOOLEAN statistic safe to reinterpret as C++
`bool`. Parquet PLAIN stores the value in the low bit and readers must accept
arbitrary padding bits, so byte `0x02` represents false; the later
`reinterpret_cast<const bool*>` instead uses an invalid C++ bool representation
and can publish true, allowing `= false` pruning to drop matching rows. Decode
`static_cast<uint8_t>(byte) & 1`, as the v1 row decoder and v2 statistics path
do, and cover a padding-bit case.
##########
be/src/format/parquet/parquet_predicate.h:
##########
@@ -211,11 +212,116 @@ 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 physical_stat_width_ok(const FieldSchema* col_schema, const
std::string& value) {
+ switch (col_schema->parquet_schema.type) {
+ case tparquet::Type::type::BOOLEAN:
+ return value.size() == 1;
+ case tparquet::Type::type::INT32:
Review Comment:
Validate INT32 DATE values, not only their byte width, before publishing the
zone map. Raw bounds `[-719528, -719527]` are ordered and four bytes each, but
`Int32ToDate` maps the first through day number 0: the dictionary fallback
ignores the failed date addition and leaves `1900-01-01`, while the second
becomes `0000-01-01`. Parsing therefore succeeds with `min > max`, and equality
on the year-0 max row can be pruned. Please reuse the v2 widened day-number
validation (and reject decoded inversion) and add this boundary case.
##########
be/src/format/parquet/parquet_predicate.h:
##########
@@ -211,11 +212,116 @@ 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 physical_stat_width_ok(const FieldSchema* col_schema, const
std::string& value) {
+ switch (col_schema->parquet_schema.type) {
+ case tparquet::Type::type::BOOLEAN:
+ return value.size() == 1;
+ case tparquet::Type::type::INT32:
+ case tparquet::Type::type::FLOAT:
+ return value.size() == 4;
+ case tparquet::Type::type::INT64:
+ case tparquet::Type::type::DOUBLE:
+ return value.size() == 8;
+ case tparquet::Type::type::INT96:
+ return value.size() == sizeof(ParquetInt96);
+ case tparquet::Type::type::FIXED_LEN_BYTE_ARRAY:
+ return col_schema->parquet_schema.type_length > 0 &&
+ value.size() ==
static_cast<size_t>(col_schema->parquet_schema.type_length);
+ case tparquet::Type::type::BYTE_ARRAY:
Review Comment:
BYTE_ARRAY is not length-agnostic when the logical type is DECIMAL. This
branch accepts any statistic length, but `StringToDecimal` later does
`memcpy(&value, ..., len)` into a fixed 4/8/16/32-byte native decimal and then
shifts by `sizeof(value) - len`; an oversized external min/max therefore writes
past the stack object before a conservative status can be returned. This is
distinct from the fixed-width thread because the bound must come from the
logical decimal destination. Validate `len <= sizeof(ValueCopyType)` (or decode
through a bounded temporary) and add an oversized BYTE_ARRAY decimal-stat test.
##########
be/src/format/parquet/parquet_predicate.h:
##########
@@ -211,11 +212,116 @@ 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 physical_stat_width_ok(const FieldSchema* col_schema, const
std::string& value) {
+ switch (col_schema->parquet_schema.type) {
+ case tparquet::Type::type::BOOLEAN:
+ return value.size() == 1;
+ case tparquet::Type::type::INT32:
+ case tparquet::Type::type::FLOAT:
+ return value.size() == 4;
+ case tparquet::Type::type::INT64:
+ case tparquet::Type::type::DOUBLE:
+ return value.size() == 8;
+ case tparquet::Type::type::INT96:
+ return value.size() == sizeof(ParquetInt96);
+ case tparquet::Type::type::FIXED_LEN_BYTE_ARRAY:
+ return col_schema->parquet_schema.type_length > 0 &&
+ value.size() ==
static_cast<size_t>(col_schema->parquet_schema.type_length);
+ case tparquet::Type::type::BYTE_ARRAY:
+ // A variable-length physical value; the string decode path is
length-agnostic.
+ return true;
+ default:
+ return true;
+ }
+ }
+
+ // An INT64 timestamp's converted min/max is only a usable pruning bound
when the interval is not
+ // corrupt (raw min <= raw max) and, for an adjusted-to-UTC value shown as
DATETIMEV2, the UTC
+ // interval crosses no backward clock transition (converting to local
civil time is not monotonic
+ // across a fall-back). TIMESTAMPTZ keeps UTC ordering, and a non-adjusted
value is shown in UTC,
+ // so both only need the inversion check. Defers the transition test to
the shared v2 helper.
+ // INT96 is handled by its own raw singleton rule before this is reached.
+ static bool int64_timestamp_range_is_usable(const FieldSchema* col_schema,
+ const std::string& encoded_min,
+ const std::string& encoded_max,
+ const cctz::time_zone& ctz,
+ PrimitiveType
logical_prim_type) {
+ if (col_schema->parquet_schema.type != tparquet::Type::type::INT64) {
+ return true;
+ }
+ if (encoded_min.size() < sizeof(int64_t) || encoded_max.size() <
sizeof(int64_t)) {
+ return false;
+ }
+ const auto raw_min = *reinterpret_cast<const
int64_t*>(encoded_min.data());
+ const auto raw_max = *reinterpret_cast<const
int64_t*>(encoded_max.data());
+ // Check the raw interval before flooring: two values in the same
second can still be
+ // inverted (e.g. MICROS 1_500_000 vs 1_000_000 both floor to second
1), and every caller
+ // trusts the published [min, max] as an ordered interval.
+ if (raw_min > raw_max) {
+ return false;
+ }
+ // TIMESTAMPTZ preserves the UTC ordering, so the inversion check
above is all it needs.
+ if (logical_prim_type != TYPE_DATETIMEV2) {
+ 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;
+ }
+ }
+ // A non-adjusted timestamp is shown in UTC (the converter uses UTC0,
which has no
+ // transitions), so it is monotonic and usable.
+ if (!adjusted) {
Review Comment:
Reject out-of-range INT64 timestamp statistics before taking this exemption.
For TIMESTAMP_MICROS, raw `min=-62167219201000000` and `max=-62167219200000000`
are ordered, but conversion crosses cctz civil year -1 to Doris year 0: the
lower endpoint falls back through a `uint16_t` year and becomes year 65535,
while the max becomes year 0. The successful parse therefore publishes `min >
max`, and equality on `0000-01-01` can prune the row containing that max. This
is distinct from the fixed raw-inversion case because raw order is correct;
please reuse the v2 supported-range validation (and reject decoded inversion)
before trusting these bounds.
##########
be/src/format/parquet/parquet_predicate.h:
##########
@@ -211,11 +212,116 @@ 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 physical_stat_width_ok(const FieldSchema* col_schema, const
std::string& value) {
+ switch (col_schema->parquet_schema.type) {
+ case tparquet::Type::type::BOOLEAN:
+ return value.size() == 1;
+ case tparquet::Type::type::INT32:
+ case tparquet::Type::type::FLOAT:
+ return value.size() == 4;
+ case tparquet::Type::type::INT64:
+ case tparquet::Type::type::DOUBLE:
+ return value.size() == 8;
+ case tparquet::Type::type::INT96:
+ return value.size() == sizeof(ParquetInt96);
+ case tparquet::Type::type::FIXED_LEN_BYTE_ARRAY:
+ return col_schema->parquet_schema.type_length > 0 &&
+ value.size() ==
static_cast<size_t>(col_schema->parquet_schema.type_length);
+ case tparquet::Type::type::BYTE_ARRAY:
+ // A variable-length physical value; the string decode path is
length-agnostic.
+ return true;
+ default:
+ return true;
+ }
+ }
+
+ // An INT64 timestamp's converted min/max is only a usable pruning bound
when the interval is not
+ // corrupt (raw min <= raw max) and, for an adjusted-to-UTC value shown as
DATETIMEV2, the UTC
+ // interval crosses no backward clock transition (converting to local
civil time is not monotonic
+ // across a fall-back). TIMESTAMPTZ keeps UTC ordering, and a non-adjusted
value is shown in UTC,
+ // so both only need the inversion check. Defers the transition test to
the shared v2 helper.
+ // INT96 is handled by its own raw singleton rule before this is reached.
+ static bool int64_timestamp_range_is_usable(const FieldSchema* col_schema,
+ const std::string& encoded_min,
+ const std::string& encoded_max,
+ const cctz::time_zone& ctz,
+ PrimitiveType
logical_prim_type) {
+ if (col_schema->parquet_schema.type != tparquet::Type::type::INT64) {
+ return true;
+ }
+ if (encoded_min.size() < sizeof(int64_t) || encoded_max.size() <
sizeof(int64_t)) {
+ return false;
+ }
+ const auto raw_min = *reinterpret_cast<const
int64_t*>(encoded_min.data());
+ const auto raw_max = *reinterpret_cast<const
int64_t*>(encoded_max.data());
+ // Check the raw interval before flooring: two values in the same
second can still be
+ // inverted (e.g. MICROS 1_500_000 vs 1_000_000 both floor to second
1), and every caller
+ // trusts the published [min, max] as an ordered interval.
+ if (raw_min > raw_max) {
+ return false;
+ }
+ // TIMESTAMPTZ preserves the UTC ordering, so the inversion check
above is all it needs.
+ if (logical_prim_type != TYPE_DATETIMEV2) {
+ 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;
+ }
+ }
+ // A non-adjusted timestamp is shown in UTC (the converter uses UTC0,
which has no
+ // transitions), so it is monotonic and usable.
+ if (!adjusted) {
+ return true;
+ }
+ // Adjusted, but the TimeUnit is absent/unrecognized: the converter
still maps it to
+ // DATETIMEV2 and applies the session timezone, so the transition risk
remains. Without a
+ // unit the raw range cannot be floored to seconds, so reject
conservatively.
+ if (units_per_second == 0) {
Review Comment:
Move the malformed-unit/range preflight before `physical_convert`. With an
adjusted TIMESTAMP whose `TimeUnit` union is empty, `ConvertParams` leaves
`second_mask=1`; an eight-byte `INT64_MAX` statistic in a fixed `+08:00`
session reaches the signed `epoch_seconds + offset_seconds` calculation before
this later `units_per_second == 0` rejection. Thus rejected external metadata
can overflow during conversion instead of falling back conservatively. Please
validate the unit/raw range first (and make the fixed-offset addition checked),
with an extreme-bound test.
--
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]
