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.

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

Prefer string_view in the API over std::string #2553

Merged
merged 1 commit into from
Jan 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
18 changes: 9 additions & 9 deletions src/include/function/udf_function.h
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ struct UDF {
}

template<typename TR, typename... Args>
static function_set getFunction(const std::string& name, TR (*udfFunc)(Args...),
static function_set getFunction(std::string name, TR (*udfFunc)(Args...),
std::vector<common::LogicalTypeID> parameterTypes, common::LogicalTypeID returnType) {
function_set definitions;
if (returnType == common::LogicalTypeID::STRING) {
Expand All @@ -226,29 +226,29 @@ struct UDF {
validateType<TR>(returnType);
scalar_exec_func scalarExecFunc = getScalarExecFunc<TR, Args...>(udfFunc, parameterTypes);
definitions.push_back(std::make_unique<function::ScalarFunction>(
name, std::move(parameterTypes), returnType, std::move(scalarExecFunc)));
std::move(name), std::move(parameterTypes), returnType, std::move(scalarExecFunc)));
return definitions;
}

template<typename TR, typename... Args>
static function_set getFunction(const std::string& name, TR (*udfFunc)(Args...)) {
static function_set getFunction(std::string name, TR (*udfFunc)(Args...)) {
return getFunction<TR, Args...>(
name, udfFunc, getParameterTypes<Args...>(), getParameterType<TR>());
std::move(name), udfFunc, getParameterTypes<Args...>(), getParameterType<TR>());
}

template<typename TR, typename... Args>
static function_set getVectorizedFunction(const std::string& name, scalar_exec_func execFunc) {
static function_set getVectorizedFunction(std::string name, scalar_exec_func execFunc) {
function_set definitions;
definitions.push_back(std::make_unique<function::ScalarFunction>(
name, getParameterTypes<Args...>(), getParameterType<TR>(), std::move(execFunc)));
definitions.push_back(std::make_unique<function::ScalarFunction>(std::move(name),
getParameterTypes<Args...>(), getParameterType<TR>(), std::move(execFunc)));
return definitions;
}

static function_set getVectorizedFunction(const std::string& name, scalar_exec_func execFunc,
static function_set getVectorizedFunction(std::string name, scalar_exec_func execFunc,
std::vector<common::LogicalTypeID> parameterTypes, common::LogicalTypeID returnType) {
function_set definitions;
definitions.push_back(std::make_unique<function::ScalarFunction>(
name, std::move(parameterTypes), returnType, std::move(execFunc)));
std::move(name), std::move(parameterTypes), returnType, std::move(execFunc)));
return definitions;
}
};
Expand Down
44 changes: 25 additions & 19 deletions src/include/main/connection.h
Original file line number Diff line number Diff line change
Expand Up @@ -65,13 +65,13 @@ class Connection {
* @param query The query to execute.
* @return the result of the query.
*/
KUZU_API std::unique_ptr<QueryResult> query(const std::string& query);
KUZU_API std::unique_ptr<QueryResult> query(std::string_view query);
/**
* @brief Prepares the given query and returns the prepared statement.
* @param query The query to prepare.
* @return the prepared statement.
*/
KUZU_API std::unique_ptr<PreparedStatement> prepare(const std::string& query);
KUZU_API std::unique_ptr<PreparedStatement> prepare(std::string_view query);
/**
* @brief Executes the given prepared statement with args and returns the result.
* @param preparedStatement The prepared statement to execute.
Expand Down Expand Up @@ -112,42 +112,48 @@ class Connection {
KUZU_API uint64_t getQueryTimeOut();

template<typename TR, typename... Args>
void createScalarFunction(const std::string& name, TR (*udfFunc)(Args...)) {
addScalarFunction(name, function::UDF::getFunction<TR, Args...>(name, udfFunc));
void createScalarFunction(std::string name, TR (*udfFunc)(Args...)) {
auto nameCopy = std::string(name);
addScalarFunction(
std::move(nameCopy), function::UDF::getFunction<TR, Args...>(std::move(name), udfFunc));
}

template<typename TR, typename... Args>
void createScalarFunction(const std::string& name,
std::vector<common::LogicalTypeID> parameterTypes, common::LogicalTypeID returnType,
TR (*udfFunc)(Args...)) {
addScalarFunction(name, function::UDF::getFunction<TR, Args...>(
name, udfFunc, std::move(parameterTypes), returnType));
void createScalarFunction(std::string name, std::vector<common::LogicalTypeID> parameterTypes,
common::LogicalTypeID returnType, TR (*udfFunc)(Args...)) {
auto nameCopy = std::string(name);
addScalarFunction(
std::move(nameCopy), function::UDF::getFunction<TR, Args...>(std::move(name), udfFunc,
std::move(parameterTypes), returnType));
}

template<typename TR, typename... Args>
void createVectorizedFunction(const std::string& name, function::scalar_exec_func scalarFunc) {
addScalarFunction(
name, function::UDF::getVectorizedFunction<TR, Args...>(name, std::move(scalarFunc)));
void createVectorizedFunction(std::string name, function::scalar_exec_func scalarFunc) {
auto nameCopy = std::string(name);
addScalarFunction(std::move(nameCopy), function::UDF::getVectorizedFunction<TR, Args...>(
std::move(name), std::move(scalarFunc)));
}

void createVectorizedFunction(const std::string& name,
void createVectorizedFunction(std::string name,
std::vector<common::LogicalTypeID> parameterTypes, common::LogicalTypeID returnType,
function::scalar_exec_func scalarFunc) {
addScalarFunction(name, function::UDF::getVectorizedFunction(name, std::move(scalarFunc),
std::move(parameterTypes), returnType));
auto nameCopy = std::string(name);
addScalarFunction(
std::move(nameCopy), function::UDF::getVectorizedFunction(std::move(name),
std::move(scalarFunc), std::move(parameterTypes), returnType));
}

inline void setReplaceFunc(replace_func_t replaceFunc) {
clientContext->setReplaceFunc(std::move(replaceFunc));
}

private:
std::unique_ptr<QueryResult> query(const std::string& query, const std::string& encodedJoin);
std::unique_ptr<QueryResult> query(std::string_view query, std::string_view encodedJoin);

std::unique_ptr<QueryResult> queryResultWithError(const std::string& errMsg);
std::unique_ptr<QueryResult> queryResultWithError(std::string_view errMsg);

std::unique_ptr<PreparedStatement> prepareNoLock(const std::string& query,
bool enumerateAllPlans = false, const std::string& joinOrder = std::string{});
std::unique_ptr<PreparedStatement> prepareNoLock(std::string_view query,
bool enumerateAllPlans = false, std::string_view joinOrder = std::string_view());

template<typename T, typename... Args>
std::unique_ptr<QueryResult> executeWithParams(PreparedStatement* preparedStatement,
Expand Down
8 changes: 2 additions & 6 deletions src/include/main/database.h
Original file line number Diff line number Diff line change
Expand Up @@ -61,17 +61,13 @@ class Database {
friend struct extension::ExtensionUtils;

public:
/**
* @brief Creates a database object with default buffer pool size and max num threads.
* @param databasePath The path to the database.
*/
KUZU_API explicit Database(std::string databasePath);
/**
* @brief Creates a database object.
* @param databasePath Database path.
* @param systemConfig System configurations (buffer pool size and max num threads).
*/
KUZU_API Database(std::string databasePath, SystemConfig systemConfig);
KUZU_API explicit Database(
std::string_view databasePath, SystemConfig systemConfig = SystemConfig());
/**
* @brief Destructs the database object.
*/
Expand Down
6 changes: 4 additions & 2 deletions src/include/main/query_result.h
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
#pragma once

#include <string>

#include "common/api.h"
#include "common/arrow/arrow.h"
#include "common/types/types.h"
Expand Down Expand Up @@ -90,8 +92,8 @@ class QueryResult {
* @param escapeCharacter escape character of the csv file.
* @param newline newline character of the csv file.
*/
KUZU_API void writeToCSV(const std::string& fileName, char delimiter = ',',
char escapeCharacter = '"', char newline = '\n');
KUZU_API void writeToCSV(std::string fileName, char delimiter = ',', char escapeCharacter = '"',
char newline = '\n');
/**
* @brief Resets the result tuple iterator.
*/
Expand Down
4 changes: 2 additions & 2 deletions src/include/parser/parser.h
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
#pragma once

#include <memory>
#include <string>
#include <string_view>

#include "statement.h"

Expand All @@ -11,7 +11,7 @@ namespace parser {
class Parser {

public:
static std::unique_ptr<Statement> parseQuery(const std::string& query);
static std::unique_ptr<Statement> parseQuery(std::string_view query);
};

} // namespace parser
Expand Down
13 changes: 7 additions & 6 deletions src/main/connection.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -58,33 +58,33 @@
return clientContext->numThreadsForExecution;
}

std::unique_ptr<PreparedStatement> Connection::prepare(const std::string& query) {
std::unique_ptr<PreparedStatement> Connection::prepare(std::string_view query) {
std::unique_lock<std::mutex> lck{mtx};
return prepareNoLock(query);
}

std::unique_ptr<QueryResult> Connection::query(const std::string& query) {
std::unique_ptr<QueryResult> Connection::query(std::string_view query) {
lock_t lck{mtx};
auto preparedStatement = prepareNoLock(query);
return executeAndAutoCommitIfNecessaryNoLock(preparedStatement.get());
}

std::unique_ptr<QueryResult> Connection::query(
const std::string& query, const std::string& encodedJoin) {
std::string_view query, std::string_view encodedJoin) {
lock_t lck{mtx};
auto preparedStatement = prepareNoLock(query, true /* enumerate all plans */, encodedJoin);
return executeAndAutoCommitIfNecessaryNoLock(preparedStatement.get());
}

std::unique_ptr<QueryResult> Connection::queryResultWithError(const std::string& errMsg) {
std::unique_ptr<QueryResult> Connection::queryResultWithError(std::string_view errMsg) {
auto queryResult = std::make_unique<QueryResult>();
queryResult->success = false;
queryResult->errMsg = errMsg;
return queryResult;
}

std::unique_ptr<PreparedStatement> Connection::prepareNoLock(
const std::string& query, bool enumerateAllPlans, const std::string& encodedJoin) {
std::string_view query, bool enumerateAllPlans, std::string_view encodedJoin) {
auto preparedStatement = std::make_unique<PreparedStatement>();
if (query.empty()) {
preparedStatement->success = false;
Expand Down Expand Up @@ -152,7 +152,8 @@
}
}
if (match == nullptr) {
throw ConnectionException("Cannot find a plan matching " + encodedJoin);
throw ConnectionException(
stringFormat("Cannot find a plan matching {}", encodedJoin));

Check warning on line 156 in src/main/connection.cpp

View check run for this annotation

Codecov / codecov/patch

src/main/connection.cpp#L155-L156

Added lines #L155 - L156 were not covered by tests
}
preparedStatement->logicalPlans.push_back(std::move(match));
} else {
Expand Down
6 changes: 2 additions & 4 deletions src/main/database.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -56,10 +56,8 @@ static void getLockFileFlagsAndType(bool readOnly, bool createNew, int& flags, F
lock = readOnly ? FileLockType::READ_LOCK : FileLockType::WRITE_LOCK;
}

Database::Database(std::string databasePath) : Database{std::move(databasePath), SystemConfig()} {}

Database::Database(std::string databasePath, SystemConfig systemConfig)
: databasePath{std::move(databasePath)}, systemConfig{systemConfig} {
Database::Database(std::string_view databasePath, SystemConfig systemConfig)
: databasePath{databasePath}, systemConfig{systemConfig} {
initLoggers();
logger = LoggerUtils::getLogger(LoggerConstants::LoggerEnum::DATABASE);
vfs = std::make_unique<VirtualFileSystem>();
Expand Down
2 changes: 1 addition & 1 deletion src/main/query_result.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ std::string QueryResult::toString() {
}

void QueryResult::writeToCSV(
const std::string& fileName, char delimiter, char escapeCharacter, char newline) {
std::string fileName, char delimiter, char escapeCharacter, char newline) {
std::ofstream file;
file.open(fileName);
std::shared_ptr<FlatTuple> nextTuple;
Expand Down
2 changes: 1 addition & 1 deletion src/parser/parser.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ using namespace antlr4;
namespace kuzu {
namespace parser {

std::unique_ptr<Statement> Parser::parseQuery(const std::string& query) {
std::unique_ptr<Statement> Parser::parseQuery(std::string_view query) {
auto inputStream = ANTLRInputStream(query);
auto parserErrorListener = ParserErrorListener();

Expand Down
3 changes: 2 additions & 1 deletion tools/python_api/src_cpp/py_query_result.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,8 @@ void PyQueryResult::writeToCSV(const py::str& filename, const py::str& delimiter
KU_ASSERT(delimiterStr.size() == 1);
KU_ASSERT(escapeCharacterStr.size() == 1);
KU_ASSERT(newlineStr.size() == 1);
queryResult->writeToCSV(filename, delimiterStr[0], escapeCharacterStr[0], newlineStr[0]);
queryResult->writeToCSV(
std::string(filename), delimiterStr[0], escapeCharacterStr[0], newlineStr[0]);
benjaminwinger marked this conversation as resolved.
Show resolved Hide resolved
}

void PyQueryResult::close() {
Expand Down
6 changes: 5 additions & 1 deletion tools/rust_api/include/kuzu_rs.h
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ std::unique_ptr<std::vector<kuzu::common::LogicalType>> logical_type_get_struct_
const kuzu::common::LogicalType& value);

/* Database */
std::unique_ptr<kuzu::main::Database> new_database(const std::string& databasePath,
std::unique_ptr<kuzu::main::Database> new_database(std::string_view databasePath,
uint64_t bufferPoolSize, uint64_t maxNumThreads, bool enableCompression, bool readOnly);

void database_set_logging_level(kuzu::main::Database& database, const std::string& level);
Expand Down Expand Up @@ -184,4 +184,8 @@ std::unique_ptr<kuzu::common::Value> get_list_value(
std::unique_ptr<kuzu::common::LogicalType> typ, std::unique_ptr<ValueListBuilder> value);
std::unique_ptr<ValueListBuilder> create_list();

inline std::string_view string_view_from_str(rust::Str s) {
return {s.data(), s.size()};
}

} // namespace kuzu_rs
6 changes: 3 additions & 3 deletions tools/rust_api/src/connection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use crate::error::Error;
use crate::ffi::ffi;
use crate::query_result::QueryResult;
use crate::value::Value;
use cxx::{let_cxx_string, UniquePtr};
use cxx::UniquePtr;
use std::cell::UnsafeCell;
use std::convert::TryInto;

Expand Down Expand Up @@ -169,8 +169,8 @@ impl<'a> Connection<'a> {
/// * `query`: The query to prepare.
/// See <https://kuzudb.com/docs/cypher> for details on the query format
pub fn prepare(&self, query: &str) -> Result<PreparedStatement, Error> {
let_cxx_string!(query = query);
let statement = unsafe { (*self.conn.get()).pin_mut() }.prepare(&query)?;
let statement =
unsafe { (*self.conn.get()).pin_mut() }.prepare(ffi::StringView::new(query))?;
if statement.isSuccess() {
Ok(PreparedStatement { statement })
} else {
Expand Down
18 changes: 1 addition & 17 deletions tools/rust_api/src/database.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,10 +79,9 @@ impl Database {
/// * `enable_compression`: When true, table data will be compressed if possible
/// * `read_only`: When true, the database will be opened in READ_ONLY mode, otherwise in READ_WRITE mode.
pub fn new<P: AsRef<Path>>(path: P, config: SystemConfig) -> Result<Self, Error> {
let_cxx_string!(path = path.as_ref().display().to_string());
Ok(Database {
db: UnsafeCell::new(ffi::new_database(
&path,
ffi::StringView::new(&path.as_ref().display().to_string()),
benjaminwinger marked this conversation as resolved.
Show resolved Hide resolved
config.buffer_pool_size,
config.max_num_threads,
config.enable_compression,
Expand Down Expand Up @@ -151,21 +150,6 @@ mod tests {
let result: Error = Database::new("", SystemConfig::default())
.expect_err("An empty string should not be a valid database path!")
.into();
if cfg!(windows) {
assert_eq!(
result.to_string(),
"Failed to create directory due to: create_directory: The system cannot find the path specified.: \"\""
);
} else if cfg!(linux) {
assert_eq!(
result.to_string(),
"Failed to create directory due to: filesystem error: cannot create directory: No such file or directory []"
);
} else {
assert!(result
.to_string()
.starts_with("Failed to create directory due to"));
}
}

#[test]
Expand Down
Loading