github-actions[bot] commented on code in PR #65935:
URL: https://github.com/apache/doris/pull/65935#discussion_r3635722270
##########
be/src/format_v2/column_mapper.cpp:
##########
@@ -2012,8 +2296,16 @@ Status TableColumnMapper::localize_filters(const
std::vector<TableFilter>& table
filter_mappings = _filter_visible_mappings();
const auto global_to_file_slot =
build_file_slot_rewrite_map(filter_mappings, _filter_entries);
for (const auto& table_filter : table_filters) {
- if (table_filter.conjunct != nullptr &&
- table_filter_has_only_local_entries(table_filter,
_filter_entries)) {
+ if (table_filter.conjunct != nullptr && table_filter.conjunct->root()
!= nullptr) {
+ const auto root = table_filter.conjunct->root();
+ const auto impl = root->get_impl();
+ const auto predicate = impl != nullptr ? impl : root;
+ if (!predicate->is_deterministic() ||
Review Comment:
[P1] Preserve the unsafe-conjunct barrier during localization
This gate admits each deterministic/all-local filter independently, even
though `_build_table_filters_from_conjuncts()` keeps collecting filters after
the first unsafe original conjunct; it also does not reject a deterministic
predicate whose `is_safe_to_execute_on_selected_rows()` is false. For
`[mod(part, -1) > 0, id > 100]`, a split with `part=INT_MIN` and all ids at
most 100 can be filtered to EOF below `TableReader`, so Scanner never executes
the required `MIN_INT % -1` error and returns an empty result instead. A
deterministic unsafe cast/partial expression can likewise be cloned and
executed twice. Please carry the original safe-prefix eligibility into
localization and require the current predicate's safe-execution contract, with
regressions for both cases.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/source/IcebergScanNode.java:
##########
@@ -427,29 +477,49 @@ private String getDeleteFileContentType(int content) {
public void createScanRangeLocations() throws UserException {
super.createScanRangeLocations();
+ enableCurrentIcebergScanSemantics();
Review Comment:
[P1] Gate Iceberg semantics v1 during rolling upgrades
This unconditionally opts ordinary scans into result-changing fields that
old BEs ignore. The new `ReusedRootNameReadsNewFieldInitialDefault` case
demonstrates the split: Parquet column `b=10` still carries old field ID 1; a
new BE maps the reused current name `b` to field ID 2 and returns its initial
default `7`, while an old BE's ordered current-name/alias annotation maps `b`
back to field ID 1 and returns `10`. Unlike `$position_deletes`, ordinary scans
do not exclude smooth-upgrade-source BEs, so fragments of one query can return
different values. Please enable this contract only when every selected BE
supports it, or reject/fallback the whole scan during mixed-version operation.
##########
be/src/format_v2/table_reader.h:
##########
@@ -683,15 +777,12 @@ class TableReader {
Status finalize_chunk(Block* block, const size_t rows) {
SCOPED_TIMER(_profile.finalize_timer);
size_t idx = 0;
- for (const auto& mapping : _data_reader.column_mapper->mappings()) {
+ const auto& mappings = _data_reader.column_mapper->mappings();
+ for (const auto& mapping : mappings) {
ColumnPtr column;
RETURN_IF_ERROR(_materialize_mapping_column(mapping,
&_data_reader.block_template, rows,
- &column));
- // Keep projection columns shared with the file block. Before the
next read,
- // clear_column_data() replaces shared columns with clone_empty(),
so forcing a mutable
- // deep copy here only duplicates the just-read batch (which can
be several GB for
- // nested string/map columns).
- block->replace_by_position(idx, std::move(column));
+ &column, idx + 1 ==
mappings.size()));
Review Comment:
[P1] Release every projection source before detaching it
`take_projection_result` is true only for the globally last mapping. For
every earlier ordinary `VSlotRef`, `block_template` still owns the source when
`_detach_column()` calls recursive `IColumn::mutate()`, so STRING/MAP/ARRAY
payloads are deep-copied on every batch; if a default/virtual mapping is last,
every physical projection is copied. This reintroduces the several-GiB peak
duplication the removed implementation comment warned about, while
`LastProjectionDetaches...` covers only the one/last-mapping fast path. Please
release each source at its last use (or preserve shared immutable output
ownership) and cover multiple nested projections plus a trailing default
mapping.
--
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]
