Skip to content
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.

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

Pass in Python callback to native FlightSQLServer, invoke in GetFlightInfoStatement #492

Merged
merged 19 commits into from
Apr 21, 2024

Conversation

sopzha
Copy link
Collaborator

@sopzha sopzha commented Apr 16, 2024

  • Create in-memory map to store and retrieve query data, using efficient concurrent hash table from libcuckoo
  • Build schema from query result field types

@sopzha sopzha self-assigned this Apr 16, 2024
src/brad/front_end/front_end.py Outdated Show resolved Hide resolved
src/brad/front_end/front_end.py Show resolved Hide resolved
Copy link
Member

@geoffxy geoffxy left a comment

Choose a reason for hiding this comment

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

Thanks @sopzha! I think this PR is almost there. The main thing left is to implement something similar to BradStatement::FetchResult() to actually convert the query result into the Arrow format. I left comments with pointers - please let me know if anything is confusing and I can explain more!

cpp/CMakeLists.txt Outdated Show resolved Hide resolved
cpp/server/brad_server_simple.cc Outdated Show resolved Hide resolved
cpp/server/brad_server_simple.cc Outdated Show resolved Hide resolved
cpp/server/brad_server_simple.cc Outdated Show resolved Hide resolved
cpp/server/brad_server_simple.h Show resolved Hide resolved
cpp/server/brad_statement.cc Outdated Show resolved Hide resolved
cpp/server/brad_server_simple.cc Outdated Show resolved Hide resolved
cpp/server/brad_statement.cc Show resolved Hide resolved
cpp/server/brad_statement_batch_reader.cc Outdated Show resolved Hide resolved
cpp/server/brad_server_simple.cc Show resolved Hide resolved
@sopzha
Copy link
Collaborator Author

sopzha commented Apr 19, 2024

Thank you so much Geoffrey for the thorough review!! Made a first pass thru the comments and will review changes again tomorrow. Feel free to add any additional comments / lmk if I missed anything! I will store RecordBatch in BradStatement in a future PR.

Copy link
Member

@geoffxy geoffxy left a comment

Choose a reason for hiding this comment

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

Thanks @sopzha - this looks great! Please also add in the Arrow conversion for string types and look over the other comments before merging.

cpp/server/brad_server_simple.h Outdated Show resolved Hide resolved
cpp/server/brad_statement.h Outdated Show resolved Hide resolved
cpp/server/brad_statement.h Outdated Show resolved Hide resolved
cpp/server/brad_statement.cc Show resolved Hide resolved
cpp/server/brad_statement.cc Outdated Show resolved Hide resolved
cpp/server/brad_statement.cc Show resolved Hide resolved
@@ -5,8 +5,16 @@
#include <string>

#include <arrow/flight/sql/server.h>
#include "brad_statement.h"
#include <arrow/result.h>

Copy link
Member

Choose a reason for hiding this comment

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

(Another minor thing:) We should still have the includes for atomic and functional (since they are used in this class) 😄. We should add #include <vector> and #include <string> too. My previous comment was referring to removing mutex since we aren't using it in the class anymore.

@sopzha sopzha merged commit 56f18b0 into main Apr 21, 2024
2 checks passed
@sopzha sopzha deleted the arrow-flight-sql branch April 21, 2024 23:43
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