Skip to content

Commit

Permalink
Check window function signature exists (#7188)
Browse files Browse the repository at this point in the history 
Summary:
Check window function signature exists before creating window function and
produce clear error message if signature doesn't exist.

Avoid failures with no context like this one:

```
VeloxRuntimeError:
	at Unknown.# 0  facebook::velox::VeloxException::VeloxException(char const*, unsigned long, char const*, std::basic_string_view<char, std::char_traits<char> >, std::basic_string_view<char, std::char_traits<char> >, std::basic_string_view<char, std::char_traits<char> >, std::basic_string_view<char, std::char_traits<char> >, bool, facebook::velox::VeloxException::Type, std::basic_string_view<char, std::char_traits<char> >)(Unknown Source)
	at Unknown.# 1  void facebook::velox::detail::veloxCheckFail<facebook::velox::VeloxRuntimeError, facebook::velox::detail::CompileTimeEmptyString>(facebook::velox::detail::VeloxCheckFailArgs const&, facebook::velox::detail::CompileTimeEmptyString)(Unknown Source)
	at Unknown.# 2  std::_Function_handler<std::unique_ptr<facebook::velox::exec::Aggregate, std::default_delete<facebook::velox::exec::Aggregate> > (facebook::velox::core::AggregationNode::Step, std::vector<std::shared_ptr<facebook::velox::Type const>, std::allocator<std::shared_ptr<facebook::velox::Type const> > > const&, std::shared_ptr<facebook::velox::Type const> const&, facebook::velox::core::QueryConfig const&), facebook::velox::aggregate::prestosql::(anonymous namespace)::registerMapUnionSum(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)::$_0>::_M_invoke(std::_Any_data const&, facebook::velox::core::AggregationNode::Step&&, std::vector<std::shared_ptr<facebook::velox::Type const>, std::allocator<std::shared_ptr<facebook::velox::Type const> > > const&, std::shared_ptr<facebook::velox::Type const> const&, facebook::velox::core::QueryConfig const&)(Unknown Source)
	at Unknown.# 3  facebook::velox::exec::Aggregate::create(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, facebook::velox::core::AggregationNode::Step, std::vector<std::shared_ptr<facebook::velox::Type const>, std::allocator<std::shared_ptr<facebook::velox::Type const> > > const&, std::shared_ptr<facebook::velox::Type const> const&, facebook::velox::core::QueryConfig const&)(Unknown Source)
	at Unknown.# 4  std::_Function_handler<std::unique_ptr<facebook::velox::exec::WindowFunction, std::default_delete<facebook::velox::exec::WindowFunction> > (std::vector<facebook::velox::exec::WindowFunctionArg, std::allocator<facebook::velox::exec::WindowFunctionArg> > const&, std::shared_ptr<facebook::velox::Type const> const&, bool, facebook::velox::memory::MemoryPool*, facebook::velox::HashStringAllocator*, facebook::velox::core::QueryConfig const&), facebook::velox::exec::registerAggregateWindowFunction(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)::$_0>::_M_invoke(std::_Any_data const&, std::vector<facebook::velox::exec::WindowFunctionArg, std::allocator<facebook::velox::exec::WindowFunctionArg> > const&, std::shared_ptr<facebook::velox::Type const> const&, bool&&, facebook::velox::memory::MemoryPool*&&, facebook::velox::HashStringAllocator*&&, facebook::velox::core::QueryConfig const&)(Unknown Source)
	at Unknown.# 5  facebook::velox::exec::WindowFunction::create(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::vector<facebook::velox::exec::WindowFunctionArg, std::allocator<facebook::velox::exec::WindowFunctionArg> > const&, std::shared_ptr<facebook::velox::Type const> const&, bool, facebook::velox::memory::MemoryPool*, facebook::velox::HashStringAllocator*, facebook::velox::core::QueryConfig const&)(Unknown Source)
	at Unknown.# 6  facebook::velox::exec::Window::createWindowFunctions()(Unknown Source)
	at Unknown.# 7  facebook::velox::exec::Window::initialize()(Unknown Source)
```

Pull Request resolved: #7188

Reviewed By: pedroerp

Differential Revision: D50557644

Pulled By: mbasmanova

fbshipit-source-id: 10fe13d3c4f16cc07334d605f659b9077c2cbefb
  • Loading branch information
@mbasmanova @facebook-github-bot
mbasmanova authored and facebook-github-bot committed Oct 23, 2023
1 parent 132bcdf commit b85f6d3
Show file tree
Hide file tree
Showing 2 changed files with 90 additions and 4 deletions.
29 changes: 27 additions & 2 deletions velox/exec/WindowFunction.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

#include "velox/exec/WindowFunction.h"
#include "velox/expression/FunctionSignature.h"
#include "velox/expression/SignatureBinder.h"

namespace facebook::velox::exec {

Expand Down Expand Up @@ -66,8 +67,32 @@ std::unique_ptr<WindowFunction> WindowFunction::create(
const core::QueryConfig& config) {
// Lookup the function in the new registry first.
if (auto func = getWindowFunctionEntry(name)) {
return func.value()->factory(
args, resultType, ignoreNulls, pool, stringAllocator, config);
std::vector<TypePtr> argTypes;
argTypes.reserve(args.size());
for (const auto& arg : args) {
argTypes.push_back(arg.type);
}

const auto& signatures = func.value()->signatures;
for (auto& signature : signatures) {
SignatureBinder binder(*signature, argTypes);
if (binder.tryBind()) {
auto type = binder.tryResolveType(signature->returnType());
VELOX_USER_CHECK(
type->equivalent(*resultType),
"Unexpected return type for window function {}. Expected {}. Got {}.",
toString(name, argTypes),
type->toString(),
resultType->toString())
return func.value()->factory(
args, resultType, ignoreNulls, pool, stringAllocator, config);
}
}

VELOX_USER_FAIL(
"Window function signature is not supported: {}. Supported signatures: {}.",
toString(name, argTypes),
toString(signatures));
}

VELOX_USER_FAIL("Window function not registered: {}", name);
Expand Down
65 changes: 63 additions & 2 deletions velox/functions/prestosql/window/tests/SimpleAggregatesTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
* limitations under the License.
*/
#include "velox/common/base/tests/GTestUtils.h"
#include "velox/exec/tests/utils/PlanBuilder.h"
#include "velox/functions/lib/window/tests/WindowTestBase.h"
#include "velox/functions/prestosql/window/WindowFunctionsRegistration.h"

Expand Down Expand Up @@ -112,10 +113,10 @@ VELOX_INSTANTIATE_TEST_SUITE_P(
SimpleAggregatesTest,
testing::ValuesIn(getAggregateTestParams()));

class StringAggregatesTest : public WindowTestBase {};
class WindowTest : public WindowTestBase {};

// Test for an aggregate function with strings that needs out of line storage.
TEST_F(StringAggregatesTest, nonFixedWidthAggregate) {
TEST_F(WindowTest, variableWidthAggregate) {
auto size = 10;
auto input = {makeRowVector({
makeRandomInputVector(BIGINT(), size, 0.2),
Expand All @@ -128,6 +129,66 @@ TEST_F(StringAggregatesTest, nonFixedWidthAggregate) {
testWindowFunction(input, "max(c2)", kOverClauses);
}

TEST_F(WindowTest, missingFunctionSignature) {
auto input = {makeRowVector({
makeFlatVector<int64_t>({1, 2, 3}),
makeFlatVector<std::string>({"A", "B", "C"}),
makeFlatVector<int64_t>({10, 20, 30}),
})};

auto runWindow = [&](const core::CallTypedExprPtr& callExpr) {
core::WindowNode::Frame frame{
core::WindowNode::WindowType::kRows,
core::WindowNode::BoundType::kUnboundedPreceding,
nullptr,
core::WindowNode::BoundType::kUnboundedFollowing,
nullptr};

core::WindowNode::Function windowFunction{callExpr, frame, false};

CursorParameters params;
params.planNode =
PlanBuilder()
.values(input)
.addNode([&](auto nodeId, auto source) -> core::PlanNodePtr {
return std::make_shared<core::WindowNode>(
nodeId,
std::vector<core::FieldAccessTypedExprPtr>{
std::make_shared<core::FieldAccessTypedExpr>(
BIGINT(), "c0")},
std::vector<core::FieldAccessTypedExprPtr>{}, // sortingKeys
std::vector<core::SortOrder>{}, // sortingOrders
std::vector<std::string>{"w"},
std::vector<core::WindowNode::Function>{windowFunction},
false,
source);
})
.planNode();

readCursor(params, [](auto*) {});
};

auto callExpr = std::make_shared<core::CallTypedExpr>(
BIGINT(),
std::vector<core::TypedExprPtr>{
std::make_shared<core::FieldAccessTypedExpr>(VARCHAR(), "c1")},
"sum");

VELOX_ASSERT_THROW(
runWindow(callExpr),
"Window function signature is not supported: sum(VARCHAR). Supported signatures:");

callExpr = std::make_shared<core::CallTypedExpr>(
VARCHAR(),
std::vector<core::TypedExprPtr>{
std::make_shared<core::FieldAccessTypedExpr>(BIGINT(), "c2")},
"sum");

VELOX_ASSERT_THROW(
runWindow(callExpr),
"Unexpected return type for window function sum(BIGINT). Expected BIGINT. Got VARCHAR.");
}

class AggregateEmptyFramesTest : public WindowTestBase {};

// Test for aggregates that return NULL as the default value for empty frames
Expand Down

0 comments on commit b85f6d3

Please sign in to comment.