github-actions[bot] commented on code in PR #65451:
URL: https://github.com/apache/doris/pull/65451#discussion_r3557033827
##########
be/src/format/orc/vorc_reader.cpp:
##########
@@ -3492,6 +3492,16 @@ void
ORCFileInputStream::_build_large_ranges_input_stripe_streams(
}
void OrcReader::_execute_filter_position_delete_rowids(IColumn::Filter&
filter, int64_t start_row) {
+ if (_deletion_vector != nullptr) {
Review Comment:
Adding the DV check here is not enough for ORC scans that have only a
deletion vector. In the no-conjunct path above, `_get_next_block_impl()` only
calls `_execute_filter_position_delete_rowids()` when `_delete_rows_filter_ptr`
is set or `_position_delete_ordered_rowids != nullptr`. A Paimon/Iceberg ORC
split with only a DV now sets `_deletion_vector` but leaves both of those
legacy fields unset, so a simple scan with no query predicates skips this
helper entirely and returns rows that the DV marks deleted. Please include
`_deletion_vector` in that call-site guard, and add an ORC DV read test without
query conjuncts.
##########
be/src/format_v2/table_reader.cpp:
##########
@@ -773,41 +786,58 @@ Status TableReader::_parse_delete_predicates(const
SplitReadOptions& options) {
DORIS_CHECK(options.cache != nullptr);
Status create_status = Status::OK();
- _delete_rows = options.cache->get<DeleteRows>(desc.key, [&]() ->
DeleteRows* {
- auto delete_rows = std::make_unique<DeleteRows>();
-
- DeletionVectorReader dv_reader(_runtime_state, _scanner_profile,
*_scan_params, desc,
- _io_ctx.get());
- create_status = dv_reader.open();
- if (!create_status.ok()) [[unlikely]] {
- return nullptr;
- }
-
- size_t bytes_read = desc.size;
- std::vector<char> buffer(bytes_read);
- DBUG_EXECUTE_IF("TableReader.parse_deletion_vector.io_error", {
- create_status = Status::IOError("injected format v2 deletion
vector read failure");
- return nullptr;
- });
- DBUG_EXECUTE_IF("TableReader.parse_deletion_vector.should_stop", {
- create_status = Status::EndOfFile("stop read.");
- return nullptr;
- });
- create_status = dv_reader.read_at(desc.start_offset,
{buffer.data(), bytes_read});
- if (!create_status.ok()) [[unlikely]] {
- return nullptr;
- }
-
- const char* buf = buffer.data();
- SCOPED_TIMER(_profile.parse_delete_file_time);
- create_status = parse_deletion_vector(buf, bytes_read,
desc.format, delete_rows.get());
- if (!create_status.ok()) [[unlikely]] {
- return nullptr;
- }
Review Comment:
This leaves Iceberg V2's DV sentinel stale.
`IcebergTableReader::_init_delete_predicates()` still treats `_delete_rows !=
nullptr` as "a deletion vector was parsed" and uses that to clear
`position_delete_files`, but this PR now stores the parsed DV in
`_deletion_vector` instead. For a split like the existing
`IcebergTableReaderIgnoresPositionDeleteFilesWhenDeletionVectorPresent` test,
`TableReader::prepare_split()` sets `_deletion_vector`, `_delete_rows` remains
null, `_init_position_delete_rows()` then reads the position-delete file, and
`_append_delete_predicate()` emits both predicates. That removes rows from the
DV and the position-delete file, contradicting the current Iceberg V2
behavior/test that position deletes are ignored when a DV is present. Please
update the Iceberg V2 guard to key off `_deletion_vector` (or otherwise make
the mixed-source behavior intentional and update the 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]
