Skip to content

Implement SQLGetInfo - #53

Merged
rscales merged 32 commits into
apache-odbcfrom
sql-get-info
Jul 3, 2025
Merged

Implement SQLGetInfo#53
rscales merged 32 commits into
apache-odbcfrom
sql-get-info

Conversation

@rscales

@rscales rscales commented Jun 19, 2025

Copy link
Copy Markdown

Implement SQLGetInfo

@alinaliBQ alinaliBQ left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

These are the comments I have so far. I see some things that we discussed during standup, so I trust you to make those changes and I have skipped the comments on these.

Comment thread cpp/src/arrow/flight/sql/odbc/odbc_api.cc Outdated
Comment thread cpp/src/arrow/flight/sql/odbc/odbc_api.cc Outdated
Comment thread cpp/src/arrow/flight/sql/odbc/odbc_api.cc Outdated
Comment thread cpp/src/arrow/flight/sql/odbc/odbc_api.cc Outdated
Comment thread cpp/src/arrow/flight/sql/odbc/odbc_api.cc Outdated
Comment thread cpp/src/arrow/flight/sql/odbc/tests/connection_info_test.cc Outdated
Comment thread cpp/src/arrow/flight/sql/odbc/tests/connection_info_test.cc Outdated
Comment thread cpp/src/arrow/flight/sql/odbc/tests/connection_info_test.cc Outdated
Comment thread cpp/src/arrow/flight/sql/odbc/tests/connection_info_test.cc Outdated
@alinaliBQ

Copy link
Copy Markdown

JFYI - If you see segfault issues in workflows, it might not be related to your PR, I am fixing it in #54

@alinaliBQ alinaliBQ left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM, will review again after we have the changes for the last 5 attributes

@alinaliBQ alinaliBQ left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

pls fix the merge conflicts and run workflows

Comment thread cpp/src/arrow/flight/sql/odbc/flight_sql/get_info_cache.cc Outdated
Comment thread cpp/src/arrow/flight/sql/odbc/flight_sql/get_info_cache.cc Outdated
Comment thread cpp/src/arrow/flight/sql/odbc/flight_sql/get_info_cache.cc
@rscales
rscales marked this pull request as ready for review June 25, 2025 23:52

@alinaliBQ alinaliBQ left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I have put suggestions for the values that the driver is returning, I found them at GetInfoCache constructor.

Comment thread cpp/src/arrow/flight/sql/odbc/tests/connection_info_test.cc Outdated
Comment thread cpp/src/arrow/flight/sql/odbc/tests/connection_info_test.cc Outdated
Comment thread cpp/src/arrow/flight/sql/odbc/tests/connection_info_test.cc Outdated
Comment thread cpp/src/arrow/flight/sql/odbc/tests/connection_info_test.cc Outdated
Comment thread cpp/src/arrow/flight/sql/odbc/tests/connection_info_test.cc Outdated
Comment thread cpp/src/arrow/flight/sql/odbc/tests/connection_info_test.cc Outdated
Co-authored-by: Alina (Xi) Li <96995091+alinaliBQ@users.noreply.github.com>
Comment thread cpp/src/arrow/flight/sql/odbc/tests/connection_info_test.cc Outdated

@alinaliBQ alinaliBQ left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM

@rscales
rscales merged commit 4bb52aa into apache-odbc Jul 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants