Arrow-flight connector by sabbasani · Pull Request #23032 · prestodb/presto

sabbasani · 2024-06-19T11:23:38Z

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

General Changes
* Add Arrow Flight connector :pr:`23032`
* Add documentation for the :doc:`/connector/base-arrow-flight`  :pr:`23032`

If release note is NOT required, use:

== NO RELEASE NOTE ==

steveburnett · 2024-06-19T14:22:18Z

Consider adding documentation for the new connector.

Suggest revising the release note entry to follow the Release Note Guidelines:

== RELEASE NOTES ==

General Changes
* Add Arrow Flight connector :pr:`23032`

tdcmeehan

It seems like there's a hardcoded presumption that the underlying datasource accepts a SQL query. Can you remove this from the PR? The service may not accept SQL.

tdcmeehan · 2024-07-16T14:53:03Z

presto-base-arrow-flight/src/main/java/com/facebook/plugin/arrow/ArrowAbstractMetadata.java

+    public List<Field> getColumnsList(String schema, String table, ConnectorSession connectorSession)
+    {
+        try {
+            String dbSpecificSchemaName = getDBSpecificSchemaName(config, schema);


Avoid "DB" references as it might not be an underlying DB

I have removed "DB" references

tdcmeehan · 2024-07-16T14:53:44Z

presto-base-arrow-flight/src/main/java/com/facebook/plugin/arrow/ArrowTypeHandle.java

+            @JsonProperty("jdbcType") int jdbcType,
+            @JsonProperty("jdbcTypeName") String jdbcTypeName,


We can't have references to JDBC in this connector.

I have removed JDBC re references

steveburnett · 2024-07-16T17:11:46Z

Suggest change to the release note entry as follows:

== RELEASE NOTES ==

General Changes
* Add Arrow Flight connector :pr:`23032`

The documentation for Arrow Flight Connector appears to be being added in #23212 , so it doesn't need to be mentioned in this release note.

BryanCutler

I took a quick pass and had a few comments. I'll take another look at the rest a little later.

BryanCutler · 2024-07-17T00:44:34Z

presto-base-arrow-flight/pom.xml

+        <air.main.basedir>${project.parent.basedir}</air.main.basedir>
+        <grpc.version>1.53.0</grpc.version>
+        <dep.okhttp.version>4.10.0</dep.okhttp.version>
+        <arrow.version>11.0.0</arrow.version>


This is a fairly old version of Arrow, can you use a more recent one?

@BryanCutler I have updated to leastest arrow version

presto-base-arrow-flight/pom.xml

presto-base-arrow-flight/src/main/java/com/facebook/plugin/arrow/ArrowColumnHandle.java

BryanCutler · 2024-07-17T01:14:12Z

presto-base-arrow-flight/src/main/java/com/facebook/plugin/arrow/ArrowFlightClientHandler.java

+            return;
+        }
+        try {
+            RootAllocator allocator = new RootAllocator(Long.MAX_VALUE);


You would usually create a RootAllocator as a class member and they should be closed when not used anymore

@BryanCutler I have made changes for closing allocator

We are not re-using the flight client. The root allocator will be closed when ArrowFlightClient is closed or auto closed.

where is this RootAllocator closed then? The FlightClient creates a child allocator that is closed along with the client, it will not close the root

ArrowFlightClient saves the root allocator. The root allocator is closed when ArrowFlightClient is closed.

You wouldn't normally create a RootAllocator for each client, it could be reused as each client will internally make a child allocator. It's ok to clean that up later, since you are closing it here.

How would we determine when the root allocator should be closed? Should we keep track of open flight clients and close root allocator when there are no flight clients open for some duration of time?

I would create 1 RootAllocator for the connector and then close it during Connector.shutdown(). The FlightClient creates it's own child allocator that is closed with the client, so the root won't be keeping any buffers directly.

presto-base-arrow-flight/src/main/java/com/facebook/plugin/arrow/ArrowFlightClientHandler.java

BryanCutler · 2024-07-17T01:21:10Z

presto-base-arrow-flight/src/main/java/com/facebook/plugin/arrow/ArrowFlightClientHandler.java

+            trustedCertificate.get().close();
+        }
+        shutdownTimer();
+        isClientClosed.set(true);


Do the calls to getClient() and close() need to be thread-safe?

@BryanCutler I have addressed comments can please review changes

linux-foundation-easycla · 2024-07-17T06:10:26Z

The committers listed above are authorized under a signed CLA.

✅ login: sabbasani (2bcce6b)

steveburnett

I reviewed the base-arrow-flight.rst documentation in #23212. If this documentation is to be included in this PR - which I think would be a good idea to do this - please address my comments in my most recent review in #23212 here.

tdcmeehan · 2024-07-24T20:15:28Z

presto-base-arrow-flight/src/main/java/com/facebook/plugin/arrow/ArrowQueryBuilder.java

+import static java.util.Objects.requireNonNull;
+import static java.util.stream.Collectors.joining;
+
+public class ArrowQueryBuilder


This might better belong in a submodule that depends on this module which implements the Flight SQL spec. I don't think it belongs here.

@tdcmeehan I have addressed comments can please review changes

sabbasani · 2024-07-25T21:12:41Z

@tdcmeehan @BryanCutler @steveburnett
we have addressed all comments above.Can you please review

steveburnett

Thanks for the docs! Minor suggestions, mostly formatting.

A local doc build returns the following warning:

/Users/steveburnett/Documents/GitHub/presto/presto-docs/src/main/sphinx/connector/base-arrow-flight.rst: WARNING: document isn't included in any toctree

To address this warning,

Add connector/base-arrow-flight to
https://github.com/prestodb/presto/blob/master/presto-docs/src/main/sphinx/connector.rst so the new page is present on the Connector page in the Presto documentation.
Add the new line in alphabetic order for the new page title Arrow-flight Connector.

presto-docs/src/main/sphinx/connector/base-arrow-flight.rst

steveburnett · 2024-07-29T16:12:08Z

Suggest update of the release note to include the PR number in both entries, and to link to the new doc from the release note.

== RELEASE NOTES ==

General Changes
* Add Arrow Flight connector :pr:`23032`
* Add documentation for the :doc:`/connector/base-arrow-flight`  :pr:`23032`

steveburnett

Thanks for including the doc from #23212 here! A few minor suggestions, looks good.

presto-docs/src/main/sphinx/connector/base-arrow-flight.rst

elbinpallimalilibm · 2025-01-04T01:54:11Z

presto-docs/src/main/sphinx/connector/base-arrow-flight.rst

+---------------------------------------------------------------------------------
+To create a plugin extending the base-arrow-module, you need to implement certain abstract methods that are specific to your use case. Below are the required classes and their purposes:
+
+* ``AbstractArrowFlightClientHandler.java``


@lithinwxd Can you update these instructions to match the latest code changes?

elbinpallimalilibm · 2025-01-06T04:34:19Z

presto-docs/src/main/sphinx/connector/base-arrow-flight.rst

+---------------------------------------------------------------------------------
+To create a plugin extending the presto-base-arrow-flight module, you need to implement certain abstract methods that are specific to your use case. Below are the required classes and their purposes:
+
+- ``BaseArrowFlightClient.java``


@steveburnett We have updated the documentation to match the latest code base. Can you review?

steveburnett

LGTM! (docs)

Pull updated branch, new local doc build, looks good. Thanks!

BryanCutler · 2025-01-06T19:08:08Z

presto-base-arrow-flight/src/main/java/com/facebook/plugin/arrow/ArrowBlockBuilder.java

+            }
+            else {
+                String value = new String(vector.get(i), StandardCharsets.UTF_8);
+                type.writeSlice(builder, Slices.utf8Slice(value));


Can you create a Slice directly from the raw UTF-8 bytes from vector.get(i) using Slice wrappedBuffer(byte... array)? That would be more efficient that copying to a String first.

BryanCutler · 2025-01-06T19:16:43Z

presto-base-arrow-flight/src/main/java/com/facebook/plugin/arrow/ArrowBlockBuilder.java

+                        elementBuilder.appendNull();
+                    }
+                    else {
+                        appendValueToBuilder(elementType, elementBuilder, value);


Can you just call buildBlockFromValueVector with the element vector instead of making a new function to append Objects?

buildBlockFromValueVector gives a block, but here we want to append multiple values to the same block.

There is a lot of additional code to deal with writing elements and it's poor performance to call reader.readObject() for each element of the list.

I think what you could do is make BlockBuilder an argument instead of creating it in each function, then build() after calling the function

void buildBlockFromValueVector(ValueVector vector, Type type, BlockBuilder builder)

then in buildBlockFromListVector you would call this function with the element vector and elementBuilder, for example

buildBlockFromValueVector(vector.getDataVector(), elementType, elementBuilder);

vector.getDataVector() gives all the values from the list, not just the element vector. Are you aware of a way to get the element vector.

You can look here for an example of accessing elements within a ListVector https://github.com/apache/spark/blob/master/sql/catalyst/src/main/java/org/apache/spark/sql/vectorized/ArrowColumnVector.java#L525

BryanCutler · 2025-01-06T19:28:13Z

presto-base-arrow-flight/src/main/java/com/facebook/plugin/arrow/ClientClosingFlightStream.java

+
+import static java.util.Objects.requireNonNull;
+
+public class ClientClosingFlightStream


I don't think this class is necessary. You shouldn't lump a client and stream together, just keep them separate.

@tdcmeehan is it okay for you to decouple client and stream ?

Since we need to close the client and stream together, it's convenient for encapsulation to put this in one type (otherwise the user of the BaseFlightClient needs to supply a client, which needs to be closed separately). What about a more generic AutoCloseableSupplier or something along those lines?

I made this comment because the Flight Server dictates how many streams a requested flight will have, and it could be multiple. The client would normally process all streams, closing each one independently. I guess if we are assuming there will always be 1 stream only for the given client, this is ok. But if that ever changes or the client needs to perform some additional actions, then this wouldn't be useful.

This remains true. Because streams are opened on a per-split basis, the concurrency is being managed by the Presto evaluation engine, and we can safely make the presumption that there will be 1 client per split, 1 stream per client per split.

Co-authored-by: sai bhaskar reddy <sai.bhaskar.reddy.sabbasani1@ibm.com> Co-authored-by: SthuthiGhosh9400 <Sthuthi.Ghosh@ibm.com> Co-authored-by: lithinwxd <Lithin.Purushothaman@ibm.com> Co-authored-by: Steve Burnett <burnett@pobox.com> Co-authored-by: elbinpallimalilibm <elbin.pallimalil@ibm.com> Co-authored-by: Tim Meehan <tim@timdmeehan.com>

ethanyzhang · 2025-01-08T19:16:48Z

Hi @sabbasani @elbinpallimalilibm based on the discussion we had with Shweta and Sandhya, we are gonna take over this work, @BryanCutler will help move this PR over the finishing line. We keep everyone who worked on this PR as co-authors. Thank you. cc @tdcmeehan

elbinpallimalilibm · 2025-01-17T13:01:59Z

presto-base-arrow-flight/src/main/java/com/facebook/plugin/arrow/ArrowTableHandle.java

+            @JsonProperty("schema") String schema,
+            @JsonProperty("table") String table)
+    {
+        this.schema = schema;


@BryanCutler We are testing table valued functions with an extension of this connector and there is a use case where the PTVF won't have a table and schema associated with it. So we need to consider such a use case when designing the table handle for this connector.

Could you explain this use case more? I don't quite understand how a TableHandle would not have a table.

DM'd you the details.

Ajas-Mangal · 2025-01-20T04:48:08Z

presto-base-arrow-flight/src/main/java/com/facebook/plugin/arrow/ArrowTableHandle.java

+            @JsonProperty("schema") String schema,
+            @JsonProperty("table") String table)
+    {
+        this.schema = schema;


Use Optional and provide appropriate constructor argument validation for schema and table.

Eg:
private final Optional <String> schema;
and
this. schema = requireNonNull(schema, "schema is null");

To reiterate, the ArrowConnector implementation controls the value of schema and table here, so it could set them to a unique identifier even if they are not necessarily needed as you say. I'm not sure it's correct to make these properties optional, and I don't see that done in any other connector. @tdcmeehan can correct me if I'm mistaken.

BryanCutler · 2025-01-24T19:19:17Z

Continuing this PR at #24427

BryanCutler · 2025-01-28T04:03:46Z

presto-base-arrow-flight/pom.xml

+            <dependency>
+                <groupId>io.perfmark</groupId>
+                <artifactId>perfmark-api</artifactId>
+                <version>${perfmark-api.version}</version>


@elbinpallimalilibm the managed dependencies for perfmark-api and error_prone_annotations don't seem to be needed anymore, was there a specific reason they were added?

We were seeing upper bound errors on build without mentioning the dependencies in the POM. Is that error not happening now?

Not seeing any errors now, looks like error prone version has been upgraded in root pom

BryanCutler · 2025-01-29T22:06:56Z

#24427 has been merged, will close this one

sabbasani requested a review from a team as a code owner June 19, 2024 11:23

sabbasani requested a review from presto-oss June 19, 2024 11:23

sabbasani marked this pull request as draft June 19, 2024 11:42

tdcmeehan self-assigned this Jul 15, 2024

tdcmeehan requested changes Jul 16, 2024

View reviewed changes

tdcmeehan requested a review from BryanCutler July 16, 2024 14:55

BryanCutler reviewed Jul 17, 2024

View reviewed changes

steveburnett requested changes Jul 17, 2024

View reviewed changes

sabbasani force-pushed the arrow-connector branch 4 times, most recently from a69b92e to 7bd4ec9 Compare July 24, 2024 13:38

tdcmeehan reviewed Jul 24, 2024

View reviewed changes

sabbasani force-pushed the arrow-connector branch 8 times, most recently from 04cdc53 to 824175b Compare July 25, 2024 21:05

tdcmeehan requested a review from BryanCutler July 26, 2024 14:11

sabbasani force-pushed the arrow-connector branch from 6d9d75b to 11bada5 Compare July 29, 2024 13:16

steveburnett requested changes Jul 29, 2024

View reviewed changes

sabbasani force-pushed the arrow-connector branch from 3b73ee3 to c4e77f1 Compare July 29, 2024 17:28

steveburnett requested changes Jan 3, 2025

View reviewed changes

presto-docs/src/main/sphinx/connector/base-arrow-flight.rst Outdated Show resolved Hide resolved

presto-docs/src/main/sphinx/connector/base-arrow-flight.rst Outdated Show resolved Hide resolved

presto-docs/src/main/sphinx/connector/base-arrow-flight.rst Outdated Show resolved Hide resolved

elbinpallimalilibm force-pushed the arrow-connector branch from a2ed31a to 734fa2b Compare January 4, 2025 01:50

elbinpallimalilibm reviewed Jan 4, 2025

View reviewed changes

elbinpallimalilibm force-pushed the arrow-connector branch 5 times, most recently from 3cb87b6 to ed3d407 Compare January 4, 2025 16:45

elbinpallimalilibm reviewed Jan 6, 2025

View reviewed changes

elbinpallimalilibm force-pushed the arrow-connector branch 3 times, most recently from f763a93 to 3dcded8 Compare January 6, 2025 06:29

steveburnett previously approved these changes Jan 6, 2025

View reviewed changes

BryanCutler reviewed Jan 6, 2025

View reviewed changes

elbinpallimalilibm dismissed steveburnett’s stale review via 925f2c4 January 8, 2025 08:07

elbinpallimalilibm force-pushed the arrow-connector branch 5 times, most recently from dcf2b14 to 034c5d7 Compare January 8, 2025 13:53

elbinpallimalilibm force-pushed the arrow-connector branch from 034c5d7 to 2bcce6b Compare January 8, 2025 14:30

elbinpallimalilibm reviewed Jan 17, 2025

View reviewed changes

Ajas-Mangal suggested changes Jan 20, 2025

View reviewed changes

BryanCutler mentioned this pull request Jan 24, 2025

Arrow Flight Connector Template #24427

Merged

6 tasks

BryanCutler reviewed Jan 28, 2025

View reviewed changes

BryanCutler closed this Jan 29, 2025

		@JsonProperty("jdbcType") int jdbcType,
		@JsonProperty("jdbcTypeName") String jdbcTypeName,


		import static java.util.Objects.requireNonNull;

		public class ClientClosingFlightStream

Conversation

sabbasani commented Jun 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

Uh oh!

steveburnett commented Jun 19, 2024

Uh oh!

tdcmeehan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steveburnett commented Jul 16, 2024

Uh oh!

BryanCutler left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

linux-foundation-easycla bot commented Jul 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

steveburnett left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sabbasani commented Jul 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

steveburnett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

steveburnett commented Jul 29, 2024

Uh oh!

steveburnett left a comment

Choose a reason for hiding this comment

Uh oh!

sabbasani commented Jun 19, 2024 •

edited

Loading

linux-foundation-easycla bot commented Jul 17, 2024 •

edited

Loading

sabbasani commented Jul 25, 2024 •

edited

Loading