Copilot commented on code in PR #66230:
URL: https://github.com/apache/doris/pull/66230#discussion_r3672517630
##########
be/src/load/group_commit/wal/wal_reader.cpp:
##########
@@ -99,13 +99,42 @@ Status WalReader::_do_get_next_block(Block* block, size_t*
read_rows, bool* eof)
return Status::InternalError("read wal {} fail, pos {}, columns
size {}", _wal_path,
pos, src_block.columns());
}
- ColumnPtr column_ptr = src_block.get_by_position(pos).column;
- if (!column_ptr && slot_desc->is_nullable()) {
- column_ptr = make_nullable(column_ptr);
+ const auto& source_column = src_block.get_by_position(pos);
+ const auto& target_column = output_block_columns[index];
+ auto source_status = source_column.check_type_and_column_match();
+ if (!source_status.ok()) {
+ return Status::InternalError(
+ "Invalid WAL column while replaying WAL {}: slot={}
(unique_id={}), "
+ "source_position={}, error={}",
+ _wal_path, slot_desc->col_name(),
slot_desc->col_unique_id(), pos,
+ source_status.to_string());
}
- dst_block.insert(index, ColumnWithTypeAndName(std::move(column_ptr),
-
output_block_columns[index].type,
-
output_block_columns[index].name));
+
+ ColumnWithTypeAndName replay_column(source_column.column,
target_column.type,
+ target_column.name);
Review Comment:
The previous implementation handled the case where the source column pointer
is null for nullable slots. This new code no longer applies a nullable wrapper
(or otherwise materializes a valid column) when `source_column.column` is null,
but still proceeds to build `ColumnWithTypeAndName` and later inserts it into
`dst_block`. If WAL can legitimately contain a null column pointer for a
nullable slot (as implied by the removed logic), this may reintroduce a null
dereference or invalid column state later in execution. Consider restoring
explicit handling for `source_column.column == nullptr` when
`slot_desc->is_nullable()` by creating an appropriate nullable column (or a
default-constructed column of the expected type) before validation/insertion.
##########
be/src/load/group_commit/wal/wal_reader.cpp:
##########
@@ -99,13 +99,42 @@ Status WalReader::_do_get_next_block(Block* block, size_t*
read_rows, bool* eof)
return Status::InternalError("read wal {} fail, pos {}, columns
size {}", _wal_path,
pos, src_block.columns());
}
- ColumnPtr column_ptr = src_block.get_by_position(pos).column;
- if (!column_ptr && slot_desc->is_nullable()) {
- column_ptr = make_nullable(column_ptr);
+ const auto& source_column = src_block.get_by_position(pos);
+ const auto& target_column = output_block_columns[index];
+ auto source_status = source_column.check_type_and_column_match();
+ if (!source_status.ok()) {
+ return Status::InternalError(
+ "Invalid WAL column while replaying WAL {}: slot={}
(unique_id={}), "
+ "source_position={}, error={}",
+ _wal_path, slot_desc->col_name(),
slot_desc->col_unique_id(), pos,
+ source_status.to_string());
}
- dst_block.insert(index, ColumnWithTypeAndName(std::move(column_ptr),
-
output_block_columns[index].type,
-
output_block_columns[index].name));
+
+ ColumnWithTypeAndName replay_column(source_column.column,
target_column.type,
+ target_column.name);
+ auto replay_status = replay_column.check_type_and_column_match();
+ if (!replay_status.ok()) {
+ return Status::InternalError(
+ "WAL replay column type mismatch: wal={}, slot={}
(unique_id={}), "
+ "source_position={}, source_name={}, source_type={},
source_column={}, "
+ "target_name={}, target_type={}, error={}",
+ _wal_path, slot_desc->col_name(),
slot_desc->col_unique_id(), pos,
+ source_column.name, source_column.type->get_name(),
+ source_column.column->get_name(), target_column.name,
+ target_column.type == nullptr ? "null" :
target_column.type->get_name(),
+ replay_status.to_string());
+ }
Review Comment:
PR description says type mismatches should return a **retryable** error, but
the new mismatch paths return `Status::InternalError(...)`. If callers rely on
error codes to decide whether WAL replay should retry, this may not achieve the
intended behavior. Please switch these mismatch returns to the project’s
retryable status/error code (whatever is used elsewhere for retry semantics)
while keeping the detailed context message.
##########
be/src/load/group_commit/wal/wal_reader.cpp:
##########
@@ -99,13 +99,42 @@ Status WalReader::_do_get_next_block(Block* block, size_t*
read_rows, bool* eof)
return Status::InternalError("read wal {} fail, pos {}, columns
size {}", _wal_path,
pos, src_block.columns());
}
- ColumnPtr column_ptr = src_block.get_by_position(pos).column;
- if (!column_ptr && slot_desc->is_nullable()) {
- column_ptr = make_nullable(column_ptr);
+ const auto& source_column = src_block.get_by_position(pos);
+ const auto& target_column = output_block_columns[index];
+ auto source_status = source_column.check_type_and_column_match();
+ if (!source_status.ok()) {
+ return Status::InternalError(
+ "Invalid WAL column while replaying WAL {}: slot={}
(unique_id={}), "
+ "source_position={}, error={}",
+ _wal_path, slot_desc->col_name(),
slot_desc->col_unique_id(), pos,
+ source_status.to_string());
}
- dst_block.insert(index, ColumnWithTypeAndName(std::move(column_ptr),
-
output_block_columns[index].type,
-
output_block_columns[index].name));
+
+ ColumnWithTypeAndName replay_column(source_column.column,
target_column.type,
+ target_column.name);
+ auto replay_status = replay_column.check_type_and_column_match();
+ if (!replay_status.ok()) {
+ return Status::InternalError(
+ "WAL replay column type mismatch: wal={}, slot={}
(unique_id={}), "
+ "source_position={}, source_name={}, source_type={},
source_column={}, "
+ "target_name={}, target_type={}, error={}",
+ _wal_path, slot_desc->col_name(),
slot_desc->col_unique_id(), pos,
+ source_column.name, source_column.type->get_name(),
+ source_column.column->get_name(), target_column.name,
+ target_column.type == nullptr ? "null" :
target_column.type->get_name(),
+ replay_status.to_string());
+ }
+ if (!source_column.type->equals(*target_column.type)) {
+ return Status::InternalError(
+ "WAL replay logical type mismatch: wal={}, slot={}
(unique_id={}), "
+ "source_position={}, source_name={}, source_type={},
target_name={}, "
+ "target_type={}",
+ _wal_path, slot_desc->col_name(),
slot_desc->col_unique_id(), pos,
+ source_column.name, source_column.type->get_name(),
target_column.name,
+ target_column.type->get_name());
+ }
Review Comment:
`target_column.type` is dereferenced in both `equals(*target_column.type)`
and `target_column.type->get_name()` without a null guard. Earlier error
formatting suggests `target_column.type` may be null (line 124 uses a ternary).
Please add an explicit check for `target_column.type == nullptr` before this
block and return an error with context, to avoid a potential crash.
--
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]
