Skip to content

Conversation

justing-bq
Copy link

Rationale for this change

We want our ODBC code to live in the arrow::flight::sql::odbc namespace.

What changes are included in this PR?

The two utils.cc/h files have been combined into one utils.cc/h and all that logic is put inside a new utils namespace inside arrow::flight::sql::odbc. The driver::flight_sql and driver::odbcabstraction namespaces have both been replaced with arrow::flight::sql::odbc.

Are these changes tested?

N/A

Are there any user-facing changes?

No

Copy link

@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.

The codebase looks much more cleaner now

namespace driver {
namespace flight_sql {
namespace arrow::flight::sql::odbc {
namespace utils {

Choose a reason for hiding this comment

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

I am okay with this change. Just a clarification:

I think I saw something else at standup when I thought Arrow used utils in existing code. I had realized that Arrow currently doesn't have any utils namespace in C++ code. Arrow does use utils namespace from other places, such as Aws::Utils:: etc. Arrow also has classes like CopyDataUtils but that's not a namespace either. Hopefully the community is good with this approach

Choose a reason for hiding this comment

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

Nvm, I see Arrow does have Arrow::util namespace. 🤦‍♀️
Should we use util instead of utils to be consistent?

Copy link
Author

Choose a reason for hiding this comment

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

Switched to util.

Update namespaces driver::flight_sql and driver::odbcabstraction to arrow::flight::sql::odbc.
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.

2 participants