diff --git a/src/carnot/planner/compiler/test_utils.h b/src/carnot/planner/compiler/test_utils.h index 2f4f7559a16..2f65f616b50 100644 --- a/src/carnot/planner/compiler/test_utils.h +++ b/src/carnot/planner/compiler/test_utils.h @@ -353,13 +353,12 @@ struct CompilerErrorMatcher { (*listener) << "Status is ok, no compiler error found."; return false; } - if (!status.has_context() && !status.msg().empty()) { - (*listener) << "Status does not have a context."; + if (status.msg().empty()) { + (*listener) << "Status does not have a message."; return false; } - if (!status.has_context() && status.msg().empty()) { - (*listener) << absl::Substitute("Status does not have a context, but has a message: '$0'", - status.msg()); + if (!status.has_context()) { + (*listener) << "Status does not have a context."; return false; } if (!status.context()->Is()) { diff --git a/src/carnot/planner/ir/ast_utils.h b/src/carnot/planner/ir/ast_utils.h index 714fc02540c..2fb4fdd6b7d 100644 --- a/src/carnot/planner/ir/ast_utils.h +++ b/src/carnot/planner/ir/ast_utils.h @@ -48,9 +48,9 @@ namespace planner { */ template Status CreateAstError(const pypa::AstPtr& ast, Args... args) { - compilerpb::CompilerErrorGroup context = - LineColErrorPb(ast->line, ast->column, absl::Substitute(args...)); - return Status(statuspb::INVALID_ARGUMENT, "", + auto msg = absl::Substitute(args...); + compilerpb::CompilerErrorGroup context = LineColErrorPb(ast->line, ast->column, msg); + return Status(statuspb::INVALID_ARGUMENT, msg, std::make_unique(context)); } @@ -116,9 +116,10 @@ Status AddOuterContextToError(Status status, const pypa::AstPtr& ast, Args... ar } compilerpb::CompilerErrorGroup error_group; CHECK(status.context()->UnpackTo(&error_group)); - AddLineColError(&error_group, ast->line, ast->column, absl::Substitute(args...)); + auto msg = absl::Substitute(args...); + AddLineColError(&error_group, ast->line, ast->column, msg); - return Status(statuspb::INVALID_ARGUMENT, "", + return Status(statuspb::INVALID_ARGUMENT, msg, std::make_unique(error_group)); } diff --git a/src/carnot/planner/ir/ast_utils_test.cc b/src/carnot/planner/ir/ast_utils_test.cc index 10b6dac00e6..70e025c2e71 100644 --- a/src/carnot/planner/ir/ast_utils_test.cc +++ b/src/carnot/planner/ir/ast_utils_test.cc @@ -68,6 +68,17 @@ INSTANTIATE_TEST_SUITE_P(AstToStringTestSuite, AstToStringTest, return info.param.name; }); +class CreateAstErrorTest : public ::testing::Test {}; + +TEST(CreateAstErrorTest, CreateAstErrorBasic) { + Parser parser; + ASSERT_OK_AND_ASSIGN(auto ast, parser.Parse("5 + 2", /* parse_doc_strings */ false)); + auto error_msg = "This is a test"; + auto ast_error = CreateAstError(ast, error_msg); + ASSERT_NOT_OK(ast_error); + ASSERT_EQ(ast_error.msg(), error_msg); +} + } // namespace planner } // namespace carnot } // namespace px diff --git a/src/carnot/planner/ir/ir_node.h b/src/carnot/planner/ir/ir_node.h index a05eeb922e7..1748d85ca28 100644 --- a/src/carnot/planner/ir/ir_node.h +++ b/src/carnot/planner/ir/ir_node.h @@ -125,9 +125,9 @@ class IRNode { */ template Status CreateIRNodeError(Args... args) const { - compilerpb::CompilerErrorGroup context = - LineColErrorPb(line(), col(), absl::Substitute(args...)); - return Status(statuspb::INVALID_ARGUMENT, "", + auto msg = absl::Substitute(args...); + compilerpb::CompilerErrorGroup context = LineColErrorPb(line(), col(), msg); + return Status(statuspb::INVALID_ARGUMENT, msg, std::make_unique(context)); } /** diff --git a/src/carnot/planner/parser/parser.cc b/src/carnot/planner/parser/parser.cc index b67334b34e0..3c0a5fc83b6 100644 --- a/src/carnot/planner/parser/parser.cc +++ b/src/carnot/planner/parser/parser.cc @@ -52,12 +52,14 @@ class PypaErrorHandler { */ Status ProcessErrors() { compilerpb::CompilerErrorGroup error_group; + std::string msg = ""; for (const auto& err : errs_) { compilerpb::CompilerError* err_pb = error_group.add_errors(); compilerpb::LineColError* lc_err_pb = err_pb->mutable_line_col_error(); CreateLineColError(lc_err_pb, err); + absl::StrAppend(&msg, err.message); } - return Status(statuspb::INVALID_ARGUMENT, "", + return Status(statuspb::INVALID_ARGUMENT, msg, std::make_unique(error_group)); } diff --git a/src/experimental/standalone_pem/vizier_server.h b/src/experimental/standalone_pem/vizier_server.h index 514ee57f7a9..ce071bf379c 100644 --- a/src/experimental/standalone_pem/vizier_server.h +++ b/src/experimental/standalone_pem/vizier_server.h @@ -126,7 +126,10 @@ class VizierServer final : public api::vizierpb::VizierService::Service { auto s_or_plan = px::carnot::planner::compiler::Compiler().Compile(reader->query_str(), compiler_state.get()); if (!s_or_plan.ok()) { - return ::grpc::Status(grpc::StatusCode::INVALID_ARGUMENT, "Failed to compile script"); + auto error_msg = + absl::Substitute("Failed to compile script with error='$0'", s_or_plan.msg()); + LOG(WARNING) << error_msg; + return ::grpc::Status(grpc::StatusCode::INVALID_ARGUMENT, error_msg); } auto plan = s_or_plan.ConsumeValueOrDie(); for (auto f : plan.nodes()) {