Arrow connector reviewcomment fixes #28

lithinwxd · 2024-11-18T10:46:09Z

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
* ... :pr:`12345`
* ... :pr:`12345`

Hive Connector Changes
* ... :pr:`12345`
* ... :pr:`12345`

If release note is NOT required, use:

== NO RELEASE NOTE ==

…doc update

Use flight descriptor instead of ArrowFlightRequest

elbinpallimalilibm · 2024-11-22T08:23:30Z

.github/workflows/arrow-tests.yml

@@ -0,0 +1,82 @@
+name: arrow tests


can you rename to arrow flight tests?

elbinpallimalilibm · 2024-11-22T08:27:30Z

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

-            FlightInfo flightInfo = clientHandler.getFlightInfo(request, connectorSession);
-            List<Field> fields = flightInfo.getSchema().getFields();
+            Optional<Schema> flightschema = clientHandler.getSchema(flightDescriptor, connectorSession);
+            List<Field> fields = flightschema.map(schema1 -> schema1.getFields()).orElse(Collections.emptyList());


can you use the method reference Schema::getFields instead of schema1 -> schema1.getFields()

elbinpallimalilibm · 2024-11-22T08:28:25Z

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

+            ConnectorHandleResolver handleResolver,
+            ConnectorSplitManager splitManager,
+            ConnectorPageSourceProvider pageSourceProvider,
+


remove this extra line

elbinpallimalilibm · 2024-11-22T08:33:04Z

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

+    {
+    }
+
+    static Block buildBlockFromVector(FieldVector vector, Type type)


can't this be made public?

elbinpallimalilibm · 2024-11-22T08:38:12Z

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

+                        appendValueToBuilder(elementType, elementBuilder, value);
+                    }
+                }
+                arrayBuilder.closeEntry();


what does this do?

closeEntry() ensures that the system knows where one array ends and the next begins.

elbinpallimalilibm · 2024-11-22T08:50:26Z

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

+            if (value instanceof Integer) {
+                type.writeLong(builder, (Integer) value);
+            }
+            else if (value instanceof JsonStringArrayList) {


won't JsonStringArrayList occur outside of ListVectors? Shouldn't we handle that case?

I guess this is needed only for List implemenation may be not required for other primitive vectors

elbinpallimalilibm · 2024-11-22T10:13:24Z

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

+            return;
+        }
+
+        if (type instanceof VarcharType) {


are we handling all the types that we handled in the buildBlockFrom.... methods?

we are handling row types varchar vectors and some of the primitive types . should we handle all ?

won't we get those types in ListVector?

The remaining types are timestamp , date , tiny int , small int etc.., we will add for them also

Remove config getter from flight client handler

elbinpallimalilibm · 2024-11-22T10:44:04Z

presto-base-arrow-flight/src/test/java/com/facebook/plugin/arrow/TestingArrowMetadata.java

+        return FlightDescriptor.command(request.getCommand());
+    }
+
+    private TestingArrowFlightRequest getArrowFlightRequest(ArrowFlightConfig config, String schema)


config is available in class member, no need to pass in method parameters

Is config still needed in this method's parameters

elbinpallimalilibm · 2024-11-22T10:48:22Z

...o-base-arrow-flight/src/test/java/com/facebook/plugin/arrow/TestingConnectionProperties.java

 {
-    public String database;
-    public String password;
+    public final String database;


these can be private, with getters added for them

…doc update

elbinpallimalilibm · 2024-11-29T10:25:01Z

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

+            }
+            else {
+                int dictionaryIndex = encodedVector.get(i);
+                if (dictionary.isNull(dictionaryIndex)) {


can't we use Dictionary and DictionaryEncoder.decode https://arrow.apache.org/docs/java/ipc.html

elbinpallimalilibm · 2024-11-29T12:13:40Z

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

+        throw new UnsupportedOperationException("Unsupported vector type: " + vector.getClass().getSimpleName());
+    }
+
+    public static Block buildBlockFromEncodedVector(FieldVector encodedVector, FieldVector dictionary)


can't we accept Dictionary type itself here.

elbinpallimalilibm · 2024-11-29T12:45:05Z

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

+    public static Block buildBlockFromEncodedVector(FieldVector encodedVector, Dictionary dictionary)
+    {
+        // Validate inputs
+        if (encodedVector == null || dictionary == null) {


you can use requireNonNull method

elbinpallimalilibm · 2024-11-29T12:59:45Z

Do the tests all pass?

elbinpallimalilibm · 2024-11-29T13:34:11Z

presto-base-arrow-flight/pom.xml

+            <dependency>
+                <groupId>org.apache.arrow</groupId>
+                <artifactId>arrow-algorithm</artifactId>
+                <version>18.0.0</version>


can't the arrow version be specified in properties?

this version is specific only to arrow-algorithm , Its used only once . should we move this to properties?

isn't there already an arrow.version property defined, can't we use that? is this version different?

yeah this is 18 , anyways i will change it to 17 and recheck

then why not set all arrow dependencies to 18

okay will change it accordingly to 18.1.0 which is latest , will re run the test cases

elbinpallimalilibm · 2024-11-29T13:43:27Z

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

+            throw new PrestoException(
+                    StandardErrorCode.INVALID_CAST_ARGUMENT,
+                    "Invalid table handle: Expected an instance of ArrowTableHandle but received "
+                            + table.getClass().getSimpleName() + ". Please check the table type being used.");


User doesn't have an option to check table type. Error message doesn't need the string "Please check....."

elbinpallimalilibm · 2024-11-29T13:44:27Z

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

+
+    public void closeRootallocator()
+    {
+        allocator.close();


what if allocator is null

elbinpallimalilibm

added my suggestions

elbinpallimalilibm · 2024-11-29T13:47:22Z

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

+import static java.lang.String.format;
+import static java.util.Objects.requireNonNull;
+
+public class ArrowOutputTableHandle


where is this class used?

This is used for json round test case in TestArrowHandle

why create a class if it's nowhere used in main

Guess this is not needed , will delete it

elbinpallimalilibm · 2024-11-29T13:50:13Z

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

+        }
+    }
+
+    public static void handleTimestampType(Type type, BlockBuilder builder, Object value)


instead of "handle...." name, wouldn't "writeTimestamp...." name be apt? Similarly for other "handle...." methods.

elbinpallimalilibm · 2024-12-06T08:33:49Z

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

+        }
+
+        // Get the Arrow indices vector
+        IntVector indicesVector = (IntVector) fieldVector;


don't we need to check type before casting?

elbinpallimalilibm · 2024-12-06T08:36:21Z

presto-base-arrow-flight/src/test/java/com/facebook/plugin/arrow/ArrowPageUtilsTest.java

+
+        // Encode using dictionary
+        Dictionary dictionary = new Dictionary(dictionaryVector, new DictionaryEncoding(1L, false, null));
+        IntVector encodedVector = (IntVector) DictionaryEncoder.encode(rawVector, dictionary);


will DictionaryEncoder.encode always return IntVector

added different testcases

DictionaryEncoder.encode will return a BaseIntVector

elbinpallimalilibm · 2024-12-06T13:00:59Z

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

+            BigIntVector bigIntIndicesVector = (BigIntVector) fieldVector;
+            int[] ids = new int[bigIntIndicesVector.getValueCount()];
+            for (int i = 0; i < bigIntIndicesVector.getValueCount(); i++) {
+                ids[i] = (int) bigIntIndicesVector.get(i);


down casting big int to int will result in data loss, right? so better we don't support creating dictionaryblock from bigint indices

yeah okay will remove that

elbinpallimalilibm · 2024-12-09T07:01:00Z

presto-base-arrow-flight/src/test/java/com/facebook/plugin/arrow/ArrowPageUtilsTest.java

+        rawVector.setValueCount(VECTOR_LENGTH);
+
+        // Encode using dictionary
+        Dictionary dictionary = new Dictionary(dictionaryVector, new DictionaryEncoding(1L, false, null));


you should mention the index type when dictionary encoding is created.

* Arrow Flight connector template Co-authored-by: sai bhaskar reddy <[email protected]> Co-authored-by: SthuthiGhosh9400 <[email protected]> Co-authored-by: lithinwxd <[email protected]> Co-authored-by: Steve Burnett <[email protected]> Co-authored-by: elbinpallimalilibm <[email protected]> * Move Arrow type to Presto type mapping to a separate function * Fix checkstyle errors * Refactor null check * Remove explicit versios * Remove explicit versions * Remove explicit versions in pom (#25) * Remove explicit versios * Remove explicit versions * Remove explicit declarations in pom * Resolve upper bound error * Do not exclude netty dependencies * Remove explicit versions from pom * Add integration smoke test * Use flight descriptor instead of ArrowFlightRequest * Remove config getter from flight client handler * Arrow connector reviewcomment fixes (#28) * Arrow CI job * added verison in property file * testing after removing hyphen * remove path to make sure CI is run during every build * changed yaml to run CI on evry pull requests and every update except doc update * review comment fixes * Use flight descriptor instead of ArrowFlightRequest * Arrow page utils changes * Use flight descriptor instead of ArrowFlightRequest * Arrow page utils changes - fixed checkstyle issues * Review comment fixes - Root allocator and typos * Remove config getter from flight client handler * Arrow CI job * added verison in property file * testing after removing hyphen * remove path to make sure CI is run during every build * changed yaml to run CI on evry pull requests and every update except doc update * review comment fixes * Arrow page utils changes * Arrow page utils changes - fixed checkstyle issues * Review comment fixes - Root allocator and typos * Review comment fixes * Review comment fixes * Review comment fixes - Changed config * Added support for small int tiny int date and timestamp * Review comment fixes - Dictionary encoding and other tests * Review comment fixes - Added Tests as per comments * Removed license header unwanted place * Removed duplicate CI job * Review comment fixes * Added more testcases * Fixed review comments and added support for other datatypes * Minor fixes on method argument * Fixed review comments - cosmetic changes and unneccessary class removal * Arrow - DictionaryEncoding usecase * Removed description which looks like generated * Added support for dictionary encoding * Fixed review comments * Fixed review comments -added index type --------- Co-authored-by: Elbin Pallimalil <[email protected]> Co-authored-by: Elbin Pallimalil <[email protected]> --------- Co-authored-by: sai bhaskar reddy <[email protected]> Co-authored-by: SthuthiGhosh9400 <[email protected]> Co-authored-by: lithinwxd <[email protected]> Co-authored-by: Steve Burnett <[email protected]>

elbinpallimalilibm · 2024-12-09T09:57:44Z

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

@@ -3,7 +3,6 @@
 Arrow Flight Connector
 ======================
 This connector allows querying multiple data sources that are supported by an Arrow Flight server.
-Apache Arrow enhances performance and efficiency in data-intensive applications through its columnar memory layout, zero-copy reads, vectorized execution, cross-language interoperability, rich data type support, and optimization for modern hardware. These features collectively reduce overhead, improve data processing speeds, and facilitate seamless data exchange between different systems and languages.


Can you a give a brief overview and link to the official documentation.

elbinpallimalilibm · 2024-12-09T10:32:20Z

...-base-arrow-flight/src/test/java/com/facebook/plugin/arrow/TestingInteractionProperties.java

@@ -15,15 +15,26 @@

 import com.fasterxml.jackson.annotation.JsonProperty;

-public class TestingInteractionProperties
+public final class TestingInteractionProperties


can you use the Immutable annotation

elbinpallimalilibm · 2024-12-09T11:27:02Z

presto-base-arrow-flight/src/test/java/com/facebook/plugin/arrow/TestingArrowServer.java

@@ -222,7 +223,7 @@ else if (selectStatement != null) {
    @Override
    public Runnable acceptPut(CallContext callContext, FlightStream flightStream, StreamListener<PutResult> streamListener)
    {
-        return null;
+        throw new UnsupportedOperationException("This operation is not supported");
    }


listActions should also throw unsupported operation exception

elbinpallimalilibm · 2024-12-09T11:40:31Z

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

+            throw new PrestoException(
+                    StandardErrorCode.INVALID_CAST_ARGUMENT,
+                    "Invalid table handle: Expected an instance of ArrowTableHandle but received "
+                            + table.getClass().getSimpleName() + "");


no need to append empty string

* Arrow Flight connector template Co-authored-by: sai bhaskar reddy <[email protected]> Co-authored-by: SthuthiGhosh9400 <[email protected]> Co-authored-by: lithinwxd <[email protected]> Co-authored-by: Steve Burnett <[email protected]> Co-authored-by: elbinpallimalilibm <[email protected]> Co-authored-by: Steve Burnett <[email protected]> Co-authored-by: Timothy Meehan <[email protected]> Co-authored-by: Timothy Meehan <[email protected]> Co-authored-by: Timothy Meehan <[email protected]> * Arrow Flight connector template Co-authored-by: sai bhaskar reddy <[email protected]> Co-authored-by: SthuthiGhosh9400 <[email protected]> Co-authored-by: lithinwxd <[email protected]> Co-authored-by: Steve Burnett <[email protected]> Co-authored-by: elbinpallimalilibm <[email protected]> * Move Arrow type to Presto type mapping to a separate function * Fix checkstyle errors * Refactor null check * Remove explicit versios * Remove explicit versions * Remove explicit versions in pom (#25) * Remove explicit versios * Remove explicit versions * Remove explicit declarations in pom * Resolve upper bound error * Do not exclude netty dependencies * Remove explicit versions from pom * Add integration smoke test * Use flight descriptor instead of ArrowFlightRequest * Remove config getter from flight client handler * Arrow connector reviewcomment fixes (#28) * Arrow CI job * added verison in property file * testing after removing hyphen * remove path to make sure CI is run during every build * changed yaml to run CI on evry pull requests and every update except doc update * review comment fixes * Use flight descriptor instead of ArrowFlightRequest * Arrow page utils changes * Use flight descriptor instead of ArrowFlightRequest * Arrow page utils changes - fixed checkstyle issues * Review comment fixes - Root allocator and typos * Remove config getter from flight client handler * Arrow CI job * added verison in property file * testing after removing hyphen * remove path to make sure CI is run during every build * changed yaml to run CI on evry pull requests and every update except doc update * review comment fixes * Arrow page utils changes * Arrow page utils changes - fixed checkstyle issues * Review comment fixes - Root allocator and typos * Review comment fixes * Review comment fixes * Review comment fixes - Changed config * Added support for small int tiny int date and timestamp * Review comment fixes - Dictionary encoding and other tests * Review comment fixes - Added Tests as per comments * Removed license header unwanted place * Removed duplicate CI job * Review comment fixes * Added more testcases * Fixed review comments and added support for other datatypes * Minor fixes on method argument * Fixed review comments - cosmetic changes and unneccessary class removal * Arrow - DictionaryEncoding usecase * Removed description which looks like generated * Added support for dictionary encoding * Fixed review comments * Fixed review comments -added index type --------- Co-authored-by: Elbin Pallimalil <[email protected]> Co-authored-by: Elbin Pallimalil <[email protected]> * Arrow connector reviewcomment fixes elbin (#32) * Arrow CI job * added verison in property file * testing after removing hyphen * remove path to make sure CI is run during every build * changed yaml to run CI on evry pull requests and every update except doc update * review comment fixes * Arrow page utils changes * Use flight descriptor instead of ArrowFlightRequest * Arrow page utils changes - fixed checkstyle issues * Review comment fixes - Root allocator and typos * Arrow CI job * added verison in property file * testing after removing hyphen * remove path to make sure CI is run during every build * changed yaml to run CI on evry pull requests and every update except doc update * review comment fixes * Arrow page utils changes * Arrow page utils changes - fixed checkstyle issues * Review comment fixes - Root allocator and typos * Review comment fixes * Review comment fixes * Review comment fixes - Changed config * Added support for small int tiny int date and timestamp * Review comment fixes - Dictionary encoding and other tests * Review comment fixes - Added Tests as per comments * Removed license header unwanted place * Removed duplicate CI job * Review comment fixes * Added more testcases * Fixed review comments and added support for other datatypes * Minor fixes on method argument * Fixed review comments - cosmetic changes and unneccessary class removal * Arrow - DictionaryEncoding usecase * Removed description which looks like generated * Added support for dictionary encoding * Fixed review comments * Fixed review comments -added index type * Added documentation link for arrow connector * Made TestingInteractionProperties immutable * Made TestingInteractionProperties immutable --------- Co-authored-by: lithinpurushothaman <[email protected]> * Update ArrowPageUtilsTest.java * Update base-arrow-flight.rst --------- Co-authored-by: sai bhaskar reddy <[email protected]> Co-authored-by: SthuthiGhosh9400 <[email protected]> Co-authored-by: lithinwxd <[email protected]> Co-authored-by: Steve Burnett <[email protected]> Co-authored-by: Timothy Meehan <[email protected]>

lithinwxd and others added 12 commits November 18, 2024 13:55

Arrow CI job

d6c9e0d

added verison in property file

2b758d8

testing after removing hyphen

84d4d89

remove path to make sure CI is run during every build

153312e

changed yaml to run CI on evry pull requests and every update except …

a597913

…doc update

review comment fixes

d466a13

Use flight descriptor instead of ArrowFlightRequest

ada5038

Use flight descriptor instead of ArrowFlightRequest

a5d11c4

Use flight descriptor instead of ArrowFlightRequest

Arrow page utils changes

d3b8063

Use flight descriptor instead of ArrowFlightRequest

72e2f07

Arrow page utils changes - fixed checkstyle issues

780f0a3

Review comment fixes - Root allocator and typos

a2cb5c5

lithinwxd requested a review from elbinpallimalilibm November 22, 2024 07:57

elbinpallimalilibm reviewed Nov 22, 2024

View reviewed changes

elbinpallimalilibm and others added 2 commits November 22, 2024 16:12

Remove config getter from flight client handler

bc25a2e

Merge pull request #30 from sabbasani/arrow-connector-workspace-elbin

40776ec

Remove config getter from flight client handler

elbinpallimalilibm reviewed Nov 22, 2024

View reviewed changes

lithinwxd added 6 commits November 22, 2024 16:19

Arrow CI job

0b06577

added verison in property file

bcc101b

testing after removing hyphen

9615f4c

remove path to make sure CI is run during every build

e8250f0

changed yaml to run CI on evry pull requests and every update except …

064b047

…doc update

review comment fixes

99d6a8f

elbinpallimalilibm reviewed Nov 29, 2024

View reviewed changes

Fixed review comments and added support for other datatypes

a32e967

elbinpallimalilibm reviewed Nov 29, 2024

View reviewed changes

Minor fixes on method argument

d42cf78

elbinpallimalilibm reviewed Nov 29, 2024

View reviewed changes

elbinpallimalilibm approved these changes Nov 29, 2024

View reviewed changes

elbinpallimalilibm reviewed Nov 29, 2024

View reviewed changes

elbinpallimalilibm requested changes Nov 29, 2024

View reviewed changes

elbinpallimalilibm reviewed Nov 29, 2024

View reviewed changes

lithinwxd added 3 commits November 29, 2024 20:17

Fixed review comments - cosmetic changes and unneccessary class removal

a27652a

Arrow - DictionaryEncoding usecase

4fb97d7

Removed description which looks like generated

0138aa7

elbinpallimalilibm reviewed Dec 6, 2024

View reviewed changes

Added support for dictionary encoding

b13eeb3

elbinpallimalilibm reviewed Dec 6, 2024

View reviewed changes

Fixed review comments

e2463e7

elbinpallimalilibm reviewed Dec 9, 2024

View reviewed changes

Fixed review comments -added index type

d391906

elbinpallimalilibm merged commit 15e1025 into arrow-connector-workspace Dec 9, 2024
52 of 53 checks passed

elbinpallimalilibm reviewed Dec 9, 2024

View reviewed changes

Arrow connector reviewcomment fixes #28

Arrow connector reviewcomment fixes #28

Conversation

lithinwxd commented Nov 18, 2024

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lithinwxd Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elbinpallimalilibm commented Nov 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elbinpallimalilibm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lithinwxd Nov 22, 2024 •

edited

Loading