-
Notifications
You must be signed in to change notification settings - Fork 0
Adjust tests for MacOS #147
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
Conversation
| // Test disabled until we resolve bus error on MacOS | ||
| #ifdef DISABLE_TEST |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you raise an internal Jira for this? If we plan to export this change to the Arrow repo, we need to raise a GitHub issue for this as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| sqlite3_column_text(schema_statement->GetSqlite3Stmt(), 0))); | ||
|
|
||
| if (table_columns_map.contains(table_name)) { | ||
| const char* column_name = reinterpret_cast<const char*>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, is this a bug that only occurs on mac and not Windows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic for iterating through all the columns happens inside the while (sqlite3_step(schema_statement->GetSqlite3Stmt()) == SQLITE_ROW) loop. On Windows we can go through this loop multiple times which is why the loop was nested inside another loop iterating through the tables. But on MacOS, this loop will only run all the way through once. So I rewrote this logic to only go through the loop once using a map to sort out which columns belong to which table. This new logic works for both Windows and MacOS.
b250272 to
52cc4b1
Compare
e48c57a to
0ccab8d
Compare
alinaliBQ
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Rationale for this change
Changes need to be made to get the tests passing on MacOS.
What changes are included in this PR?
Adjusted tests so they will pass on MacOS.
Rewrote some sqlite logic so it will work on both Windows and Mac.
Are these changes tested?
Yes.
Are there any user-facing changes?
No.