github-actions[bot] commented on code in PR #65548:
URL: https://github.com/apache/doris/pull/65548#discussion_r3584565920
##########
be/src/format_v2/parquet/parquet_reader.cpp:
##########
@@ -454,6 +508,12 @@ Status
ParquetReader::open(std::shared_ptr<format::FileScanRequest> request) {
DORIS_CHECK(local_id >= 0 && local_id < num_fields);
}
+ // Reject requested unsupported logical leaves before row-group
statistics, dictionaries,
+ // bloom filters or page indexes inspect their physical fallback type. For
example, a predicate
+ // on TIME_MILLIS must fail here even when its INT32 statistics would
prune every row group;
+ // otherwise the same unsupported SELECT could fail or silently succeed
depending on data.
+ RETURN_IF_ERROR(validate_requested_columns_supported(_state->file_schema,
*request_snapshot));
+
Review Comment:
This validates every predicate column, but Iceberg can add a predicate
column that the query did not actually request. When all equality-delete keys
are missing from the data file, `_append_equality_delete_row_count_carrier()`
appends the first `DATA_COLUMN` only so the delete predicate has a batch row
count, and final materialization ignores that hidden dependency. If the first
physical data column is an unsupported logical leaf such as `TIME_MILLIS`, a
query that projects only supported columns now fails here because the carrier
is treated like a real requested column. Please choose a supported carrier, or
use a virtual row-position carrier that this validation already skips, before
rejecting unsupported projected leaves.
##########
be/src/format_v2/column_mapper.cpp:
##########
@@ -843,6 +843,192 @@ static bool rewrite_struct_element_path_to_file_expr(
return true;
}
+static VExprSPtr cast_file_expr_to_table_type(const VExprSPtr& file_expr,
+ const DataTypePtr& table_type,
+ RewriteContext* rewrite_context)
{
+ DORIS_CHECK(file_expr != nullptr);
+ DORIS_CHECK(table_type != nullptr);
+ DORIS_CHECK(rewrite_context != nullptr);
+ auto cast_expr = Cast::create_shared(table_type);
+ cast_expr->add_child(file_expr);
+ rewrite_context->add_created_expr(cast_expr);
+ return cast_expr;
+}
+
+// Prefer comparing in the physical file leaf type when a table predicate uses
a promoted struct
+// child. For example, with table STRUCT<a: BIGINT>, old-file STRUCT<a: INT>,
and `s.a = 10`, the
+// localized predicate should be `file_s.a::INT = 10::INT`, not
+// `CAST(file_s.a::INT AS BIGINT) = 10::BIGINT`. Converting one literal avoids
a cast for every row.
+//
+// This rewrite is valid only when every possible file value survives
file-to-table conversion and
+// the particular literal survives a table-to-file-to-table round trip. A
value such as BIGINT
+// 2147483648 cannot be represented by an INT file leaf, so that case
deliberately falls back to
+// `CAST(file_s.a AS BIGINT) = 2147483648`, which preserves the original
table-level semantics.
+static bool rewrite_binary_struct_literal_predicate(
+ const VExprSPtr& expr, const std::vector<ColumnMapping>&
filter_mappings,
+ const std::map<GlobalIndex, FileSlotRewriteInfo>& global_to_file_slot,
+ RewriteContext* rewrite_context) {
+ if (!is_binary_comparison_predicate(expr)) {
+ return false;
+ }
+ auto children = expr->children();
+ int struct_child_idx = -1;
+ int literal_child_idx = -1;
+ if (is_struct_element_expr(children[0])) {
+ struct_child_idx = 0;
+ literal_child_idx = 1;
+ } else if (is_struct_element_expr(children[1])) {
+ struct_child_idx = 1;
+ literal_child_idx = 0;
+ } else {
+ return false;
+ }
+
+ const auto table_leaf_type = children[struct_child_idx]->data_type();
+ DORIS_CHECK(table_leaf_type != nullptr);
+ auto table_literal =
unwrap_literal_for_file_cast(children[literal_child_idx], table_leaf_type);
+ if (table_literal == nullptr ||
+ !rewrite_struct_element_path_to_file_expr(children[struct_child_idx],
filter_mappings,
+ global_to_file_slot,
rewrite_context)) {
+ return false;
+ }
+
+ const auto file_leaf_type = children[struct_child_idx]->data_type();
+ DORIS_CHECK(file_leaf_type != nullptr);
+ const FileSlotRewriteInfo leaf_rewrite_info {
+ .block_position = 0,
+ .file_type = file_leaf_type,
+ .table_type = table_leaf_type,
+ .file_column_name = {},
+ };
+ auto file_literal =
+ rewrite_literal_to_file_type(table_literal, leaf_rewrite_info,
rewrite_context);
+ if (file_literal != nullptr) {
+ children[literal_child_idx] = std::move(file_literal);
Review Comment:
This localizes the struct-child comparison whenever the numeric value
round-trips through the file leaf type, but the proof ignores nullability.
`is_lossless_file_to_table_numeric_cast()` removes nullable from both sides, so
an old file leaf `Nullable(INT)` mapped to a required table leaf `INT` is
treated as safe. With a filter like `WHERE s.a > 10`, the new file-local
predicate can drop rows where `s.a` is NULL before TableReader materializes the
struct and `_align_column_nullability()` rejects NULLs under the required table
type. The same file without pushdown would fail that table contract, while the
pushed predicate silently succeeds on the surviving rows. Please keep these
struct literal predicates at table level unless the file leaf nullability is
also compatible with the table leaf, or add the same nullability proof used by
materialization before returning a file-domain literal.
--
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]
