-
Notifications
You must be signed in to change notification settings - Fork 0
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
[GAC-39] update Arrow version that flightsql-odbc
(Dremio Seed) works with
#11
base: GAC-odbc-driver
Are you sure you want to change the base?
[GAC-39] update Arrow version that flightsql-odbc
(Dremio Seed) works with
#11
Conversation
Thanks for opening a pull request! If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project. Then could you also rename the pull request title in the following format?
or
In the case of PARQUET issues on JIRA the title also supports:
See also: |
24421a8
to
0aa0902
Compare
8abf8bd
to
59a015f
Compare
0aa0902
to
65fd7c9
Compare
flightsql-odbc
(Dremio Seed) works with flightsql-odbc
(Dremio Seed) works with
4a96998
to
5c03e97
Compare
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 just one minor nit
* Use 1.3 Zlib version - The built-in base line does not work, since it will try to get zlib from https://www.zlib.net/zlib-1.2.12.tar.gz, which is no longer valid. To fix this, I changed ZLIB version to 1.3. * Fix ARROW_GIT_REPOSITORY not set error * In `flight_sql\CMakeLists.txt`, set(ARROW_GIT_REPOSITORY ... CACHE STRING) only sets the default value of ARROW_GIT_REPOSITORY. This value is supposed to be set in the build script (e.g., `cpp/src/flightsql_odbc/flightsql-odbc/build_win64.bat`), and the setting of this value is fixed. * Create flightsqlodbc_build.yml to add Windows workflow to build flightsql-odbc. Only 64-bit build is supported as Apache Arrow does not support Windows 32-bit build.
* Set C+ standard+ to 17 to accommodate arrow build. `flightsql-odbc` used C++ 11, but arrow 14.0.0 uses C++ 17, so C++ standard is set to 17 to support building of arrow. * Error arrow/utils/optional.h no longer found is fixed. * I found that [Arrow commit 5e49174](apache@5e49174) removes arrow/utils/optional.h and replaces it with <optional> C++ 17 header. So I updated flightsql-odbc code with when looking at this commit. * Removed deprecated `std::binary_function`. * There are build errors from `boost::variant`, so changed `boost::variant` to `std::variant` to align with rest of Arrow repository. Also replaced `boost::get` with `std::get` as a result. * Fixed "error C2660: 'arrow::flight::FlightInfo::GetSchema': function does not take 2 arguments": I checked [commit d214455](apache@d214455) that changed GetSchema, and found that GetSchema(dictionary_memo, out) can be replaced with GetSchema(dictionary_memo).Value(out). * Fixed error C2661: 'arrow::flight::FlightClient::Connect': no overloaded function takes 3 arguments: I checked [commit 9e08c50](apache@9e08c50) that changed Connect, and found that Connect(location, options, client) can be replaced with Connect(location, options).Value(client). * Fixed error C2660: 'arrow::flight::Location::ForGrpcTls': function does not take 3 arguments: I checked [commit d214455](apache@d214455) that changed ForGrpcTls, and found that Connect(host, port, location) can be replaced with ForGrpcTls(host, port).Value(location). Same process to fix error with `Location::ForGrpcTcp`. * Fixed error of scalar->value being deprecated. I see from [commit 6cc37cf](apache@6cc37cf) that scalar::value is replaced with scalar::child_value(), so I went with this change. * Fixed linking errors. `ARROW_FLIGHT_SQL_STATIC` was added in [commit 9c93f82](apache@9c93f82), so now we need to define `ARROW_FLIGHT_SQL_STATIC` in CMakeLists.txt also.
5c03e97
to
e6ba2c8
Compare
Rationale for this change
We plan to let
flightsql-odbc
be build with the Arrow repository that it resides in. As a first step, it needs to work with today's Arrow version.What changes are included in this PR?
Short summary:
flightsql-odbc
.flightsql-odbc
from 11 to 17.flightsql-odbc
from Arrow 9.0.0 version to Arrow 14.0.0 version.Detailed changes:
flightsql-odbc
used C++ 11, but arrow 14.0.0 uses C++ 17, so C++ standard is set to 17 to support building of arrow.arrow/utils/optional.h no longer found
is fixed.removes arrow/utils/optional.h and replaces it with C++ 17 header.
So I updated flightsql-odbc code with when looking at this commit.
std::binary_function
.boost::variant
, so changedboost::variant
tostd::variant
to align with rest of Arrow repository. Also replacedboost::get
withstd::get
as a result.I checked commit https://github.com/Bit-Quill/arrow/commit/d214455c2179f8ba15bcdce48f36bcb123b47123
that changed GetSchema, and found that GetSchema(dictionary_memo, out) can be replaced with GetSchema(dictionary_memo).Value(out).
I checked commit https://github.com/Bit-Quill/arrow/commit/9e08c503f97fe3848039c80ec07e324db7dfbadf
that changed Connect, and found that Connect(location, options, client) can be replaced with Connect(location, options).Value(client).
I checked commit https://github.com/Bit-Quill/arrow/commit/d214455c2179f8ba15bcdce48f36bcb123b47123
that changed ForGrpcTls, and found that Connect(host, port, location) can be replaced with ForGrpcTls(host, port).Value(location). Same process to fix error with
Location::ForGrpcTcp
.that scalar::value is replaced with scalar::child_value(), so I went with this change.
ARROW_FLIGHT_SQL_STATIC
was added in commit https://github.com/Bit-Quill/arrow/commit/9c93f82a06ecae4d0e51f64b25fc63d32701d734, so now we need to defineARROW_FLIGHT_SQL_STATIC
in CMakeLists.txt also.Are these changes tested?
The existing
flightsql-odbc
tests are passing. No new tests have been added as part of this PR.Are there any user-facing changes?
N/A.