Skip to content

Fix flightsql-odbc build error #1

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
Merged
merged 14 commits into from
May 10, 2025
Merged

Fix flightsql-odbc build error #1

merged 14 commits into from
May 10, 2025

Conversation

@alinaliBQ
Copy link
Collaborator

@alinaliBQ alinaliBQ commented May 6, 2025

Rationale for this change

Fix flightsql-odbc build error

What changes are included in this PR?

  • Build fixes for flightsql-odbc. For example to use newer versions of arrow code.
  • Initial framework for ODBC code

Are these changes tested?

Changes are tested locally and by GitHub Actions

Are there any user-facing changes?

No

alinaliBQ and others added 8 commits April 9, 2025 14:43
@alinaliBQ
Add initial framework for odbc dll
f795885 
- Add ARROW_FLIGHT_SQL_ODBC option. If we set `ARROW_FLIGHT_SQL_ODBC=ON`, the flightsql odbc folder will be built
- Add odbc api layer for entry_point.cc
- builds odbc dll file, with ODBC APIs exported in odbc.def
@alinaliBQ
Address James' comments
b251d50
@alinaliBQ
Fix odbcabstraction build errors and partially fix flightsql_odbc…
d85771f 
… errors

Fix boost-variant not found error
- Adding dependencies from odbc/vcpkg.json to cpp/vcpkg.json

- Fix whereami.cc and .h dependency; ported lates code

Update whereami.cc

- use `long` instead of `int64`. Fixed namespace issues.

- PR CI fix: Add `parquet-testing` back

Partial build fix for `flight_sql` folder

- Replaced `namespace arrow` and `namespace odbcabstraction` with `using namespace ...`

- fix flight_sql_connection.cc

Fix `util::nullopt` to use `std::nullopt`

- fix std::optional

- fix BufferReader

- Fix GetSchema

- fix json_converter.cc

- partial fix configuration.h

-  partial fix get_info_cache.cc

- Fix winsock build error

- Comment out `flight_sql` files that cannot build
   - Comment out configuration and unit tests
   - Comment out get info cache and system trust store
@rscales
Create initial odbc tests folder
4485696
@alinaliBQ @rscales
Implement SQLAllocEnv
f7d52d5 
Fix cmake build
@rscales
Implement SQLFreeEnv
0983cb0
@alinaliBQ
Fix rest of build errors from flightsql-odbc
bf48db0 
- Fix get info errors

- Fix for configuration window

  - added odbcinst library

- Fix system trust store

- unit test fixes

   - Add dependency of ARROW_COMPUTE. `arrow/compute/api.h` is used in
     `flight_sql`. Adding `ARROW_COMPUTE=ON` during build fixed run time unit
     tests failures.
@rscales
Implement SQLAllocConnect and SQLFreeConnect
0851610
@github-actions
Copy link

github-actions bot commented May 6, 2025

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?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

See also:

@alinaliBQ
Trim ODBC fragments
5ec1fdd 
And fix unneeded changes
@alinaliBQ alinaliBQ force-pushed the fix-flightsql-odbc-err branch 6 times, most recently from 1de8b94 to df90149 Compare May 6, 2025 23:50
@alinaliBQ alinaliBQ force-pushed the fix-flightsql-odbc-err branch from df90149 to 3ecdfd4 Compare May 7, 2025 16:50
alinaliBQ
alinaliBQ commented May 7, 2025
View reviewed changes
cpp/submodules/parquet-testing Outdated
Copy link
Collaborator Author

@alinaliBQ alinaliBQ May 7, 2025

Choose a reason for hiding this comment

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

Adding this submodule back, this addresses my comment on apache#40939

@alinaliBQ alinaliBQ force-pushed the fix-flightsql-odbc-err branch 4 times, most recently from c97db94 to dc20315 Compare May 8, 2025 01:10
@alinaliBQ alinaliBQ force-pushed the fix-flightsql-odbc-err branch from dc20315 to dda37df Compare May 8, 2025 01:16
@alinaliBQ alinaliBQ marked this pull request as ready for review May 8, 2025 01:33
@alinaliBQ alinaliBQ force-pushed the fix-flightsql-odbc-err branch 2 times, most recently from 39d63eb to f1e9507 Compare May 9, 2025 18:27
@alinaliBQ alinaliBQ mentioned this pull request May 9, 2025
Merged
alinaliBQ
alinaliBQ commented May 9, 2025
View reviewed changes
Copy link
Collaborator Author

@alinaliBQ alinaliBQ left a comment

Choose a reason for hiding this comment

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

Adding comments to remind myself to make those comments for the community to review.

cpp/build-support/lint_cpp_cli.py Outdated
Comment on lines 79 to 83
Copy link
Collaborator Author

@alinaliBQ alinaliBQ May 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Note to myself: After this PR is merged, I will make comment on apache#40939:

CLI/C++ support is being removed (apache#45810) to allow usage of std::mutex, so adding the lint ignore here

cpp/vcpkg.json Outdated
Comment on lines 18 to +28
Copy link
Collaborator Author

@alinaliBQ alinaliBQ May 9, 2025

Choose a reason for hiding this comment

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

Note to myself: After this PR is merged, I will make comment on apache#40939:

please review the cpp/vcpkg.json changes

cpp/vcpkg.json Outdated
Copy link
Collaborator Author

@alinaliBQ alinaliBQ May 9, 2025

Choose a reason for hiding this comment

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

Note to myself: After this PR is merged, I will make comment on apache#40939:

please review the cpp/vcpkg.json changes

dev/release/rat_exclude_files.txt Outdated
Comment on lines 20 to 23
Copy link
Collaborator Author

@alinaliBQ alinaliBQ May 9, 2025

Choose a reason for hiding this comment

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

Note to myself: After this PR is merged, I will make comment on apache#40939:

  1. Is it ok for whereami.h and whereami.cc to be added to rat exclusion? The whereami library itself is MIT-licensed.
  2. Do file whereami.h and whereami.cc need to be moved to cpp/src/arrow/vendored/*?

cpp/src/arrow/flight/sql/odbc/flight_sql/flight_sql_connection.cc Outdated
Comment on lines +65 to +78
Copy link
Collaborator Author

@alinaliBQ alinaliBQ May 9, 2025

Choose a reason for hiding this comment

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

Note to myself: After this PR is merged, I will make comment on apache#40939:

Using std::string is more suitable in the code base. Is usage of NOLINT ok here?

@alinaliBQ alinaliBQ force-pushed the fix-flightsql-odbc-err branch from f1e9507 to 31c90bf Compare May 9, 2025 23:00
@jbonofre jbonofre merged commit faca7ad into jbonofre:FLIGHT_SQL_ODBC May 10, 2025
18 of 35 checks passed
@alinaliBQ alinaliBQ deleted the fix-flightsql-odbc-err branch May 22, 2025 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

awaiting review Component: C++ Component: FlightRPC

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@alinaliBQ @jbonofre @rscales