Prefer string_view in the API over std::string

[benjaminwinger]

For #2521, but it also is generally preferable over const std::string& since std::string already has a layer of indirection to refer to the owned string data.

This change doesn't really have any cost other than in QueryResult::writeToCSV I think (where the string_view needs to be converted to a const char* to pass it to ofstream::open, which may not work since std::string_view isn't necessarily null-terminated, so I have it converting it to a std::string to ensure that it is).

I also modified Database to store an absolute path, instead of a copy of the path passed to it, to solve a minor issue I'd noticed: that the database will try to move if you chdir after creating it using a relative path. (Removed as this won't play nicely with the VFS at the moment, and integrating the changes would be more work that should probably be done separately).

This doesn't by itself fix #2521. In addition to some functions where it would be more complicated to remove the std::string (e.g. QueryResult::toString), some of the types themselves contain STL types which won't have a consistent size between the release and debug builds. To fix that, we can hide the implementations so that the Database, Connection, QueryResult, etc. are just wrappers containing a unique_ptr to internal types which contain the actual implementation (which will also keep our exposed API more concise), but that's more complicated and can be done in a separate PR.

[codecov]

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (7e4e0ea) 93.37% compared to head (47d5bfc) 93.37%.

Files	Patch %	Lines
src/main/connection.cpp	60.00%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2553   +/-   ##
=======================================
  Coverage   93.37%   93.37%           
=======================================
  Files        1063     1063           
  Lines       39403    39404    +1     
=======================================
+ Hits        36794    36795    +1     
  Misses       2609     2609           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[acquamarin]

This api has already been replaced by 'copy to' statement, so you can remove it.

[benjaminwinger]

COPY ... TO (according to the docs) doesn't have a newline option, so it can't be used as a direct replacement in one of the tests (AlternateDelimCSVTest), though we could just change that test to expect \n as the newline character. Or maybe just remove those tests, or even update and add them to copy_to_csv.test.

[benjaminwinger]

I'm going to merge this as-is. We can discuss removing writeToCSV later.

[Riolku]

It doesn't make sense to use std::string_view in the API when we are going to construct a string regardless. In this case, we should take a std::string by value and let the user decide how to construct it.

[benjaminwinger]

That's true. Though it brings up the question of do we want to prefer string_view anyway for #2521 (noting again that it's still not working in that context for other reasons), or should we take the more performant route to taking strings by value and moving them when we want ownership? Maybe we should go with the latter for the moment, but eventually move towards the former when and if the rest of what's required for #2521 is implemented?

[Riolku]

Personally, I think we should just distribute a debug version of our windows DLL. Other approaches hurt everyone across the board.

[benjaminwinger]

#2521 was also about compiling from scratch though, and even the dynamic debug versions are quite large. On the other hand it shouldn't be that hard for someone linking against kuzu to link against different versions depending on the build mode, but it does take more time, work and disk space, as well as slows things down unnecessarily assuming they are debugging their code and not kuzu.