feat: improve request mode in RAW SQL

cases/plan/error_query.yaml

@@ -78,3 +78,12 @@ cases:
       success: false
       msg: |
         SHOW JOB statement require a target name following
+  - id: 10
+    mode: batch-unsupport
+    sql: |
+      SELECT * from t1
+      CONFIG (execute_mode = 'request', values = [12])
+    expect:
+      success: false
+      msg: |
+        FAILED_PRECONDITION: element type of the request values must be struct constructed with parentheses, but got [12]

cases/query/fail_query.yaml

@@ -70,3 +70,35 @@ cases:
     expect:
       success: false
       msg: unsupport join type RightJoin
+  - id: 4
+    mode: batch-unsupport
+    inputs:
+      - name: t1
+        columns: ["c1 string","c2 int","c4 timestamp"]
+        indexs: ["index1:c1:c4"]
+        rows:
+          - ["aa",20,1000]
+          - ["bb",30,1000]
+    sql: |
+      SELECT * from t1
+      CONFIG (execute_mode = 'request', values = [])
+    expect:
+      success: false
+      msg: |
+        FAILED_PRECONDITION: element number of the request values must not empty
+  - id: 5
+    mode: batch-unsupport
+    inputs:
+      - name: t1
+        columns: ["c1 string","c2 int","c4 timestamp"]
+        indexs: ["index1:c1:c4"]
+        rows:
+          - ["aa",20,1000]
+          - ["bb",30,1000]
+    sql: |
+      SELECT * from t1
+      CONFIG (execute_mode = 'request', values = ("c1"))
+    expect:
+      success: false
+      msg: |
+        INTERNAL: pass in expr number do not match, expect 3 but got 1: (c1)

hybridse/examples/toydb/src/cmd/toydb_run_engine.cc

@@ -91,7 +91,7 @@ int RunSingle(const std::string& yaml_path) {
         } else {
             default_mode = kBatchRequestMode;
         }
-        auto mode = Engine::TryDetermineMode(sql_case.sql_str_, default_mode);
+        auto mode = Engine::TryDetermineEngineMode(sql_case.sql_str_, default_mode);
         int ret = DoRunEngine(sql_case, options, mode);
         if (ret != ENGINE_TEST_RET_SUCCESS) {
             return ret;

hybridse/examples/toydb/src/testing/toydb_engine_test_base.cc

@@ -194,7 +194,7 @@ void BatchRequestEngineCheck(const SqlCase& sql_case,
 
 void EngineCheck(const SqlCase& sql_case, const EngineOptions& options,
                  EngineMode engine_mode) {
-    engine_mode = hybridse::vm::Engine::TryDetermineMode(sql_case.sql_str(), engine_mode);
+    engine_mode = hybridse::vm::Engine::TryDetermineEngineMode(sql_case.sql_str(), engine_mode);
     if (engine_mode == kBatchMode) {
         ToydbBatchEngineTestRunner engine_test(sql_case, options);
         engine_test.RunCheck();

hybridse/include/vm/engine.h

@@ -20,9 +20,10 @@
 #include <memory>
 #include <set>
 #include <string>
+#include <unordered_map>
 #include <utility>
 #include <vector>
-#include <unordered_map>
+
 #include "base/spin_lock.h"
 #include "codec/fe_row_codec.h"
 #include "vm/catalog.h"
@@ -36,6 +37,7 @@ using ::hybridse::codec::Row;
 
 inline constexpr const char* LONG_WINDOWS = "long_windows";
 
+class SqlContext;
 class Engine;
 /// \brief An options class for controlling engine behaviour.
 class EngineOptions {
@@ -178,13 +180,6 @@ class RunSession {
 
     void SetIndexHintsHandler(std::shared_ptr<IndexHintHandler> handler) { index_hints_ = handler; }
 
-    /// extract request rows info in SQL.
-    /// A SQL e.g 'SELECT ... FROM t1 options (execute_mode = "request", values = ...)'
-    /// request row info exists in 'values' option, as a format of:
-    /// 1. [(col1_expr, col2_expr, ... ), (...), ...]
-    /// 2. (col1_expr, col2_expr, ... )
-    absl::Status ExtractRequestRowsInSQL(std::vector<Row> * rows) const;
-
  protected:
     std::shared_ptr<hybridse::vm::CompileInfo> compile_info_;
     hybridse::vm::EngineMode engine_mode_;
@@ -370,8 +365,8 @@ class Engine {
     static void InitializeUnsafeRowOptFlag(bool isUnsafeRowOpt);
 
     /// determine engine mode for `sql`, `sql` may contains option defining
-    /// execute_mode, `default_mode` used if not.
-    static EngineMode TryDetermineMode(absl::string_view sql, EngineMode default_mode);
+    /// execute_mode, `default_mode` used if not or error.
+    static EngineMode TryDetermineEngineMode(absl::string_view sql, EngineMode default_mode);
 
     /// \brief Compile sql in db and stored the results in the session
     bool Get(const std::string& sql, const std::string& db,
@@ -429,6 +424,13 @@ class Engine {
     EngineOptions GetEngineOptions();
 
  private:
+    /// extract request rows info in SQL.
+    /// A SQL e.g 'SELECT ... FROM t1 options (execute_mode = "request", values = ...)'
+    /// request row info exists in 'values' option, as a format of:
+    /// 1. [(col1_expr, col2_expr, ... ), (...), ...]
+    /// 2. (col1_expr, col2_expr, ... )
+    static absl::Status ExtractRequestRowsInSQL(SqlContext* ctx);
+
     std::shared_ptr<CompileInfo> GetCacheLocked(const std::string& db,
                                                 const std::string& sql,
                                                 EngineMode engine_mode);

hybridse/include/vm/sql_ctx.h

@@ -20,6 +20,7 @@
 #include <memory>
 #include <string>
 #include <unordered_map>
+#include <vector>
 
 #include "node/node_manager.h"
 #include "vm/engine_context.h"
@@ -79,6 +80,9 @@ struct SqlContext {
     // 1. Array [ StructCtorWithParens ]
     // 2. StructCtorWithParens
     const node::ExprNode* request_expressions = nullptr;
+    // compiled request rows from SQL CONFIG clause, `values` option
+    // request_rows get fullfilled if engine mode is kRequestMode or kBatchRequestMode
+    std::vector<codec::Row> request_rows;
 
     ::hybridse::vm::BatchRequestInfo batch_request_info;
 

hybridse/src/planv2/plan_api.cc

@@ -26,6 +26,8 @@
 namespace hybridse {
 namespace plan {
 
+static absl::Status VerifyRequestExpression(vm::EngineMode mode, const node::ExprNode *expr) ABSL_ATTRIBUTE_NONNULL();
+
 base::Status PlanAPI::CreatePlanTreeFromScript(vm::SqlContext *ctx) {
     zetasql::ParserOptions parser_opts;
     zetasql::LanguageOptions language_opts;
@@ -59,6 +61,11 @@ base::Status PlanAPI::CreatePlanTreeFromScript(vm::SqlContext *ctx) {
         }
     }
 
+    if (ctx->request_expressions != nullptr) {
+        auto s = VerifyRequestExpression(ctx->engine_mode, ctx->request_expressions);
+        CHECK_TRUE(s.ok(), common::kTypeError, s.ToString());
+    }
+
     return {};
 }
 
@@ -141,6 +148,24 @@ absl::StatusOr<codec::Schema> ParseTableColumSchema(absl::string_view str) {
 
     return create->GetColumnDefListAsSchema();
 }
+absl::Status VerifyRequestExpression(vm::EngineMode mode, const node::ExprNode *expr) {
+    if (mode == vm::kRequestMode || mode == vm::kBatchRequestMode) {
+        if (expr->GetExprType() == node::kExprArray) {
+            auto arr = expr->GetAsOrNull<node::ArrayExpr>();
+            for (auto ele : arr->children_) {
+                if (ele->GetExprType() != node::kExprStructCtorParens) {
+                    return absl::FailedPreconditionError(absl::Substitute(
+                        "element type of the request values must be struct constructed with parentheses, but got $0",
+                        expr->GetExprString()));
+                }
+            }
+        }
+        // else won't check
+        // without request table schema, it is not able to verify expression like
+        // '(expr1)' since AST parser simplify it to 'expr1'
+    }
+    return absl::OkStatus();
+}
 
 }  // namespace plan
 }  // namespace hybridse

hybridse/src/planv2/planner_v2_test.cc

@@ -91,15 +91,14 @@ INSTANTIATE_TEST_SUITE_P(SQLCmdParserTest, PlannerV2Test,
 TEST_P(PlannerV2Test, PlannerSucessTest) {
     const auto& param = GetParam();
     std::string sqlstr = param.sql_str();
-    std::cout << sqlstr << std::endl;
-    base::Status status;
-    node::PlanNodeList plan_trees;
-    EXPECT_EQ(param.expect().success_, PlanAPI::CreatePlanTreeFromScript(sqlstr, plan_trees, manager_, status))
-        << status;
+    vm::SqlContext ctx;
+    ctx.sql = sqlstr;
+    auto status = PlanAPI::CreatePlanTreeFromScript(&ctx);
+    EXPECT_EQ(param.expect().success_, status.isOK()) << status;
     if (param.expect().success_) {
         if (!param.expect().plan_tree_str_.empty()) {
             // HACK: weak implementation, but usually it works
-            EXPECT_EQ(param.expect().plan_tree_str_, plan_trees.at(0)->GetTreeString());
+            EXPECT_EQ(param.expect().plan_tree_str_, ctx.logical_plan.at(0)->GetTreeString());
         }
     } else {
         if (!param.expect().msg_.empty()) {
@@ -117,10 +116,13 @@ TEST_P(PlannerV2Test, PlannerClusterOnlineServingOptTest) {
         LOG(INFO) << "Skip mode " << sql_case.mode();
         return;
     }
-    base::Status status;
-    node::PlanNodeList plan_trees;
+    vm::SqlContext ctx;
+    ctx.sql = sqlstr;
+    ctx.engine_mode = vm::kRequestMode;
+    ctx.is_cluster_optimized = true;
+    auto status = PlanAPI::CreatePlanTreeFromScript(&ctx);
     // TODO(ace): many tests defined in 'cases/plan/' do not pass, should annotated in yaml file
-    plan::PlanAPI::CreatePlanTreeFromScript(sqlstr, plan_trees, manager_, status, false, true);
+    plan::PlanAPI::CreatePlanTreeFromScript(&ctx);
 }
 
 TEST_F(PlannerV2Test, SimplePlannerCreatePlanTest) {
@@ -1880,9 +1882,11 @@ TEST_P(PlannerV2ErrorTest, RequestModePlanErrorTest) {
         LOG(INFO) << "Skip mode " << sql_case.mode() << " for request mode error test";
         return;
     }
-    base::Status status;
-    node::PlanNodeList plan_trees;
-    EXPECT_FALSE(plan::PlanAPI::CreatePlanTreeFromScript(sqlstr, plan_trees, manager_, status, false, false)) << status;
+    vm::SqlContext ctx;
+    ctx.sql = sqlstr;
+    ctx.engine_mode = vm::kRequestMode;
+    auto status = plan::PlanAPI::CreatePlanTreeFromScript(&ctx);
+    EXPECT_FALSE(status.isOK()) << status;
     if (!sql_case.expect_.msg_.empty()) {
         EXPECT_EQ(absl::StripAsciiWhitespace(sql_case.expect_.msg_), status.msg);
     }
@@ -1896,9 +1900,12 @@ TEST_P(PlannerV2ErrorTest, ClusterRequestModePlanErrorTest) {
         LOG(INFO) << "Skip mode " << sql_case.mode() << " for cluster request mode error test";
         return;
     }
-    base::Status status;
-    node::PlanNodeList plan_trees;
-    EXPECT_FALSE(plan::PlanAPI::CreatePlanTreeFromScript(sqlstr, plan_trees, manager_, status, false, true)) << status;
+    vm::SqlContext ctx;
+    ctx.sql = sqlstr;
+    ctx.engine_mode = vm::kRequestMode;
+    ctx.is_cluster_optimized = true;
+    auto status = plan::PlanAPI::CreatePlanTreeFromScript(&ctx);
+    EXPECT_FALSE(status.isOK()) << status;
     if (!sql_case.expect_.msg_.empty()) {
         EXPECT_EQ(absl::StripAsciiWhitespace(sql_case.expect_.msg_), status.msg);
     }
@@ -1911,9 +1918,11 @@ TEST_P(PlannerV2ErrorTest, BatchModePlanErrorTest) {
         LOG(INFO) << "Skip mode " << sql_case.mode() << " for batch mode error test";
         return;
     }
-    base::Status status;
-    node::PlanNodeList plan_trees;
-    EXPECT_FALSE(plan::PlanAPI::CreatePlanTreeFromScript(sqlstr, plan_trees, manager_, status, true)) << status;
+    vm::SqlContext ctx;
+    ctx.sql = sqlstr;
+    ctx.engine_mode = vm::kBatchMode;
+    auto status = plan::PlanAPI::CreatePlanTreeFromScript(&ctx);
+    EXPECT_FALSE(status.isOK()) << status;
     if (!sql_case.expect_.msg_.empty()) {
         EXPECT_EQ(absl::StripAsciiWhitespace(sql_case.expect_.msg_), status.msg);
     }

hybridse/src/testing/engine_test_base.cc

@@ -18,6 +18,7 @@
 #include <utility>
 
 #include "absl/cleanup/cleanup.h"
+#include "absl/strings/ascii.h"
 #include "absl/time/clock.h"
 #include "base/texttable.h"
 #include "google/protobuf/util/message_differencer.h"
@@ -419,7 +420,7 @@ Status EngineTestRunner::Compile() {
     if (!ok || !status.isOK()) {
         DLOG(INFO) << status;
         if (!sql_case_.expect().msg_.empty()) {
-            EXPECT_EQ(sql_case_.expect().msg_, status.msg);
+            EXPECT_EQ(absl::StripAsciiWhitespace(sql_case_.expect().msg_), status.msg);
         }
         return_code_ = ENGINE_TEST_RET_COMPILE_ERROR;
     } else {