Skip to content

DSN window implementation #35

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
Open
wants to merge 1 commit into
base: apache-odbc
Choose a base branch
from
Open

DSN window implementation #35

wants to merge 1 commit into from

Conversation

alinaliBQ
Copy link

DSN window implementation developed according to driver guidelines spec

@alinaliBQ
Copy link
Author

I am setting the PR to merge into sql-driver-connect for easier viewing. When it is ready for review, this PR will be set to merge into apache-odbc.

@alinaliBQ alinaliBQ force-pushed the sql-driver-connect branch from ab159f4 to 2779187 Compare May 23, 2025 17:57
@alinaliBQ alinaliBQ force-pushed the sql-driver-connect-dsn-window branch 3 times, most recently from 2b39bca to 96d283b Compare May 23, 2025 19:17
@alinaliBQ alinaliBQ force-pushed the sql-driver-connect branch from c98f368 to ce4a14e Compare May 23, 2025 22:29
@alinaliBQ alinaliBQ force-pushed the sql-driver-connect-dsn-window branch from 96d283b to d23e867 Compare May 23, 2025 22:31
@alinaliBQ alinaliBQ changed the base branch from sql-driver-connect to apache-odbc May 23, 2025 22:32
@alinaliBQ alinaliBQ force-pushed the sql-driver-connect-dsn-window branch from d23e867 to 5e005da Compare May 23, 2025 22:38
@alinaliBQ alinaliBQ marked this pull request as ready for review May 23, 2025 22:39
jduo
jduo requested changes May 23, 2025
View reviewed changes
cpp/src/arrow/flight/sql/odbc/flight_sql/win_system_dsn.cc Outdated
if (DisplayConnectionWindow(windowParent, config)) {
properties = config.GetProperties();
} else {
throw DriverException("Connection canceled by user", "HY008");
Copy link

Choose a reason for hiding this comment

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

Is HY008 right? That's for query cancellation usually.
Aside from that, I think if the user cancels the connection dialog the driver is supposed to return SQL_NO_DATA or SQL_NEED_DATA rather than an error. Please check this.

Copy link

Choose a reason for hiding this comment

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

Probably should log if there's a cancelled dialog though.

Copy link
Author

Choose a reason for hiding this comment

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

Hmm you're right, HY008 is what Apache Ignite passed (link) but the spec does say that HY008 is for cancelled queries. And the spec does say that SQL_NO_DATA should be returned, there seems to be no error message for canceled dialog actually, so I have removed the throw.

Copy link
Author

Choose a reason for hiding this comment

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

And yes driver logging would be useful. However we haven't enabled logging yet (it is planned for milestone 2), so I have added a todo to add log here after it is enabled.

@alinaliBQ alinaliBQ force-pushed the sql-driver-connect-dsn-window branch 2 times, most recently from f957577 to 56b3ad0 Compare May 24, 2025 00:01
@alinaliBQ alinaliBQ force-pushed the sql-driver-connect-dsn-window branch from 56b3ad0 to 95c0c3d Compare May 24, 2025 00:05
@alinaliBQ alinaliBQ requested a review from jduo May 24, 2025 00:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jduo jduo jduo approved these changes

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
@alinaliBQ @jduo