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.

Sign up for GitHub

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

Jump to bottom

SNOW-692945: refactoring for resultset C wrapper #724

Open
wants to merge 11 commits into
base: master-2.0.0
Choose a base branch
from
Open

SNOW-692945: refactoring for resultset C wrapper #724

wants to merge 11 commits into from

Conversation

SimbaGithub
Copy link
Collaborator

Preparing for adding multiple statements support, refactoring on the resultset C wrapper. Remove unnecessary code and make it more cleaner for adding new feature.

@SimbaGithub SimbaGithub requested a review from a team as a code owner May 31, 2024 16:29
@SimbaGithub SimbaGithub force-pushed the SNOW-1371086-multiple-statements branch from 52a9e80 to 0c5b00a Compare May 31, 2024 17:30
@SimbaGithub SimbaGithub mentioned this pull request Jun 11, 2024
Open
@SimbaGithub SimbaGithub force-pushed the SNOW-1371086-multiple-statements branch from 0c5b00a to a6b3a3e Compare June 13, 2024 17:18
sfc-gh-dprzybysz
sfc-gh-dprzybysz reviewed Jul 5, 2024
View reviewed changes
cpp/lib/ResultSetArrow.cpp Outdated Show resolved Hide resolved
cpp/lib/result_set.cpp Outdated Show resolved Hide resolved
include/snowflake/client.h Outdated Show resolved Hide resolved
include/snowflake/client.h Outdated Show resolved Hide resolved
include/snowflake/client.h Outdated
@@ -443,9 +453,9 @@ typedef struct SF_STMT {
char request_id[SF_UUID4_LEN];
SF_ERROR_STRUCT error;
SF_CONNECT *connection;
void *qrf;
QueryResultFormat_t qrf;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this change compatible for users? our public API is changing there - don't we need * anymore?

Copy link
Collaborator

@sfc-gh-ext-simba-hx sfc-gh-ext-simba-hx Jul 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it was void* and I don't think application would use it without knowing the actual data type it points to.
It was actually pointing to QueryResultFormat_t type which was dynamically allocated, having the enum data directly would make much more sense.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And that't the reason why we cannot merge it even now - right?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A part of. Would like to have 2 approvals (which is required when merging to master) although merging to master-2.0.0 doesn't really need that.

@sfc-gh-ext-simba-hx sfc-gh-ext-simba-hx changed the title SNOW-1371086: refactoring for resultset C wrapper SNOW-692945: refactoring for resultset C wrapper Aug 1, 2024
@sfc-gh-ext-simba-hx
Copy link
Collaborator

@sfc-gh-dstempniak Please help to check what's the failure with continuous-integration/jenkins/pr-head

@sfc-gh-dstempniak
Copy link
Contributor

@sfc-gh-ext-simba-hx One test failed on Macaarch64:

2024-09-05 00:02:15 56: [ RUN      ] test_simple_put_use_dev_urandom
2024-09-05 00:02:15 56: [  ERROR   ] --- Test failed with exception: Segmentation fault: 11(11)
2024-09-05 00:02:15 56: [  FAILED  ] test_simple_put_use_dev_urandom

After rerun it succeeded:

2024-09-05 12:21:30 56: [ RUN      ] test_simple_put_use_dev_urandom
2024-09-05 12:21:34 56: [       OK ] test_simple_put_use_dev_urandom

@sfc-gh-ext-simba-hx sfc-gh-ext-simba-hx changed the base branch from master to master-2.0.0 October 3, 2024 16:46
sfc-gh-jszczerbinski
sfc-gh-jszczerbinski requested changes Oct 9, 2024
View reviewed changes
cpp/lib/result_set.cpp Outdated
}
QueryResultFormat query_result_format =
static_cast<Snowflake::Client::ResultSet*>(rs)->getResultFormat();
switch (query_result_format){
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use virtual destructor instead of casting

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

cpp/lib/result_set.cpp Outdated
{
switch (*query_result_format)
ERROR_IF_NULL(rs);
QueryResultFormat query_result_format =
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use virtual function

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

cpp/lib/result_set.cpp Outdated
return rs_json_create(json_rowset, metadata, tz_string);
#ifndef SF_WIN32
case SF_ARROW_FORMAT:
return new Snowflake::Client::ResultSetArrow(json_rowset, metadata, std::string(tz_string));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Create factory method in ResultSet?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

cpp/lib/result_set.cpp Outdated
return rs_arrow_create_with_chunk((NON_JSON_RESP*)initial_chunk, metadata, tz_string);
case JSON_FORMAT:
return rs_json_create((cJSON*)initial_chunk, metadata, tz_string);
#ifndef SF_WIN32
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Create factory method in ResultSet?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@sfc-gh-dprzybysz sfc-gh-dprzybysz mentioned this pull request Oct 11, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sfc-gh-ext-simba-hx sfc-gh-ext-simba-hx sfc-gh-ext-simba-hx left review comments

@sfc-gh-dprzybysz sfc-gh-dprzybysz sfc-gh-dprzybysz approved these changes

@sfc-gh-dstempniak sfc-gh-dstempniak sfc-gh-dstempniak approved these changes

@sfc-gh-jszczerbinski sfc-gh-jszczerbinski Awaiting requested review from sfc-gh-jszczerbinski

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@SimbaGithub @sfc-gh-ext-simba-hx @sfc-gh-dstempniak @sfc-gh-dprzybysz @sfc-gh-jszczerbinski