You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "alinaliBQ (via GitHub)" <gi...@apache.org> on 2025/10/08 19:50:00 UTC
[PR] GH-46098 : [C++][FlightRPC] ODBC Environment Attribute Implementation [arrow]
alinaliBQ opened a new pull request, #47760:
URL: https://github.com/apache/arrow/pull/47760
### Rationale for this change
ODBC driver needs to set and get environment attributes, such as ODBC driver version.
### What changes are included in this PR?
- Implement SQLGetEnvAttr and SQLSetEnvAttr APIs for retrieving and setting environment attribute values
- Tests
### Are these changes tested?
- Will be tested in CI when PR is ready for review
### Are there any user-facing changes?
No
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46098 : [C++][FlightRPC] ODBC Environment Attribute Implementation [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #47760:
URL: https://github.com/apache/arrow/pull/47760#discussion_r2415221704
##########
cpp/src/arrow/flight/sql/odbc/odbc_api.cc:
##########
@@ -100,16 +100,107 @@ SQLRETURN SQLGetEnvAttr(SQLHENV env, SQLINTEGER attr, SQLPOINTER value_ptr,
ARROW_LOG(DEBUG) << "SQLGetEnvAttr called with env: " << env << ", attr: " << attr
<< ", value_ptr: " << value_ptr << ", buffer_length: " << buffer_length
<< ", str_len_ptr: " << static_cast<const void*>(str_len_ptr);
- // GH-46575 TODO: Implement SQLGetEnvAttr
- return SQL_INVALID_HANDLE;
+
+ using driver::odbcabstraction::DriverException;
+ using ODBC::ODBCEnvironment;
+
+ ODBCEnvironment* environment = reinterpret_cast<ODBCEnvironment*>(env);
+
+ return ODBCEnvironment::ExecuteWithDiagnostics(environment, SQL_ERROR, [=]() {
+ switch (attr) {
+ case SQL_ATTR_ODBC_VERSION: {
+ if (!value_ptr && !str_len_ptr) {
+ throw DriverException("Invalid null pointer for attribute.", "HY000");
+ }
Review Comment:
Test this case as well?
##########
cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc:
##########
@@ -0,0 +1,82 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+#include "arrow/flight/sql/odbc/tests/odbc_test_suite.h"
+
+#ifdef _WIN32
+# include <windows.h>
+#endif
+
+#include <sql.h>
+#include <sqltypes.h>
+#include <sqlucode.h>
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace arrow::flight::sql::odbc {
+
+TEST(SQLGetEnvAttr, TestSQLGetEnvAttrODBCVersion) {
+ // ODBC Environment
Review Comment:
nit, but I don't think these comments add anything
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46098 : [C++][FlightRPC] ODBC Environment Attribute Implementation [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #47760:
URL: https://github.com/apache/arrow/pull/47760#discussion_r2415647500
##########
cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc:
##########
@@ -0,0 +1,82 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+#include "arrow/flight/sql/odbc/tests/odbc_test_suite.h"
+
+#ifdef _WIN32
+# include <windows.h>
+#endif
Review Comment:
Should we use `cpp/src/arrow/flight/sql/odbc/odbcabstraction/include/odbcabstraction/platform.h` instead?
##########
cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc:
##########
@@ -0,0 +1,82 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+#include "arrow/flight/sql/odbc/tests/odbc_test_suite.h"
+
+#ifdef _WIN32
+# include <windows.h>
+#endif
+
+#include <sql.h>
+#include <sqltypes.h>
+#include <sqlucode.h>
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace arrow::flight::sql::odbc {
+
+TEST(SQLGetEnvAttr, TestSQLGetEnvAttrODBCVersion) {
+ // ODBC Environment
+ SQLHENV env;
+
+ SQLINTEGER version;
+
+ // Allocate an environment handle
+ SQLRETURN return_env = SQLAllocEnv(&env);
+
+ EXPECT_EQ(SQL_SUCCESS, return_env);
Review Comment:
Should we use `ASSERT_EQ()` not `EXPECT_EQ()`?
Do we need to proceed this test even when this assertion is failed?
##########
cpp/src/arrow/flight/sql/odbc/odbc_api.cc:
##########
@@ -100,16 +100,107 @@ SQLRETURN SQLGetEnvAttr(SQLHENV env, SQLINTEGER attr, SQLPOINTER value_ptr,
ARROW_LOG(DEBUG) << "SQLGetEnvAttr called with env: " << env << ", attr: " << attr
<< ", value_ptr: " << value_ptr << ", buffer_length: " << buffer_length
<< ", str_len_ptr: " << static_cast<const void*>(str_len_ptr);
- // GH-46575 TODO: Implement SQLGetEnvAttr
- return SQL_INVALID_HANDLE;
+
+ using driver::odbcabstraction::DriverException;
+ using ODBC::ODBCEnvironment;
+
+ ODBCEnvironment* environment = reinterpret_cast<ODBCEnvironment*>(env);
+
+ return ODBCEnvironment::ExecuteWithDiagnostics(environment, SQL_ERROR, [=]() {
+ switch (attr) {
+ case SQL_ATTR_ODBC_VERSION: {
+ if (!value_ptr && !str_len_ptr) {
+ throw DriverException("Invalid null pointer for attribute.", "HY000");
+ }
Review Comment:
Should we check this out of this `switch`?
##########
cpp/src/arrow/flight/sql/odbc/odbc_api.cc:
##########
@@ -100,16 +100,107 @@ SQLRETURN SQLGetEnvAttr(SQLHENV env, SQLINTEGER attr, SQLPOINTER value_ptr,
ARROW_LOG(DEBUG) << "SQLGetEnvAttr called with env: " << env << ", attr: " << attr
<< ", value_ptr: " << value_ptr << ", buffer_length: " << buffer_length
<< ", str_len_ptr: " << static_cast<const void*>(str_len_ptr);
Review Comment:
Can we remove this debug print now?
##########
cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc:
##########
@@ -0,0 +1,82 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+#include "arrow/flight/sql/odbc/tests/odbc_test_suite.h"
+
+#ifdef _WIN32
+# include <windows.h>
+#endif
+
+#include <sql.h>
+#include <sqltypes.h>
+#include <sqlucode.h>
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace arrow::flight::sql::odbc {
+
+TEST(SQLGetEnvAttr, TestSQLGetEnvAttrODBCVersion) {
+ // ODBC Environment
+ SQLHENV env;
+
+ SQLINTEGER version;
+
+ // Allocate an environment handle
+ SQLRETURN return_env = SQLAllocEnv(&env);
+
+ EXPECT_EQ(SQL_SUCCESS, return_env);
+
+ SQLRETURN return_get = SQLGetEnvAttr(env, SQL_ATTR_ODBC_VERSION, &version, 0, 0);
+
+ EXPECT_EQ(SQL_SUCCESS, return_get);
+
+ EXPECT_EQ(SQL_OV_ODBC2, version);
+}
+
+TEST(SQLSetEnvAttr, TestSQLSetEnvAttrODBCVersionValid) {
+ // ODBC Environment
+ SQLHENV env;
+
+ // Allocate an environment handle
+ SQLRETURN return_env = SQLAllocEnv(&env);
+
+ EXPECT_EQ(SQL_SUCCESS, return_env);
+
+ // Attempt to set to unsupported version
+ SQLRETURN return_set =
+ SQLSetEnvAttr(env, SQL_ATTR_ODBC_VERSION, reinterpret_cast<void*>(SQL_OV_ODBC2), 0);
+
+ EXPECT_EQ(SQL_SUCCESS, return_set);
Review Comment:
Is `SQL_SUCCESS` expected?
##########
cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc:
##########
@@ -0,0 +1,82 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+#include "arrow/flight/sql/odbc/tests/odbc_test_suite.h"
+
+#ifdef _WIN32
+# include <windows.h>
+#endif
+
+#include <sql.h>
+#include <sqltypes.h>
+#include <sqlucode.h>
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace arrow::flight::sql::odbc {
+
+TEST(SQLGetEnvAttr, TestSQLGetEnvAttrODBCVersion) {
+ // ODBC Environment
+ SQLHENV env;
+
+ SQLINTEGER version;
+
+ // Allocate an environment handle
+ SQLRETURN return_env = SQLAllocEnv(&env);
+
+ EXPECT_EQ(SQL_SUCCESS, return_env);
+
+ SQLRETURN return_get = SQLGetEnvAttr(env, SQL_ATTR_ODBC_VERSION, &version, 0, 0);
+
+ EXPECT_EQ(SQL_SUCCESS, return_get);
+
+ EXPECT_EQ(SQL_OV_ODBC2, version);
+}
+
+TEST(SQLSetEnvAttr, TestSQLSetEnvAttrODBCVersionValid) {
+ // ODBC Environment
+ SQLHENV env;
+
+ // Allocate an environment handle
+ SQLRETURN return_env = SQLAllocEnv(&env);
+
+ EXPECT_EQ(SQL_SUCCESS, return_env);
+
+ // Attempt to set to unsupported version
+ SQLRETURN return_set =
+ SQLSetEnvAttr(env, SQL_ATTR_ODBC_VERSION, reinterpret_cast<void*>(SQL_OV_ODBC2), 0);
+
+ EXPECT_EQ(SQL_SUCCESS, return_set);
Review Comment:
Should we assert set value by `SQLGetEnvAttr()`?
##########
cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc:
##########
@@ -0,0 +1,82 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+#include "arrow/flight/sql/odbc/tests/odbc_test_suite.h"
+
+#ifdef _WIN32
+# include <windows.h>
+#endif
+
+#include <sql.h>
+#include <sqltypes.h>
+#include <sqlucode.h>
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace arrow::flight::sql::odbc {
+
+TEST(SQLGetEnvAttr, TestSQLGetEnvAttrODBCVersion) {
+ // ODBC Environment
+ SQLHENV env;
+
+ SQLINTEGER version;
+
+ // Allocate an environment handle
+ SQLRETURN return_env = SQLAllocEnv(&env);
+
+ EXPECT_EQ(SQL_SUCCESS, return_env);
+
+ SQLRETURN return_get = SQLGetEnvAttr(env, SQL_ATTR_ODBC_VERSION, &version, 0, 0);
+
+ EXPECT_EQ(SQL_SUCCESS, return_get);
+
+ EXPECT_EQ(SQL_OV_ODBC2, version);
+}
+
+TEST(SQLSetEnvAttr, TestSQLSetEnvAttrODBCVersionValid) {
+ // ODBC Environment
+ SQLHENV env;
+
+ // Allocate an environment handle
+ SQLRETURN return_env = SQLAllocEnv(&env);
+
+ EXPECT_EQ(SQL_SUCCESS, return_env);
+
+ // Attempt to set to unsupported version
Review Comment:
Is this comment correct?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46098 : [C++][FlightRPC] ODBC Environment Attribute Implementation [arrow]
Posted by "alinaliBQ (via GitHub)" <gi...@apache.org>.
alinaliBQ commented on PR #47760:
URL: https://github.com/apache/arrow/pull/47760#issuecomment-3383021651
@lidavidm @kou Please review this draft ODBC API PR. The testing folder structure will be in a separate PR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46098 : [C++][FlightRPC] ODBC Environment Attribute Implementation [arrow]
Posted by "alinaliBQ (via GitHub)" <gi...@apache.org>.
alinaliBQ commented on PR #47760:
URL: https://github.com/apache/arrow/pull/47760#issuecomment-3438776430
Rebased on top of `main` branch to resolve merge conflicts. This PR is ready for review
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46098 : [C++][FlightRPC] ODBC Environment Attribute Implementation [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm merged PR #47760:
URL: https://github.com/apache/arrow/pull/47760
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46098 : [C++][FlightRPC] ODBC Environment Attribute Implementation [arrow]
Posted by "alinaliBQ (via GitHub)" <gi...@apache.org>.
alinaliBQ commented on code in PR #47760:
URL: https://github.com/apache/arrow/pull/47760#discussion_r2446303776
##########
cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc:
##########
@@ -0,0 +1,82 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+#include "arrow/flight/sql/odbc/tests/odbc_test_suite.h"
+
+#ifdef _WIN32
+# include <windows.h>
+#endif
Review Comment:
fixed
##########
cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc:
##########
@@ -0,0 +1,82 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+#include "arrow/flight/sql/odbc/tests/odbc_test_suite.h"
+
+#ifdef _WIN32
+# include <windows.h>
+#endif
+
+#include <sql.h>
+#include <sqltypes.h>
+#include <sqlucode.h>
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace arrow::flight::sql::odbc {
+
+TEST(SQLGetEnvAttr, TestSQLGetEnvAttrODBCVersion) {
+ // ODBC Environment
+ SQLHENV env;
+
+ SQLINTEGER version;
+
+ // Allocate an environment handle
+ SQLRETURN return_env = SQLAllocEnv(&env);
+
+ EXPECT_EQ(SQL_SUCCESS, return_env);
Review Comment:
fixed to use `ASSERT_EQ`
##########
cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc:
##########
@@ -0,0 +1,82 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+#include "arrow/flight/sql/odbc/tests/odbc_test_suite.h"
+
+#ifdef _WIN32
+# include <windows.h>
+#endif
+
+#include <sql.h>
+#include <sqltypes.h>
+#include <sqlucode.h>
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace arrow::flight::sql::odbc {
+
+TEST(SQLGetEnvAttr, TestSQLGetEnvAttrODBCVersion) {
+ // ODBC Environment
+ SQLHENV env;
+
+ SQLINTEGER version;
+
+ // Allocate an environment handle
+ SQLRETURN return_env = SQLAllocEnv(&env);
+
+ EXPECT_EQ(SQL_SUCCESS, return_env);
+
+ SQLRETURN return_get = SQLGetEnvAttr(env, SQL_ATTR_ODBC_VERSION, &version, 0, 0);
+
+ EXPECT_EQ(SQL_SUCCESS, return_get);
+
+ EXPECT_EQ(SQL_OV_ODBC2, version);
+}
+
+TEST(SQLSetEnvAttr, TestSQLSetEnvAttrODBCVersionValid) {
+ // ODBC Environment
+ SQLHENV env;
+
+ // Allocate an environment handle
+ SQLRETURN return_env = SQLAllocEnv(&env);
+
+ EXPECT_EQ(SQL_SUCCESS, return_env);
+
+ // Attempt to set to unsupported version
+ SQLRETURN return_set =
+ SQLSetEnvAttr(env, SQL_ATTR_ODBC_VERSION, reinterpret_cast<void*>(SQL_OV_ODBC2), 0);
+
+ EXPECT_EQ(SQL_SUCCESS, return_set);
Review Comment:
yes `SQL_SUCCESS` is expected since the version is supposed to be supported.
Added check for `SQLGetEnvAttr`
##########
cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc:
##########
@@ -0,0 +1,82 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+#include "arrow/flight/sql/odbc/tests/odbc_test_suite.h"
+
+#ifdef _WIN32
+# include <windows.h>
+#endif
+
+#include <sql.h>
+#include <sqltypes.h>
+#include <sqlucode.h>
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace arrow::flight::sql::odbc {
+
+TEST(SQLGetEnvAttr, TestSQLGetEnvAttrODBCVersion) {
+ // ODBC Environment
+ SQLHENV env;
+
+ SQLINTEGER version;
+
+ // Allocate an environment handle
+ SQLRETURN return_env = SQLAllocEnv(&env);
+
+ EXPECT_EQ(SQL_SUCCESS, return_env);
+
+ SQLRETURN return_get = SQLGetEnvAttr(env, SQL_ATTR_ODBC_VERSION, &version, 0, 0);
+
+ EXPECT_EQ(SQL_SUCCESS, return_get);
+
+ EXPECT_EQ(SQL_OV_ODBC2, version);
+}
+
+TEST(SQLSetEnvAttr, TestSQLSetEnvAttrODBCVersionValid) {
+ // ODBC Environment
+ SQLHENV env;
+
+ // Allocate an environment handle
+ SQLRETURN return_env = SQLAllocEnv(&env);
+
+ EXPECT_EQ(SQL_SUCCESS, return_env);
+
+ // Attempt to set to unsupported version
Review Comment:
fixed the comment, thanks good catch
##########
cpp/src/arrow/flight/sql/odbc/tests/connection_test.cc:
##########
@@ -0,0 +1,82 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+#include "arrow/flight/sql/odbc/tests/odbc_test_suite.h"
+
+#ifdef _WIN32
+# include <windows.h>
+#endif
+
+#include <sql.h>
+#include <sqltypes.h>
+#include <sqlucode.h>
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace arrow::flight::sql::odbc {
+
+TEST(SQLGetEnvAttr, TestSQLGetEnvAttrODBCVersion) {
+ // ODBC Environment
Review Comment:
removed
##########
cpp/src/arrow/flight/sql/odbc/odbc_api.cc:
##########
@@ -100,16 +100,107 @@ SQLRETURN SQLGetEnvAttr(SQLHENV env, SQLINTEGER attr, SQLPOINTER value_ptr,
ARROW_LOG(DEBUG) << "SQLGetEnvAttr called with env: " << env << ", attr: " << attr
<< ", value_ptr: " << value_ptr << ", buffer_length: " << buffer_length
<< ", str_len_ptr: " << static_cast<const void*>(str_len_ptr);
- // GH-46575 TODO: Implement SQLGetEnvAttr
- return SQL_INVALID_HANDLE;
+
+ using driver::odbcabstraction::DriverException;
+ using ODBC::ODBCEnvironment;
+
+ ODBCEnvironment* environment = reinterpret_cast<ODBCEnvironment*>(env);
+
+ return ODBCEnvironment::ExecuteWithDiagnostics(environment, SQL_ERROR, [=]() {
+ switch (attr) {
+ case SQL_ATTR_ODBC_VERSION: {
+ if (!value_ptr && !str_len_ptr) {
+ throw DriverException("Invalid null pointer for attribute.", "HY000");
+ }
Review Comment:
Added a test `DISABLED_TestSQLGetEnvAttrNullValuePointer` for this case. The test case is disabled because call to SQLGetEnvAttr is handled by the driver manager on Windows. And the Windows driver manager doesn't error out when null pointer is passed, even though it should.
Potentially this can be tested on mac/linux depending on the driver manager on these systems
##########
cpp/src/arrow/flight/sql/odbc/odbc_api.cc:
##########
@@ -100,16 +100,107 @@ SQLRETURN SQLGetEnvAttr(SQLHENV env, SQLINTEGER attr, SQLPOINTER value_ptr,
ARROW_LOG(DEBUG) << "SQLGetEnvAttr called with env: " << env << ", attr: " << attr
<< ", value_ptr: " << value_ptr << ", buffer_length: " << buffer_length
<< ", str_len_ptr: " << static_cast<const void*>(str_len_ptr);
Review Comment:
We generally log the ODBC API calls for debugging so we know which ODBC APIs got passed to the driver, is it ok to keep these logs?
##########
cpp/src/arrow/flight/sql/odbc/odbc_api.cc:
##########
@@ -100,16 +100,107 @@ SQLRETURN SQLGetEnvAttr(SQLHENV env, SQLINTEGER attr, SQLPOINTER value_ptr,
ARROW_LOG(DEBUG) << "SQLGetEnvAttr called with env: " << env << ", attr: " << attr
<< ", value_ptr: " << value_ptr << ", buffer_length: " << buffer_length
<< ", str_len_ptr: " << static_cast<const void*>(str_len_ptr);
- // GH-46575 TODO: Implement SQLGetEnvAttr
- return SQL_INVALID_HANDLE;
+
+ using driver::odbcabstraction::DriverException;
+ using ODBC::ODBCEnvironment;
+
+ ODBCEnvironment* environment = reinterpret_cast<ODBCEnvironment*>(env);
+
+ return ODBCEnvironment::ExecuteWithDiagnostics(environment, SQL_ERROR, [=]() {
+ switch (attr) {
+ case SQL_ATTR_ODBC_VERSION: {
+ if (!value_ptr && !str_len_ptr) {
+ throw DriverException("Invalid null pointer for attribute.", "HY000");
+ }
Review Comment:
Probably not in this case. I think attribute needs to be checked first before the pointers, and ODBC needs to error out on attribute value first.
In a case where attribute, `value_ptr` and `str_len_ptr` are all invalid, `HYC00` (error code for invalid attribute) should be returned first. So we put `value_ptr` and `str_len_ptr` check under the `switch` statement
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46098 : [C++][FlightRPC] ODBC Environment Attribute Implementation [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #47760:
URL: https://github.com/apache/arrow/pull/47760#issuecomment-3442074494
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 56e38362def18a5822e137c14b3d10bf7516a5e2.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/53569135745) has more details. It also includes information about 11 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org