Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor nullOutput handing in CastExpr #4185

Closed
wants to merge 1 commit into from
Closed

Refactor nullOutput handing in CastExpr #4185

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 26 additions & 28 deletions velox/expression/CastExpr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,40 +39,34 @@ namespace {
/// @tparam From The expression type
/// @param row The index of the current row
/// @param input The input vector (of type From)
/// @param resultFlatVector The output vector (of type To)
/// @param result The output vector (of type To)
/// @return False if the result is null
template <typename To, typename From, bool Truncate>
void applyCastKernel(
vector_size_t row,
const BaseVector& input,
FlatVector<To>* resultFlatVector,
const SimpleVector<From>* input,
FlatVector<To>* result,
bool& nullOutput) {
auto* inputVector = input.asUnchecked<SimpleVector<From>>();

// Special handling for string target type
if constexpr (CppToType<To>::typeKind == TypeKind::VARCHAR) {
if (nullOutput) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is wrong. 'nullOutput' is always 'true' here, hence, it never caused a problem.

resultFlatVector->setNull(row, true);
} else {
auto output =
util::Converter<CppToType<To>::typeKind, void, Truncate>::cast(
inputVector->valueAt(row), nullOutput);
auto output =
util::Converter<CppToType<To>::typeKind, void, Truncate>::cast(
input->valueAt(row), nullOutput);
if (!nullOutput) {
// Write the result output to the output vector
auto writer = exec::StringWriter<>(resultFlatVector, row);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic should be conditioned on 'nullOutput' being false. It just happens so that Converter::cast never sets 'nullOutput' to true.

auto writer = exec::StringWriter<>(result, row);
writer.resize(output.size());
if (output.size()) {
std::memcpy(writer.data(), output.data(), output.size());
}
writer.finalize();
}
} else {
auto result =
auto output =
util::Converter<CppToType<To>::typeKind, void, Truncate>::cast(
inputVector->valueAt(row), nullOutput);
if (nullOutput) {
resultFlatVector->setNull(row, true);
} else {
resultFlatVector->set(row, result);
input->valueAt(row), nullOutput);
if (!nullOutput) {
result->set(row, output);
}
}
}
Expand Down Expand Up @@ -150,16 +144,15 @@ void CastExpr::applyCastWithTry(
const auto& queryConfig = context.execCtx()->queryCtx()->queryConfig();
auto isCastIntByTruncate = queryConfig.isCastIntByTruncate();

auto* inputSimpleVector = input.as<SimpleVector<From>>();

if (!isCastIntByTruncate) {
context.applyToSelectedNoThrow(rows, [&](int row) {
bool nullOutput = false;
try {
// Passing a false truncate flag
bool nullOutput = false;
applyCastKernel<To, From, false>(
row, input, resultFlatVector, nullOutput);
if (nullOutput) {
throw std::invalid_argument("");
}
row, inputSimpleVector, resultFlatVector, nullOutput);
} catch (const VeloxRuntimeError& re) {
VELOX_FAIL(
makeErrorMessage(input, row, resultFlatVector->type()) + " " +
Expand All @@ -173,17 +166,18 @@ void CastExpr::applyCastWithTry(
makeErrorMessage(input, row, resultFlatVector->type()) + " " +
e.what());
}

if (nullOutput) {
VELOX_USER_FAIL(makeErrorMessage(input, row, resultFlatVector->type()));
}
});
} else {
context.applyToSelectedNoThrow(rows, [&](int row) {
bool nullOutput = false;
try {
// Passing a true truncate flag
bool nullOutput = false;
applyCastKernel<To, From, true>(
row, input, resultFlatVector, nullOutput);
if (nullOutput) {
throw std::invalid_argument("");
}
row, inputSimpleVector, resultFlatVector, nullOutput);
} catch (const VeloxRuntimeError& re) {
VELOX_FAIL(
makeErrorMessage(input, row, resultFlatVector->type()) + " " +
Expand All @@ -197,6 +191,10 @@ void CastExpr::applyCastWithTry(
makeErrorMessage(input, row, resultFlatVector->type()) + " " +
e.what());
}

if (nullOutput) {
VELOX_USER_FAIL(makeErrorMessage(input, row, resultFlatVector->type()));
}
});
}

Expand Down