Rework parameter handling

andyfengHKU · andyfengHKU · commit 76bc25cf4661 · 2025-09-09T18:39:52.000+08:00
diff --git a/src/binder/bind_expression/bind_parameter_expression.cpp b/src/binder/bind_expression/bind_parameter_expression.cpp
@@ -1,6 +1,7 @@
 #include "binder/expression/parameter_expression.h"
 #include "binder/expression_binder.h"
 #include "parser/expression/parsed_parameter_expression.h"
+#include "common/exception/binder.h"
 
 using namespace kuzu::common;
 using namespace kuzu::parser;
@@ -12,14 +13,12 @@ std::shared_ptr<Expression> ExpressionBinder::bindParameterExpression(
     const ParsedExpression& parsedExpression) {
     auto& parsedParameterExpression = parsedExpression.constCast<ParsedParameterExpression>();
     auto parameterName = parsedParameterExpression.getParameterName();
-    parsedParameters.insert(parameterName);
-    if (parameterMap.contains(parameterName)) {
-        return make_shared<ParameterExpression>(parameterName, *parameterMap.at(parameterName));
-    } else {
-        auto value = std::make_shared<Value>(Value::createNullValue());
-        parameterMap.insert({parameterName, value});
-        return std::make_shared<ParameterExpression>(parameterName, *value);
+    if (knownParameters.contains(parameterName)) {
+        return make_shared<ParameterExpression>(parameterName, *knownParameters.at(parameterName));
     }
+    // LCOV_EXCL_START
+    throw BinderException(stringFormat("Cannot find parameter {}. This should not happen.", parameterName));
+    // LCOV_EXCL_STOP
 }
 
 } // namespace binder
diff --git a/src/binder/binder.cpp b/src/binder/binder.cpp
@@ -274,13 +274,5 @@ TableFunction Binder::getScanFunction(const FileTypeInfo& typeInfo,
     return *func->ptrCast<TableFunction>();
 }
 
-void Binder::validateAllInputParametersParsed() const {
-    for (const auto& [name, _] : expressionBinder.parameterMap) {
-        if (!expressionBinder.parsedParameters.contains(name)) {
-            throw Exception("Parameter " + name + " not found.");
-        }
-    }
-}
-
 } // namespace binder
 } // namespace kuzu
diff --git a/src/binder/expression_binder.cpp b/src/binder/expression_binder.cpp
@@ -35,10 +35,8 @@ std::shared_ptr<Expression> ExpressionBinder::bindExpression(
         bool allParamExist = true;
         for (auto& parsedExpr : collector.getParamExprs()) {
             auto name = parsedExpr->constCast<ParsedParameterExpression>().getParameterName();
-            if (!parameterMap.contains(name)) {
-                auto value = std::make_shared<Value>(Value::createNullValue());
-                parameterMap.insert({name, value});
-                parsedParameters.insert(name);
+            if (!knownParameters.contains(name)) {
+                unknownParameters.insert(name);
                 allParamExist = false;
             }
         }
@@ -147,5 +145,11 @@ std::string ExpressionBinder::getUniqueName(const std::string& name) const {
     return binder->getUniqueExpressionName(name);
 }
 
+void ExpressionBinder::addParameter(const std::string& name, std::shared_ptr<Value> value) {
+    KU_ASSERT(!knownParameters.contains(name));
+    knownParameters[name] = value;
+}
+
+
 } // namespace binder
 } // namespace kuzu
diff --git a/src/include/binder/binder.h b/src/include/binder/binder.h
@@ -75,15 +75,6 @@ class Binder {
 
     KUZU_API std::unique_ptr<BoundStatement> bind(const parser::Statement& statement);
 
-    void setInputParameters(
-        std::unordered_map<std::string, std::shared_ptr<common::Value>> parameters) {
-        expressionBinder.parameterMap = std::move(parameters);
-    }
-
-    std::unordered_map<std::string, std::shared_ptr<common::Value>> getParameterMap() {
-        return expressionBinder.parameterMap;
-    }
-
     KUZU_API std::shared_ptr<Expression> createVariable(const std::string& name,
         const common::LogicalType& dataType);
     KUZU_API std::shared_ptr<Expression> createInvisibleVariable(const std::string& name,
@@ -311,7 +302,6 @@ class Binder {
     KUZU_API static void validateColumnExistence(const catalog::TableCatalogEntry* entry,
         const std::string& columnName);
 
-    void validateAllInputParametersParsed() const;
     /*** helpers ***/
     std::string getUniqueExpressionName(const std::string& name);
 
diff --git a/src/include/binder/expression_binder.h b/src/include/binder/expression_binder.h
@@ -129,15 +129,24 @@ class ExpressionBinder {
     std::shared_ptr<Expression> forceCast(const std::shared_ptr<Expression>& expression,
         const common::LogicalType& targetType);
 
+    // Parameter
+    void addParameter(const std::string& name, std::shared_ptr<common::Value> value);
+    const std::unordered_set<std::string>& getUnknownParameters() const {
+        return unknownParameters;
+    }
+    const std::unordered_map<std::string, std::shared_ptr<common::Value>>& getKnownParameters() const {
+        return knownParameters;
+    }
+
     std::string getUniqueName(const std::string& name) const;
 
     const ExpressionBinderConfig& getConfig() { return config; }
 
 private:
     Binder* binder;
     main::ClientContext* context;
-    std::unordered_map<std::string, std::shared_ptr<common::Value>> parameterMap;
-    std::unordered_set<std::string> parsedParameters;
+    std::unordered_set<std::string> unknownParameters;
+    std::unordered_map<std::string, std::shared_ptr<common::Value>> knownParameters;
     ExpressionBinderConfig config;
 };
 
diff --git a/src/include/common/parameter_manager.h b/src/include/common/parameter_manager.h
@@ -0,0 +1,5 @@
+//#pragma once
+//
+//namespace common {
+//
+//}
diff --git a/src/include/main/client_context.h b/src/include/main/client_context.h
@@ -208,8 +208,7 @@ class KUZU_API ClientContext {
 
     PrepareResult prepareNoLock(std::shared_ptr<parser::Statement> parsedStatement,
         bool shouldCommitNewTransaction,
-        std::optional<std::unordered_map<std::string, std::shared_ptr<common::Value>>> inputParams =
-            std::nullopt);
+        std::unordered_map<std::string, std::shared_ptr<common::Value>> inputParams = {});
 
     template<typename T, typename... Args>
     std::unique_ptr<QueryResult> executeWithParams(PreparedStatement* preparedStatement,
diff --git a/src/include/main/prepared_statement.h b/src/include/main/prepared_statement.h
@@ -62,15 +62,18 @@ class PreparedStatement {
      */
     KUZU_API bool isReadOnly() const;
 
-    std::unordered_map<std::string, std::shared_ptr<common::Value>>& getParameterMapUnsafe() {
-        return parameterMap;
+    const std::unordered_set<std::string>& getUnknownParameters() const {
+        return unknownParameters;
     }
+    std::unordered_set<std::string> getKnownParameters();
+    void updateParameter(const std::string& name, common::Value* value);
+    void addParameter(const std::string& name, common::Value* value);
 
     std::string getName() const { return cachedPreparedStatementName; }
 
     common::StatementType getStatementType() const;
 
-    void validateExecuteParam(const std::string& paramName, common::Value* param) const;
+    void validateParam(const std::string& paramName, common::Value* param) const;
 
     static std::unique_ptr<PreparedStatement> getPreparedStatementWithError(
         const std::string& errorMessage);
@@ -81,6 +84,7 @@ class PreparedStatement {
     std::string errMsg;
     PreparedSummary preparedSummary;
     std::string cachedPreparedStatementName;
+    std::unordered_set<std::string> unknownParameters;
     std::unordered_map<std::string, std::shared_ptr<common::Value>> parameterMap;
 };
 
diff --git a/src/main/client_context.cpp b/src/main/client_context.cpp
@@ -323,17 +323,19 @@ std::unique_ptr<PreparedStatement> ClientContext::prepareWithParams(std::string_
 
 static void bindParametersNoLock(PreparedStatement& preparedStatement,
     const std::unordered_map<std::string, std::unique_ptr<Value>>& inputParams) {
-    auto& parameterMap = preparedStatement.getParameterMapUnsafe();
-    for (auto& [name, value] : inputParams) {
-        if (!parameterMap.contains(name)) {
-            throw Exception("Parameter " + name + " not found.");
+    for (auto& key : preparedStatement.getKnownParameters()) {
+        if (inputParams.contains(key)) {
+            // Found input. Update parameter map.
+            preparedStatement.updateParameter(key, inputParams.at(key).get());
         }
-        preparedStatement.validateExecuteParam(name, value.get());
-        // The much more natural `parameterMap.at(name) = std::move(v)` fails.
-        // The reason is that other parts of the code rely on the existing Value object to be
-        // modified in-place, not replaced in this map.
-        *parameterMap.at(name) = std::move(*value);
     }
+    for (auto& key : preparedStatement.getUnknownParameters()) {
+        if (!inputParams.contains(key)) {
+            throw Exception("Parameter " + key + " not found.");
+        }
+        preparedStatement.addParameter(key, inputParams.at(key).get());
+    }
+
 }
 
 std::unique_ptr<QueryResult> ClientContext::executeWithParams(PreparedStatement* preparedStatement,
@@ -469,7 +471,7 @@ void ClientContext::validateTransaction(bool readOnly, bool requireTransaction)
 
 ClientContext::PrepareResult ClientContext::prepareNoLock(
     std::shared_ptr<Statement> parsedStatement, bool shouldCommitNewTransaction,
-    std::optional<std::unordered_map<std::string, std::shared_ptr<Value>>> inputParams) {
+    std::unordered_map<std::string, std::shared_ptr<Value>> inputParams) {
     auto preparedStatement = std::make_unique<PreparedStatement>();
     auto cachedStatement = std::make_unique<CachedPreparedStatement>();
     cachedStatement->parsedStatement = parsedStatement;
@@ -489,12 +491,13 @@ ClientContext::PrepareResult ClientContext::prepareNoLock(
             *transactionContext,
             [&]() -> void {
                 auto binder = Binder(this, localDatabase->getBinderExtensions());
-                if (inputParams) {
-                    binder.setInputParameters(*inputParams);
+                auto expressionBinder = binder.getExpressionBinder();
+                for (auto& [name, value] : inputParams) {
+                    expressionBinder->addParameter(name, value);
                 }
                 const auto boundStatement = binder.bind(*parsedStatement);
-                binder.validateAllInputParametersParsed();
-                preparedStatement->parameterMap = binder.getParameterMap();
+                preparedStatement->unknownParameters = expressionBinder->getUnknownParameters();
+                preparedStatement->parameterMap = expressionBinder->getKnownParameters();
                 cachedStatement->columns = boundStatement->getStatementResult()->getColumns();
                 auto planner = Planner(this);
                 auto bestPlan = planner.planStatement(*boundStatement);
diff --git a/src/main/prepared_statement.cpp b/src/main/prepared_statement.cpp
@@ -45,8 +45,7 @@ StatementType PreparedStatement::getStatementType() const {
     return preparedSummary.statementType;
 }
 
-void PreparedStatement::validateExecuteParam(const std::string& paramName,
-    common::Value* param) const {
+void PreparedStatement::validateParam(const std::string& paramName, Value* param) const {
     if (param->getDataType().getLogicalTypeID() == LogicalTypeID::POINTER &&
         (!parameterMap.contains(paramName) ||
             parameterMap.at(paramName)->getValue<uint8_t*>() != param->getValue<uint8_t*>())) {
@@ -59,6 +58,23 @@ void PreparedStatement::validateExecuteParam(const std::string& paramName,
     }
 }
 
+std::unordered_set<std::string> PreparedStatement::getKnownParameters() {
+    std::unordered_set<std::string> result;
+    for (auto& [k, _] : parameterMap) {
+        result.insert(k);
+    }
+    return result;
+}
+
+void PreparedStatement::updateParameter(const std::string& name, Value* value) {
+    validateParam(name, value);
+    *parameterMap.at(name) = std::move(*value);
+}
+
+void PreparedStatement::addParameter(const std::string& name, Value* value) {
+    parameterMap.insert({name, std::make_shared<Value>(*value)});
+}
+
 PreparedStatement::~PreparedStatement() = default;
 
 std::unique_ptr<PreparedStatement> PreparedStatement::getPreparedStatementWithError(
diff --git a/test/api/api_test.cpp b/test/api/api_test.cpp
@@ -251,19 +251,22 @@ TEST_F(ApiTest, PrepareWithSkipLimitError) {
     auto prepared = conn->prepare("MATCH (p:person) RETURN p.ID skip $sp");
     auto result = conn->execute(prepared.get());
     ASSERT_FALSE(result->isSuccess());
-    ASSERT_EQ(result->toString(), "Runtime exception: Cannot evaluate $sp as a valid skip number.");
+    ASSERT_EQ(result->toString(), "Parameter sp not found.");
+
+    result = conn->execute(prepared.get(), std::make_pair(std::string("sp"), "abc"));
+    ASSERT_FALSE(result->isSuccess());
+    ASSERT_EQ(result->toString(),
+        "Runtime exception: The number of rows to skip/limit must be a non-negative integer.");
 
     prepared = conn->prepare("MATCH (p:person) RETURN p.ID limit $sp");
     result = conn->execute(prepared.get());
     ASSERT_FALSE(result->isSuccess());
-    ASSERT_EQ(result->toString(),
-        "Runtime exception: Cannot evaluate $sp as a valid limit number.");
+    ASSERT_EQ(result->toString(), "Parameter sp not found.");
 
     prepared = conn->prepare("MATCH (p:person) RETURN p.ID skip $s limit $sp");
     result = conn->execute(prepared.get(), std::make_pair(std::string("s"), 3));
     ASSERT_FALSE(result->isSuccess());
-    ASSERT_EQ(result->toString(),
-        "Runtime exception: Cannot evaluate $sp as a valid limit number.");
+    ASSERT_EQ(result->toString(), "Parameter sp not found.");
 
     prepared = conn->prepare("MATCH (p:person) RETURN p.ID skip $s");
     result = conn->execute(prepared.get(), std::make_pair(std::string("s"), 3.4));
@@ -316,8 +319,13 @@ TEST_F(ApiTest, MissingParam) {
     std::unordered_map<std::string, std::unique_ptr<Value>> params;
     params["val1"] = std::make_unique<Value>(Value::createValue(3));
     auto prep = conn->prepareWithParams("RETURN $val1 + $val2", std::move(params));
-    ASSERT_FALSE(prep->isSuccess());
-    ASSERT_STREQ("Parameter val1 not found.", prep->getErrorMessage().c_str());
+    ASSERT_TRUE(prep->isSuccess());
+    auto result = conn->execute(prep.get(), std::make_pair(std::string("s"), 3));
+    ASSERT_FALSE(result->isSuccess());
+    ASSERT_STREQ("Parameter val2 not found.", result->getErrorMessage().c_str());
+    result = conn->execute(prep.get(), std::make_pair(std::string("val2"), 1.1));
+    ASSERT_TRUE(result->isSuccess());
+    ASSERT_STREQ("4.100000\n", result->getNext()->toString().c_str());
 }
 
 TEST_F(ApiTest, CloseDatabaseBeforeQueryResultAndConnection) {
diff --git a/test/api/prepare_test.cpp b/test/api/prepare_test.cpp
@@ -204,7 +204,10 @@ TEST_F(ApiTest, ParamNotExist) {
         conn->prepare("MATCH (a:person) WHERE a.fName STARTS WITH $n RETURN COUNT(*)");
     auto result = conn->execute(preparedStatement.get(), std::make_pair(std::string("a"), "A"));
     ASSERT_FALSE(result->isSuccess());
-    ASSERT_STREQ("Parameter a not found.", result->getErrorMessage().c_str());
+    ASSERT_STREQ("Parameter n not found.", result->getErrorMessage().c_str());
+    result = conn->execute(preparedStatement.get(), std::make_pair(std::string("a"), "A"),  std::make_pair(std::string("n"), "A"));
+    ASSERT_TRUE(result->isSuccess());
+    ASSERT_STREQ("1\n", result->getNext()->toString().c_str());
 }
 
 TEST_F(ApiTest, ParamTypeError) {
diff --git a/tools/python_api/test/test_exception.py b/tools/python_api/test/test_exception.py
@@ -8,7 +8,7 @@
 def test_exception(conn_db_readonly: ConnDB) -> None:
     conn, _ = conn_db_readonly
 
-    with pytest.raises(RuntimeError, match=r"Parameter asd not found."):
+    with pytest.raises(RuntimeError, match=r"Parameter 1 not found."):
         conn.execute("MATCH (a:person) WHERE a.registerTime = $1 RETURN COUNT(*);", {"asd": 1})
 
     with pytest.raises(RuntimeError, match=r"Binder exception: Cannot find property dummy for a."):

-Original file line number
+Diff line change
@@ @@ -0,0 +1,5 @@ @@
 +//#pragma once
 +//
 +//namespace common {
 +//
 +//}