github-actions[bot] commented on code in PR #68148:
URL: https://github.com/apache/doris/pull/68148#discussion_r4069220057
##########
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) {
Review Comment:
Validate the physical/logical annotation pair, not only each bound's size.
With physical `INT64` plus logical `DATE`, exact eight-byte statistics pass
this gate and build a `ColumnInt64`, but logical inference selects
`Int32ToDate`, which statically reads that object as `ColumnInt32`; footer
pruning reaches this before row-reader construction, so type-confused reads
occur before any conservative `Status`. Likewise, `FLOAT16` on
`FIXED_LEN_BYTE_ARRAY(1)` passes the declared width while its converter still
copies two bytes under a release-disabled `DCHECK`. This differs from the
wrong-stat-width thread because the strings match the malformed schema. Reuse
v2's `validate_physical_annotation` (preferably during v1 schema
initialization) before constructing converters, with tests for both
incompatible pairs.
##########
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:
Exact carrier width is not enough for a narrow logical `INTEGER`. For signed
`INT(8)`, a physical page `[-256, 2, 257]` has truthful raw bounds
`[-256,257]`, but `LittleIntPhysicalConverter` materializes the rows as
`[0,2,1]` and the two bounds as the still-ordered zone map `[0,1]`; `= 2` can
therefore prune the matching interior row. This is separate from DATE validity:
it is the logical integer bit-width contract, and v2's strict statistics path
checks `parquet_logical_integer_carrier_fits` before narrowing. Please validate
signed/unsigned carriers against the declared bit width before conversion and
cover this non-monotonic narrowing 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:
Review Comment:
The same exact-width problem exists for numeric DECIMAL carriers. `INT64` /
`DECIMAL(9,0)` is a valid physical pairing but selects DECIMAL32; a physical
page `[-4294967296, 2, 4294967297]` materializes permissively as `[0,2,1]`,
while its truthful raw bounds narrow unchecked in `NumberToDecimal` to the
ordered zone map `[0,1]`. An equality predicate for 2 can then prune the
matching interior row. This is separate from fixed/variable binary DECIMAL
paths. Validate INT32/INT64 decimal statistics against the selected native
width and declared precision before narrowing, as v2's strict decoder does, and
cover this 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:
Review Comment:
Finite floating endpoints still do not make this statistic a complete
pruning interval: Parquet requires writers to omit NaNs from min/max. A page
`{5.0, NaN}` therefore legitimately reports `[5.0,5.0]`, but Doris orders NaN
above finite values and treats it as unequal to 5. The footer/page `camp_field`
paths will consequently discard that page for both `> 10` and `!= 5`. Rejecting
only NaN endpoints cannot detect this case. Propagate an unknown/known
NaN-presence state and conservatively disable affected FLOAT/DOUBLE/FLOAT16
pruning, as the v2 zone-map path does, with footer and page-index coverage.
##########
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:
Review Comment:
For fixed DECIMAL, equality with `type_length` still does not make the
statistic safe for the logical destination. `FIXED_LEN_BYTE_ARRAY(17)` /
`DECIMAL(38,0)` maps to Doris DECIMAL128; for physical values `{0,1,2^128}`,
the truthful raw bounds `[0,2^128]` decode through an Int256 temporary and then
narrow unchecked to `[0,0]`, allowing `= 1` to prune the matching interior row.
This differs from the BYTE_ARRAY thread's oversized memcpy: these bounds match
the physical width, but violate destination width/precision. Validate fixed
decimal values against the destination native width and declared precision (or
reuse the v2 strict decoder), and add this exact-width 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:
Review Comment:
`INTERVAL` statistics must be ignored even when both values are exactly 12
bytes. V1 falls back from the valid `INTERVAL` / `FIXED_LEN_BYTE_ARRAY(12)`
pair to STRING and trusts the raw little-endian components as lexical bounds,
but Parquet defines no order for INTERVAL statistics. For month components
`{1,256,257}`, numeric bounds 1/257 encode as lexically ordered endpoints while
the present 256 encoding sorts below the encoded minimum, so equality on 256
can discard the page; a generic `min <= max` check will not catch this. Reject
INTERVAL footer/page statistics unconditionally (and gate other page statistics
on a supported type-defined order), with a page-index 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:
Review Comment:
This exact-width check also rejects bounds that `read_column_stats`
intentionally rewrites after reading valid external bytes. For unannotated
`FIXED_LEN_BYTE_ARRAY(3)` mapped to STRING, footer bounds `aaa` / `azz` are
transformed by `_try_read_old_utf8_stats` into `aaa` / the conservative upper
prefix `a{`; this branch then rejects the two-byte max and disables row-group
pruning for every caller. Please validate the original external fixed-width
values before the UTF-8 rewrite, then preserve a safe logical representation
(or safe padding) for the synthesized bound, and add a footer-path fixed-length
string test.
##########
be/src/format/parquet/parquet_predicate.h:
##########
@@ -339,20 +445,30 @@ class ParquetPredicate {
if (std::signbit(max_value) != 0 && max_value == -0.0F) {
max_value = 0.0F;
}
- } else if (col_schema->parquet_schema.type ==
tparquet::Type::type::INT96 ||
- logical_prim_type == TYPE_DATETIMEV2) {
- auto min_value = min_field->get<TYPE_DATETIMEV2>();
- auto max_value = min_field->get<TYPE_DATETIMEV2>();
-
+ } else if (col_schema->parquet_schema.type ==
tparquet::Type::type::INT96) {
Review Comment:
Move the INT96 singleton/range preflight before `physical_convert`. Exact
12-byte page-index bounds with `hi=INT32_MAX` reach
`ParquetInt96::to_timestamp_micros()` first, where the day-to-microsecond
multiplication overflows signed `int64_t` (the helper is marked
`NO_SANITIZE_UNDEFINED`), even when the raw bounds differ and this branch will
reject them. A byte-equal singleton with an extreme Julian day also converts
through that arithmetic and then passes the equality rule. This is distinct
from the wrong-width thread because both carriers are exactly 12 bytes. Reject
unequal bytes up front, then use widened arithmetic and the supported Doris
timestamp range to validate an equal singleton before converting it.
--
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]
