morningman commented on a change in pull request #8069:
URL: https://github.com/apache/incubator-doris/pull/8069#discussion_r810080378
##########
File path: be/src/exec/parquet_reader.cpp
##########
@@ -121,7 +121,10 @@ Status ParquetReaderWrap::init_parquet_reader(const
std::vector<SlotDescriptor*>
}
void ParquetReaderWrap::close() {
- _parquet->Close();
+ arrow::Status st = _parquet->Close();
+ if (!st.ok()) {
+ LOG(WARNING) << "close parquet file error";
Review comment:
add more info in log
##########
File path: be/src/exec/parquet_writer.cpp
##########
@@ -42,7 +42,10 @@ ParquetOutputStream::ParquetOutputStream(FileWriter*
file_writer)
}
ParquetOutputStream::~ParquetOutputStream() {
- Close();
+ arrow::Status st = Close();
+ if (!st.ok()) {
+ LOG(WARNING) << "close parquet file error";
Review comment:
add more info in log
##########
File path: be/src/exec/parquet_reader.cpp
##########
@@ -537,7 +540,10 @@ Status ParquetReaderWrap::read(Tuple* tuple, const
std::vector<SlotDescriptor*>&
ParquetFile::ParquetFile(FileReader* file) : _file(file) {}
ParquetFile::~ParquetFile() {
- Close();
+ arrow::Status st = Close();
+ if (!st.ok()) {
+ LOG(WARNING) << "close parquet file error";
Review comment:
add more info in log
##########
File path: be/src/util/arrow/row_batch.cpp
##########
@@ -300,7 +300,7 @@ class FromRowBatchConverter : public arrow::TypeVisitor {
arrow::NumericBuilder<T> builder(_pool);
size_t num_rows = _batch.num_rows();
- builder.Reserve(num_rows);
+ ARROW_RETURN_NOT_OK(builder.Reserve(num_rows));
Review comment:
Can this be checked by clang?
##########
File path: be/src/vec/functions/function_rpc.cpp
##########
@@ -495,8 +495,8 @@ void convert_to_block(Block& block, const PValues& result,
size_t pos) {
null_map_data[i] = false;
}
}
- block.replace_by_position(
- pos, std::move(ColumnNullable::create(std::move(data_col),
std::move(null_col))));
+ block.replace_by_position(pos,
+ ColumnNullable::create(std::move(data_col),
std::move(null_col)));
Review comment:
Why not using std::move?
##########
File path: be/src/exec/parquet_writer.cpp
##########
@@ -463,7 +466,10 @@ void ParquetWriterWrapper::close() {
_rg_writer = nullptr;
}
_writer->Close();
- _outstream->Close();
+ arrow::Status st = _outstream->Close();
+ if (!st.ok()) {
+ LOG(WARNING) << "close parquet file error";
Review comment:
add more info in log. status + file name
##########
File path: be/src/olap/rowset/segment_v2/segment_iterator.cpp
##########
@@ -592,7 +592,8 @@ Status SegmentIterator::next_batch(RowBlockV2* block) {
void SegmentIterator::_vec_init_lazy_materialization() {
_is_pred_column.resize(_schema.columns().size(), false);
- std::set<ColumnId> pred_column_ids; // including short_cir_pred_col_id_set
and vec_pred_col_id_set
+ std::set<ColumnId>
Review comment:
strange format
##########
File path: be/src/vec/functions/function_cast.h
##########
@@ -374,9 +374,9 @@ struct ToNumberMonotonicity {
Float64 right_float = right.get<Float64>();
if (left_float >= std::numeric_limits<T>::min() &&
- left_float <= std::numeric_limits<T>::max() &&
+ left_float <=
static_cast<Float64>(std::numeric_limits<T>::max()) &&
Review comment:
Why no need `static_cast` for `left_float >=
std::numeric_limits<T>::min()`?
##########
File path: be/src/olap/decimal12.h
##########
@@ -109,12 +109,16 @@ struct decimal12_t {
integer = 999999999999999999;
fraction = 999999999;
} else {
+ int32_t f = 0;
+ int64_t i = 0;
if (sepr == value_string) {
- sscanf(value_string, ".%9d", &fraction);
- integer = 0;
+ int32_t f = 0;
Review comment:
Why using same name `f`?
##########
File path: be/src/vec/data_types/data_type_decimal.h
##########
@@ -330,19 +330,9 @@ convert_to_decimal(const typename FromDataType::FieldType&
value, UInt32 scale)
}
auto out = value * ToDataType::get_scale_multiplier(scale);
- if constexpr (std::is_same_v<ToNativeType, Int128>) {
- static constexpr __int128 min_int128 =
__int128(0x8000000000000000ll) << 64;
- static constexpr __int128 max_int128 =
- (__int128(0x7fffffffffffffffll) << 64) +
0xffffffffffffffffll;
- if (out <= static_cast<ToNativeType>(min_int128) ||
- out >= static_cast<ToNativeType>(max_int128)) {
- LOG(FATAL) << "Decimal convert overflow. Float is out of
Decimal range";
- }
- } else {
- if (out <= std::numeric_limits<ToNativeType>::min() ||
- out >= std::numeric_limits<ToNativeType>::max()) {
- LOG(FATAL) << "Decimal convert overflow. Float is out of
Decimal range";
- }
+ if (out <=
static_cast<FromFieldType>(std::numeric_limits<ToNativeType>::min()) ||
+ out >=
static_cast<FromFieldType>(std::numeric_limits<ToNativeType>::max())) {
+ LOG(FATAL) << "Decimal convert overflow. Float is out of Decimal
range";
Review comment:
No need to handle int128 separately? And why using FATAL log here?
##########
File path: be/src/exec/csv_scan_node.cpp
##########
@@ -556,7 +556,8 @@ bool CsvScanNode::split_check_fill(const std::string& line,
RuntimeState* state)
fields[i].length(), slot, state,
&error_msg);
if (flag == false) {
- LOG(INFO) << error_msg.str();;
+ LOG(INFO) << error_msg.str();
+ ;
Review comment:
remove
--
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: commits-unsubscr...@doris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@doris.apache.org
For additional commands, e-mail: commits-h...@doris.apache.org
