Add Windows Support #1573

benjaminwinger · 2023-05-25T22:31:01Z

A few tests seem to have regressed with the most recent changes from master. I haven't gotten around to looking into those yet, but I thought I should get this PR open for review in the meantime.

Mostly fixes #1504 (note omissions below) and supersedes #1544.

Notable changes

I added a KUZU_ prefix to the type enum in the C API. Unfortunately, windows headers pollute the import namespace making some of the more common names (e.g. BOOL) ambiguous.

I removed the VectorOperations class and moved {Unary,Binary,Ternary,Const}ExecFunction into the enclosing kuzu::function namespace (they could get their own child namespace if that's preferable). MSVC doesn't appear to allow function pointers to templated member functions (see this minimal example).

Omissions

The shell is not being built on windows as linenoise isn't yet working (it looks like the simplest option may be to try to get the current version of linenoise being used working with windows via some patches which exist for older versions of linenoise).
The python build is working, but a CI job for creating wheels has not been set up.
Bulding Debug or RelWithDebInfo on windows is tricky due to how the arrow build is set up and the fact that debug and release builds can't be linked together on windows, so it needs to match the main build. I've added an ARROW_INSTALL option which can point to the installed arrow prefix and can be used to switch between versions of arrow manually. Setting something up in the Makefile to do it automatically should be possible, but would probably be messy.

Also note that performance is rather poor. It seems to largely be related to file i/o.

andyfengHKU · 2023-05-26T00:18:24Z

src/include/function/boolean/vector_boolean_operations.h

@@ -6,7 +6,39 @@
 namespace kuzu {
 namespace function {

-class VectorBooleanOperations : public VectorOperations {
+template<typename FUNC>


Can u try to put these back into VectorBooleanOperations. This probably won't cause compilation problem in MSVC.

src/include/function/comparison/vector_comparison_operations.h

andyfengHKU · 2023-05-26T00:19:33Z

src/include/function/list/vector_list_operations.h

-            *params[0], *params[1], result);
-    }
-
+template<typename A_TYPE, typename B_TYPE, typename C_TYPE, typename RESULT_TYPE, typename FUNC>


ditto. try put back into VectorListOperations

andyfengHKU · 2023-05-26T00:19:52Z

src/include/function/null/vector_null_operations.h

@@ -6,7 +6,23 @@
 namespace kuzu {
 namespace function {

-class VectorNullOperations : public VectorOperations {
+template<typename FUNC>


ditto try put back into VectorNullOperations

andyfengHKU · 2023-05-26T00:43:33Z

test/demo_db/demo_db_test.cpp

@@ -97,15 +97,6 @@ TEST_F(DemoDBTest, DeleteRelTest) {
    ASSERT_EQ(TestHelper::convertResultToString(*result), groundTruth);
 }

-TEST_F(DemoDBTest, DeleteWithExceptionTest) {


Why is this removed?

I'm not sure, that was something @acquamarin did in this commit. @acquamarin?

src/main/connection.cpp

andyfengHKU · 2023-05-26T00:47:46Z

src/main/connection.cpp

-        result += "\t" + tableIDSchema.second->tableName + "\n";
+        relTableNames.push_back(tableIDSchema.second->tableName);
+    }
+    std::sort(relTableNames.begin(), relTableNames.end());


andyfengHKU · 2023-05-26T00:51:39Z

src/include/processor/operator/aggregate/aggregate_hash_table.h

@@ -9,6 +9,13 @@
 namespace kuzu {
 namespace processor {

+template<typename type>
+bool compareEntryWithKeys(const uint8_t* keyValue, const uint8_t* entry) {


ditto try put back. We have many places that are written in a similar way. I don't know why some of them can be compiled while others we need to change

Could you give an example of another place which didn't require changing? That might provide some insight into other ways the issue can be worked around.

It's worth noting that it seems to work fine as long as the function pointer isn't returned from another function (that's the point where the function resolution fails).

E.g. this works, but this does not.

I see. This is a good point. Then I wonder if we also pass functor reference rather than return a functor. Because expose static function in header files is not a good coding style

For example in vector_boolean_operation we can do

void VectorBooleanOperations::bindExecFunction( ExpressionType expressionType, const binder::expression_vector& children, scalar_exec_func& func) { if (isExpressionBinary(expressionType)) { func = bindBinaryExecFunction(expressionType, children); } else { assert(isExpressionUnary(expressionType)); func = bindUnaryExecFunction(expressionType, children); } }

I think @acquamarin, @ray6080 and @benjaminwinger should all comment on this. If so, @benjaminwinger you can ignore my other comment and do this in a separate PR.

That seems like a reasonable solution to me.

I see. This is a good point. Then I wonder if we also pass functor reference rather than return a functor. Because expose static function in header files is not a good coding style

For example in vector_boolean_operation we can do

void VectorBooleanOperations::bindExecFunction( ExpressionType expressionType, const binder::expression_vector& children, scalar_exec_func& func) { if (isExpressionBinary(expressionType)) { func = bindBinaryExecFunction(expressionType, children); } else { assert(isExpressionUnary(expressionType)); func = bindUnaryExecFunction(expressionType, children); } }

I think @acquamarin, @ray6080 and @benjaminwinger should all comment on this. If so, @benjaminwinger you can ignore my other comment and do this in a separate PR.

Sounds good to me.

andyfengHKU · 2023-05-26T00:52:04Z

src/include/processor/operator/order_by/order_by_key_encoder.h

@@ -138,5 +133,9 @@ class OrderByKeyEncoder {
    std::vector<encode_function_t> encodeFunctions;
 };

+template<typename type>


andyfengHKU · 2023-05-26T00:59:59Z

src/include/function/vector_operations.h

-    }
-};
+
+// Note: Previously part of a VectorOperations class, but MSVC is unable to resolve references to the function within subclasses.


I would still try to wrap them in a class. It's not a good idea to expose static function directly.

ray6080

I have some comments. But I kind of want to take a look after rebase with master. As for now, it is not very clear some changes are already in master or made in this PR.

ray6080 · 2023-05-26T16:49:13Z

external/arrow/apache_arrow.cmake

@@ -2,13 +2,19 @@ include(ExternalProject)

 set(CMAKE_POSITION_INDEPENDENT_CODE ON)

+if(WIN32)


We can always keep arrow build type same as cmake build type regardless platforms here.

Sure, though it would trigger an unnecessary rebuild of arrow when switching on other platforms, and building arrow isn't very fast.

ray6080 · 2023-05-26T17:13:01Z

src/storage/store/node_table.cpp

@@ -26,6 +26,13 @@ void NodeTable::initializeData(NodeTableSchema* nodeTableSchema) {
    }
 }

+void NodeTable::resetColumns(catalog::NodeTableSchema* nodeTableSchema) {


Why do we need this ?

This function is used to close the files for nodeTable, if we don't close the files, we can't do the rename

ray6080 · 2023-05-26T17:13:47Z

src/transaction/transaction_manager.cpp

@@ -91,6 +91,7 @@ void TransactionManager::stopNewTransactionsAndWaitUntilAllReadTransactionsLeave
            numTimesWaited++;
            if (numTimesWaited * THREAD_SLEEP_TIME_WHEN_WAITING_IN_MICROS >
                checkPointWaitTimeoutForTransactionsToLeaveInMicros) {
+                mtxForStartingNewTransactions.unlock();


Was this a bug?

ray6080 · 2023-05-26T17:22:33Z

src/storage/store/rel_table.cpp

@@ -73,6 +73,21 @@ void DirectedRelTableData::initializeLists(RelTableSchema* tableSchema, WAL* wal
    }
 }

+void DirectedRelTableData::resetColumnsAndLists(


Ditto here. Is this introduced in this PR?

ray6080 · 2023-05-26T17:25:12Z

src/main/database.cpp

+        status.dwLength = sizeof(status);
+        GlobalMemoryStatusEx(&status);
+        auto systemMemSize = (std::uint64_t)status.ullTotalPhys;
+        bufferPoolSize = (uint64_t)(BufferPoolConstants::DEFAULT_PHY_MEM_SIZE_RATIO_FOR_BM *


Can we move this line bufferPoolSize = (uint64_t)... out of the #if #else ?

acquamarin · 2023-05-26T17:39:09Z

src/common/file_utils.cpp

 std::unique_ptr<FileInfo> FileUtils::openFile(const std::string& path, int flags) {
+#if defined(_WIN32)
+    // Not providing GENERIC_READ seems to cause problems.
+    int dwDesiredAccess = GENERIC_READ;


Our convension is to use auto instead of the actual type

That doesn't seem to be mentioned in the style guide. Is the style guide out of date?

acquamarin · 2023-05-26T17:40:21Z

src/common/file_utils.cpp

+    int dwShareMode = FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE;
+    int dwFlagsAndAttributes = FILE_ATTRIBUTE_NORMAL;
+    if (flags & O_CREAT)
+        dwCreationDisposition = OPEN_ALWAYS;


I think you can rewrite this as auto dwCreationDisposition = (flags & O_CREAT) ? OPEN_ALWAYS : OPEN_EXISTING

acquamarin · 2023-05-26T17:40:23Z

src/common/file_utils.cpp

+    int dwFlagsAndAttributes = FILE_ATTRIBUTE_NORMAL;
+    if (flags & O_CREAT)
+        dwCreationDisposition = OPEN_ALWAYS;
+    if (flags & (O_CREAT | O_WRONLY | O_RDWR))


Our convension is to always add brackets to if statements

acquamarin · 2023-05-26T17:41:53Z

src/common/file_utils.cpp

@@ -125,5 +211,32 @@ std::vector<std::string> FileUtils::globFilePath(const std::string& path) {
    return result;
 }

+void FileUtils::truncateFileToSize(FileInfo* fileInfo, uint64_t size) {
+#if defined(_WIN32)
+    long offsetHigh = size >> 32;


I think the size of long is dependent on the platform. Better to use auto offsetHigh = (uint32_t)(size >> 32)?

It is platform-specific code though. I was following the API for SetFilePointer, which says the arguments are a LONG, which according to this is just typedef long LONG;.

But I'll still change it; it's unintuitive.

Though that actually would require casting the pointer back to a long*/LONG* when passing the argument to SetFilePointer, which seems a little redundant. Maybe it's better to just actually use LONG to indicate it's a microsoft-specified type for a microsoft-specified interface.

src/main/connection.cpp

acquamarin · 2023-05-26T17:48:53Z

src/storage/store/node_table.cpp

@@ -26,6 +26,13 @@ void NodeTable::initializeData(NodeTableSchema* nodeTableSchema) {
    }
 }

+void NodeTable::resetColumns(catalog::NodeTableSchema* nodeTableSchema) {


This function is used to close the files for nodeTable, if we don't close the files, we can't do the rename

codecov · 2023-05-26T21:36:57Z

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.01 ⚠️

Comparison is base (dba7110) 91.95% compared to head (dcbe4b4) 91.95%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1573      +/-   ##
==========================================
- Coverage   91.95%   91.95%   -0.01%     
==========================================
  Files         701      701              
  Lines       25207    25322     +115     
==========================================
+ Hits        23179    23284     +105     
- Misses       2028     2038      +10

Impacted Files	Coverage Δ
src/binder/bind/bind_query.cpp	`100.00% <ø> (ø)`
src/binder/bind/bind_reading_clause.cpp	`92.30% <ø> (ø)`
src/function/vector_null_operations.cpp	`62.50% <ø> (+5.35%)`	⬆️
...function/comparison/vector_comparison_operations.h	`81.42% <ø> (+10.97%)`	⬆️
...ude/function/interval/vector_interval_operations.h	`95.45% <ø> (ø)`
.../include/function/schema/vector_label_operations.h	`100.00% <ø> (ø)`
...include/function/schema/vector_offset_operations.h	`87.50% <ø> (ø)`
...include/function/string/vector_string_operations.h	`96.00% <ø> (ø)`
...include/function/struct/vector_struct_operations.h	`100.00% <ø> (ø)`
src/include/planner/subplans_table.h	`100.00% <ø> (ø)`
... and 43 more

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

ray6080 · 2023-05-27T00:14:47Z

src/common/file_utils.cpp

+
+int64_t FileInfo::getFileSize() {
+#ifdef _WIN32
+    return GetFileSize((HANDLE)handle, nullptr);


Should we use GetFileSizeEx? GetFileSize seems only return 4 bytes as the result, which is not large enough.

ray6080 · 2023-05-27T00:30:59Z

src/common/file_utils.cpp

 std::unique_ptr<FileInfo> FileUtils::openFile(const std::string& path, int flags) {
+#if defined(_WIN32)
+    // Not providing GENERIC_READ seems to cause problems.


Why is this? Can you take a look at this implementation? https://github.com/duckdb/duckdb/blob/09486d2b9d93270aa05920c6e7e54af00ce9d296/src/common/local_file_system.cpp#L524

ray6080 · 2023-05-27T00:33:17Z

src/include/processor/operator/aggregate/aggregate_hash_table.h

@@ -9,6 +9,13 @@
 namespace kuzu {
 namespace processor {

+template<typename type>
+bool compareEntryWithKeys(const uint8_t* keyValue, const uint8_t* entry) {


I see. This is a good point. Then I wonder if we also pass functor reference rather than return a functor. Because expose static function in header files is not a good coding style

For example in vector_boolean_operation we can do

void VectorBooleanOperations::bindExecFunction( ExpressionType expressionType, const binder::expression_vector& children, scalar_exec_func& func) { if (isExpressionBinary(expressionType)) { func = bindBinaryExecFunction(expressionType, children); } else { assert(isExpressionUnary(expressionType)); func = bindUnaryExecFunction(expressionType, children); } }

I think @acquamarin, @ray6080 and @benjaminwinger should all comment on this. If so, @benjaminwinger you can ignore my other comment and do this in a separate PR.

Sounds good to me.

andyfengHKU reviewed May 26, 2023

View reviewed changes

benjaminwinger force-pushed the windows3 branch from 3af46ed to 27e5950 Compare May 26, 2023 16:14

ray6080 reviewed May 26, 2023

View reviewed changes

benjaminwinger force-pushed the windows3 branch 2 times, most recently from 0637b5c to cbac99a Compare May 26, 2023 17:32

acquamarin reviewed May 26, 2023

View reviewed changes

benjaminwinger force-pushed the windows3 branch from cbac99a to 5ef2feb Compare May 26, 2023 20:09

Add Windows Support

dcbe4b4

benjaminwinger force-pushed the windows3 branch from 5ef2feb to dcbe4b4 Compare May 26, 2023 21:15

ray6080 reviewed May 27, 2023

View reviewed changes

andyfengHKU approved these changes May 27, 2023

View reviewed changes

andyfengHKU merged commit 25fd9f0 into kuzudb:master May 27, 2023
7 of 8 checks passed

This was referenced May 29, 2023

Don't always set GENERIC_READ when opening files on windows #1585

Merged

Use GetFileSizeEX instead of GetFileSize on windows #1587

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Windows Support #1573

Add Windows Support #1573

benjaminwinger commented May 25, 2023

andyfengHKU May 26, 2023

andyfengHKU May 26, 2023

andyfengHKU May 26, 2023

andyfengHKU May 26, 2023

benjaminwinger May 26, 2023

andyfengHKU May 26, 2023

andyfengHKU May 26, 2023

benjaminwinger May 26, 2023

andyfengHKU May 26, 2023 •

edited

Loading

benjaminwinger May 26, 2023

ray6080 May 27, 2023

andyfengHKU May 26, 2023

andyfengHKU May 26, 2023

ray6080 left a comment

ray6080 May 26, 2023

benjaminwinger May 26, 2023

ray6080 May 26, 2023

acquamarin May 26, 2023

ray6080 May 26, 2023

ray6080 May 26, 2023

ray6080 May 26, 2023

acquamarin May 26, 2023

benjaminwinger May 26, 2023

acquamarin May 26, 2023

acquamarin May 26, 2023

acquamarin May 26, 2023

benjaminwinger May 26, 2023

benjaminwinger May 26, 2023

acquamarin May 26, 2023

codecov bot commented May 26, 2023

ray6080 May 27, 2023

ray6080 May 27, 2023

ray6080 May 27, 2023

		@@ -2,13 +2,19 @@ include(ExternalProject)

		set(CMAKE_POSITION_INDEPENDENT_CODE ON)

		if(WIN32)

Add Windows Support #1573

Add Windows Support #1573

Conversation

benjaminwinger commented May 25, 2023

Notable changes

Omissions

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyfengHKU May 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ray6080 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 26, 2023

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyfengHKU May 26, 2023 •

edited

Loading