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-710476: bulk update essential #776

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

SNOW-710476: bulk update essential #776

wants to merge 5 commits into from
+1,665 −106

Conversation

sfc-gh-ext-simba-hx
Copy link
Collaborator

essential functionality only.
Will add following features in separated PR:

  • batch execution for query with "arrayBindSupported=false" returned from server.
  • error handling: fallback to regular binding when stage binding failed.
  • configuration to disable stage binding in case customer have issue with it.

@sfc-gh-ext-simba-hx sfc-gh-ext-simba-hx requested a review from a team as a code owner November 13, 2024 00:34
@sfc-gh-ext-simba-hx sfc-gh-ext-simba-hx changed the base branch from master to master-2.0.0 November 13, 2024 00:34
@sfc-gh-ext-simba-hx sfc-gh-ext-simba-hx marked this pull request as draft November 13, 2024 01:03
@sfc-gh-ext-simba-hx sfc-gh-ext-simba-hx force-pushed the SNOW-710476-bulk-update-essential branch 5 times, most recently from 4aec3d8 to 10ecadb Compare November 19, 2024 21:00
@sfc-gh-ext-simba-hx sfc-gh-ext-simba-hx marked this pull request as ready for review November 19, 2024 23:10
@sfc-gh-ext-simba-hx
Copy link
Collaborator Author

@sfc-gh-jszczerbinski cc @sfc-gh-dprzybysz Please help me to check what's the failure on continuous-integration/jenkins/pr-head

@sfc-gh-dprzybysz
Copy link
Collaborator

sfc-gh-dprzybysz commented Nov 20, 2024 •
edited
Loading

@sfc-gh-ext-simba-hx I see some problems related with CMake on jenkins:

2024-11-19 22:14:09  CMake Error at tests/CMakeLists.txt:130 (elseif):
2024-11-19 22:14:09    given arguments:
2024-11-19 22:14:09  
2024-11-19 22:14:09      "AND" "Release" "STREQUAL" "Debug"
2024-11-19 22:14:09  
2024-11-19 22:14:09    Unknown arguments specified
2024-11-19 22:14:09  
2024-11-19 22:14:09  
2024-11-19 22:14:09  -- Configuring incomplete, errors occurred!
Stage "Tests" skipped due to earlier failure(s)
ERROR: script returned exit code 1

@sfc-gh-jszczerbinski was changing the minimal version of make recently - I think we should merge or rebase the 2.0 branch with main

@sfc-gh-ext-simba-hx sfc-gh-ext-simba-hx force-pushed the SNOW-710476-bulk-update-essential branch 2 times, most recently from 0a61e80 to 7dcf893 Compare November 20, 2024 19:28
@sfc-gh-dprzybysz
Copy link
Collaborator

Hi @sfc-gh-ext-simba-hx now all linux aarch64 test jobs fails with:

2024-11-21 01:17:04 6: Test command: /mnt/host/cmake-build-Release/tests/test_bind_params
2024-11-21 01:17:04 6: Working Directory: /mnt/host/cmake-build-Release/tests
2024-11-21 01:17:04 6: Test timeout computed to be: 10000000
2024-11-21 01:17:04 6: [==========] Running 3 test(s).
2024-11-21 01:17:04 6: [ RUN      ] test_bind_parameters
2024-11-21 01:17:07 6: [       OK ] test_bind_parameters
2024-11-21 01:17:07 6: [ RUN      ] test_array_binding_normal
2024-11-21 01:17:09 6: [  ERROR   ] --- "244" != "-12"
2024-11-21 01:17:09 6: [   LINE   ] --- /mnt/host/tests/test_bind_params.c:286: error: Failure!
2024-11-21 01:17:09 6: [  FAILED  ] test_array_binding_normal
2024-11-21 01:17:09 6: [ RUN      ] test_array_binding_stage
2024-11-21 01:17:13 6: [  ERROR   ] --- "244" != "-12"
2024-11-21 01:17:13 6: [   LINE   ] --- /mnt/host/tests/test_bind_params.c:286: error: Failure!
2024-11-21 01:17:13 6: [  FAILED  ] test_array_binding_stage
2024-11-21 01:17:13 6: [==========] 3 test(s) run.
2024-11-21 01:17:13 6: [  PASSED  ] 1 test(s).
2024-11-21 01:17:13 6: [  FAILED  ] 2 test(s), listed below:
2024-11-21 01:17:13 6: [  FAILED  ] test_array_binding_normal
2024-11-21 01:17:13 6: [  FAILED  ] test_array_binding_stage
2024-11-21 01:17:13 6: 
2024-11-21 01:17:13 6:  2 FAILED TEST(S)
2024-11-21 01:17:13  6/65 Test  #6: test_bind_params ......................***Failed    8.38 sec
...
2024-11-21 01:22:11 The following tests FAILED:
2024-11-21 01:22:11 	  6 - test_bind_params (SEGFAULT)
2024-11-21 01:22:11 Errors while running CTest

sfc-gh-dprzybysz
sfc-gh-dprzybysz reviewed Nov 22, 2024
View reviewed changes
cpp/lib/ClientBindUploader.cpp
{
namespace Client
{
ClientBindUploader::ClientBindUploader(SF_STMT *sfstmt,
Copy link
Collaborator

Choose a reason for hiding this comment

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

ClientBindUploader is separated from BindUploader since BindUploader is fully ported from ODBC and will be still used in ODBC?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

BindUploader is ported from ODBC and refactored so it can be reused on both libsfclient and ODBC side. When reusing it on ODBC side there would be ODBCBindUploader class to have ODBC specific implementation.

include/snowflake/BindUploader.hpp
namespace Client
{

class BindUploader
Copy link
Collaborator

Choose a reason for hiding this comment

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

with add this file to include/snowflake for ODBC usage? if so we should consider it not as a public API of the driver - could we put some "internal" notice on top of this header file?

Copy link
Collaborator

Choose a reason for hiding this comment

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

and for 2.x we can mark in that way all files that we should consider internal

Copy link
Collaborator Author

@sfc-gh-ext-simba-hx sfc-gh-ext-simba-hx Nov 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Sure. Will add "internal" notice on top of the header file.
I think we might need that in most (if not all) hpp files since in general libsfclient is C API library any C++ implementation should be for internal use only.

include/snowflake/client.h
@@ -875,6 +906,8 @@ uint64 STDCALL snowflake_num_params(SF_STMT *sfstmt);
*
* For Positional parameters:
* SF_BIND_INPUT name = NULL;
*
Copy link
Collaborator

Choose a reason for hiding this comment

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

nitpicking: seems to be unnecessary new lines?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sfc-gh-dprzybysz sfc-gh-dprzybysz sfc-gh-dprzybysz left review comments

@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.
2 participants
@sfc-gh-ext-simba-hx @sfc-gh-dprzybysz