Skip to content

Implement the Go ADBC Interface and build script#865

Open
vishwamartur wants to merge 2 commits into
timeplus-io:developfrom
vishwamartur:add-adbc-support-2
Open

Implement the Go ADBC Interface and build script#865
vishwamartur wants to merge 2 commits into
timeplus-io:developfrom
vishwamartur:add-adbc-support-2

Conversation

@vishwamartur

Copy link
Copy Markdown
  • ADBC Interface

    • Implement the Go ADBC Interface in adbc_driver.go
    • Adhere to the ADBC specifications
  • Build Script

    • Add build.sh to compile the Go ADBC Interface as a C shared object
    • Use the existing SDK for compilation

* **ADBC Interface**
  - Implement the Go ADBC Interface in `adbc_driver.go`
  - Adhere to the ADBC specifications

* **Build Script**
  - Add `build.sh` to compile the Go ADBC Interface as a C shared object
  - Use the existing SDK for compilation
@@ -0,0 +1,65 @@
package main

@zeroshade zeroshade Nov 26, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

this file seems wrong? there's nothing here and it's using the wrong library version

Comment thread src/ADBC/ADBCDriver.h
Comment on lines +3 to +4
#include <adbc.h>
#include <adbc_driver_manager.h>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

include paths should be <arrow-adbc/adbc.h> and <arrow-adbc/adbc_driver_manager.h>

Comment thread src/ADBC/ADBCDriver.cpp
Comment on lines +28 to +29
// Initialize the ADBC driver manager
AdbcStatusCode status = AdbcDriverManagerInitialize();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Where is this function defined? There is no AdbcDriverManagerInitialize() in adbc_driver_manager.h`

Comment thread src/ADBC/ADBCDriver.cpp
// Initialize the ADBC driver manager
AdbcStatusCode status = AdbcDriverManagerInitialize();
if (status != ADBC_STATUS_OK) {
std::cerr << "Failed to initialize ADBC driver manager: " << AdbcStatusCodeToString(status) << std::endl;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

AdbcStatusCodeMessage not AdbcStatusCodeToString

Comment thread src/ADBC/ADBCDriver.cpp
}

// Register the Arrow Flight SQL driver
status = AdbcDriverManagerRegisterDriver("arrow-flight-sql", &arrow_flight_sql_driver);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

this function doesn't exist either, did you mean AdbcLoadDriver?

Also, if you are only using the flightsql driver, why use the driver manager? Why not just link against libadbc_driver_flightsql.so directly?

Comment thread src/ADBC/ADBCDriver.cpp
std::shared_ptr<arrow::Table> ExecuteQuery(const std::string& query) {
// Create a connection to the Arrow Flight SQL server
AdbcConnection connection;
AdbcStatusCode status = AdbcConnectionInitialize(&connection, "arrow-flight-sql", nullptr);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

AdbcConnectionNew and AdbcConnectionInit?

Comment thread src/ADBC/ADBCDriver.cpp

// Execute the query
AdbcStatement statement;
status = AdbcStatementInitialize(&statement, &connection);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

AdbcStatementNew and AdbcStatementInit

Comment thread src/ADBC/ADBCDriver.cpp
AdbcDriverManagerCleanup();
}

std::shared_ptr<arrow::Table> ExecuteQuery(const std::string& query) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You should return an std::shared_ptr<arrow::RecordBatchReader> to stream the results instead

Comment thread src/ADBC/ADBCDriver.cpp
Comment on lines +71 to +84
AdbcResult result;
status = AdbcStatementExecuteQuery(&statement, &result);
if (status != ADBC_STATUS_OK) {
std::cerr << "Failed to execute query: " << AdbcStatusCodeToString(status) << std::endl;
throw std::runtime_error("Failed to execute query");
}

// Convert the result to an Arrow Table
std::shared_ptr<arrow::Table> table;
arrow::Status arrow_status = arrow::flight::sql::ResultToTable(result, &table);
if (!arrow_status.ok()) {
std::cerr << "Failed to convert result to Arrow Table: " << arrow_status.ToString() << std::endl;
throw std::runtime_error("Failed to convert result to Arrow Table");
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

execute query uses an ArrowArrayStream there is no AdbcResult

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants