lidavidm commented on a change in pull request #11507:
URL: https://github.com/apache/arrow/pull/11507#discussion_r749667718
##########
File path: cpp/src/arrow/flight/flight_sql/CMakeLists.txt
##########
@@ -87,7 +83,7 @@ if(ARROW_BUILD_TESTS OR ARROW_BUILD_EXAMPLES)
add_executable(flight_sql_test_server
server.cc
- sql_info_util.cc
+ sql_info_internal.cc
Review comment:
Hang on. Why are we still linking the library source files into an
executable?
##########
File path: cpp/src/arrow/flight/flight_sql/server.h
##########
@@ -20,24 +20,15 @@
#pragma once
-#include <boost/variant.hpp>
#include <memory>
#include <string>
#include <unordered_map>
-#include "arrow/flight/flight_sql/example/sqlite_statement.h"
-#include "arrow/flight/flight_sql/example/sqlite_statement_batch_reader.h"
#include "arrow/flight/flight_sql/server.h"
-#include "arrow/flight/flight_sql/sql_info_util.h"
+#include "arrow/flight/flight_sql/sql_info_internal.h"
Review comment:
Note that _internal headers will not get installed, so they can't be
part of public headers. If we need the typedefs, we should put them in a common
header.
##########
File path: cpp/src/arrow/flight/flight_sql/sql_info_internal.h
##########
@@ -20,13 +20,17 @@
#pragma once
-#include <boost/variant.hpp>
+#include <arrow/util/variant.h>
Review comment:
nit - please take care to #include arrow with angle brackets.
##########
File path: cpp/src/arrow/flight/flight_sql/CMakeLists.txt
##########
@@ -87,7 +83,7 @@ if(ARROW_BUILD_TESTS OR ARROW_BUILD_EXAMPLES)
add_executable(flight_sql_test_server
server.cc
- sql_info_util.cc
+ sql_info_internal.cc
Review comment:
Even if the files are marked internal, you should still be able to get
at the symbols during linking (you may need to ARROW_EXPORT the
classes/functions/etc.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
