feat: Add Read Support for Presto-MaxCompute Connector #4

dingxin-tech · 2024-05-09T11:33:33Z

This PR introduces the read support for the Presto-MaxCompute Connector, enabling users to perform queries on tables within Alibaba's MaxCompute.
While the current state of the connector does not support write operations, DDL operations, or querying partitioned tables, it is a significant step forward in integrating MaxCompute with Presto.

Key features included in this PR:

Read support for MaxCompute standard tables.
Configuration setup to connect Presto with MaxCompute projects.
Documentation updates for setting up and using the connector.

The following are yet to be implemented in future PRs:

The ability to query partitioned tables.
Write support for MaxCompute tables.
Support for DDL operations within MaxCompute from Presto.

tdcmeehan · 2024-05-09T13:32:37Z

@dingxin-tech thank you for your contribution. Regarding testing: can you describe how to add tests for this plugin? Is it possible to add integration tests that run against a mocked version of MaxCompute, or through some other means?

dingxin-tech · 2024-05-10T02:07:55Z

@tdcmeehan I have no idea how to add integration tests since MaxCompute currently does not provide external testing accounts. Do you know how other similar databases handle this? Maybe I should implement something like a MaxCompute emulator similar to the BigQuery emulator?

tdcmeehan · 2024-05-22T14:59:44Z

@dingxin-tech ideally, Alibaba would give us a testing token to just run some minimal workload. If you could create an emulator, that would be awesome.

dingxin-tech · 2024-05-23T12:19:43Z

@dingxin-tech ideally, Alibaba would give us a testing token to just run some minimal workload. If you could create an emulator, that would be awesome.

Yes, I am currently working on the development of the MaxCompute-Emulator, and I am pleased to report that we have made some significant incremental progress. With the current pace, I anticipate that we will be able to complete support for Presto regression within the next two to three weeks.

tdcmeehan · 2024-06-05T13:01:12Z

@dingxin-tech I see you've added an emulator, is this ready to review?

dingxin-tech · 2024-06-06T07:06:14Z

@dingxin-tech I see you've added an emulator, is this ready to review?

Thank you for your attention.

Yes, I believe this PR is ready for review.

Although some critical features (such as reading the partition table, writing to the table) are not supported yet (in fact, the emulator also does not support these functionalities at the moment), I believe the current functionalities form a good initial whole for the first commit in this repository. These new features can be added in subsequent PRs to further improve the connector.

tdcmeehan

Really great work. Can you please add integration style tests and more unit tests for the various utilities? For example, I think we should have decent unit test coverage over the Arrow to Page conversions. I also think we can add a dependency on presto-tests, and run AbstractTestIntegrationSmokeTest using your new query runner. Can you give it a try?

src/main/java/com/facebook/presto/maxcompute/MaxComputeInputSplit.java

src/main/resources/META-INF/services/com.facebook.presto.spi.Plugin

Co-authored-by: Timothy Meehan <[email protected]>

…rated smoke testing Update dependencies to support the new version of the MaxCompute simulator image, and introduce new test classes to verify basic integration behavior. At the same time, Maven configuration is enhanced to better support the Presto plug-in development process.

dingxin-tech · 2024-08-14T02:26:09Z

I have made changes based on your suggestions:

I removed the configuration from META-INF and added a dependency on the presto-maven-plugin.
I upgraded the emulator and added TestIntegrationSmokeTest.
Regarding the additional tests for ArrowToPage that you mentioned, I currently have no idea on how to determine if a Page meets expectations, but I believe the SmokeTest has already covered that part of the cases.

Could you please review it again? @tdcmeehan

tdcmeehan · 2024-09-12T13:00:54Z

Can you please add a Github action to trigger the tests to run in CI? See here for reference: https://github.com/prestodb/airlift/pull/77/files

elharo

What is our current thinking about adding new connectors to the core repo as opposed to third party repos? Is there anything preventing this from being a separate project?

elharo · 2024-09-12T15:26:18Z

pom.xml

+        <dependency>
+            <groupId>com.google.guava</groupId>
+            <artifactId>guava</artifactId>
+            <version>26.0-jre</version>


can push this to the current version?

elharo · 2024-09-12T15:27:11Z

src/checkstyle/presto-checks.xml

@@ -0,0 +1,222 @@
+<?xml version="1.0" encoding="UTF-8"?>


I'm not sure this file belongs in this PR

maybe we should add it ?

elharo · 2024-09-12T15:29:05Z

src/main/java/com/facebook/presto/maxcompute/utils/ArrowUtils.java

+                }
+                return new SimpleStruct((StructTypeInfo) structTypeInfo, values);
+            }
+            catch (Exception e) {


can you be more specific about the exception?

Sure, I realized that this method doesn't actually throw a checked exception, so I've removed the try-catch block. I'm not sure why I implemented it that way :)

src/main/java/com/facebook/presto/maxcompute/utils/ArrowUtils.java

elharo · 2024-09-12T15:30:09Z

src/main/java/com/facebook/presto/maxcompute/utils/CommonUtils.java

+
+    private CommonUtils() {}
+
+    public static String serialize(Serializable object)


why is object serialization used at all? It's mostly avoided in java these days as it's been a common source of security issues

The serialize and deserialize methods are specifically utilized for serializing and deserializing TableBatchReadSession objects. This is particularly important for MaxComputeSplit, where JSON format is employed for message transmission within Presto. Given that TableBatchReadSession contains numerous variables and has been well-implemented with the Serializable interface, I believe utilizing these methods is a sound approach.

elharo · 2024-09-12T15:30:40Z

src/main/java/com/facebook/presto/maxcompute/utils/CommonUtils.java

+                    throw new IOException("Failed after retries", e);
+                }
+                try {
+                    LOG.warn("Retrying after exception: " + e.getMessage(), e);


Is this actionable? Will anyone read it?

The method was actually not being used, so I've removed it.

tdcmeehan · 2024-09-12T15:55:36Z

What is our current thinking about adding new connectors to the core repo as opposed to third party repos? Is there anything preventing this from being a separate project?

There's nothing preventing this from being a separate project, however we would like to move more of our modules in presto to third party modules, and given that MaxCompute is a widely used and publicly available offering, I'm OK adding this to the Presto org. I also think it will be a good model for other connectors to follow.

steveburnett

Thanks for the documentation! I commented with several nits about phrasing, and a couple of suggestions about formatting. If any of my suggestions don't work for you, I'm happy to discuss them if you like and explain my reasoning.

Also, I suggest you add a new page to the Presto documentation for the new connector, by creating a new file in https://github.com/prestodb/presto/tree/master/presto-docs/src/main/sphinx/connector and adding that new file to the Connectors index file.

README.md

steveburnett · 2024-09-12T17:15:16Z

README.md

+- `analytics.properties` for the analysis project.
+  With this setup, `sales` and `analytics` directories will be created in Presto respectively.
+
+### Data Type Mapping


This Data Type Mapping section would be ideal to move into the connector documentation for this new connector, in a new file in https://github.com/prestodb/presto/tree/master/presto-docs/src/main/sphinx/connector.

- Upgraded `odps.sdk.version` from 0.48.5-public to 0.49.0-public - Upgrade guava version from 26.0-jre to 33.3.1-jre

Co-authored-by: Steve Burnett <[email protected]>

dingxin-tech · 2024-09-27T06:01:35Z

Thanks for the documentation! I commented with several nits about phrasing, and a couple of suggestions about formatting. If any of my suggestions don't work for you, I'm happy to discuss them if you like and explain my reasoning.

Also, I suggest you add a new page to the Presto documentation for the new connector, by creating a new file in https://github.com/prestodb/presto/tree/master/presto-docs/src/main/sphinx/connector and adding that new file to the Connectors index file.

Thank you for your input; all of your suggestions have been incorporated into the changes. I agree that it would be beneficial to add a link to the MaxCompute connector in the Presto documentation repository. However, this should be done in a separate PR, potentially initiated after this current PR has been merged.

dingxin-tech · 2024-09-27T06:10:48Z

Really great work. Can you please add integration style tests and more unit tests for the various utilities? For example, I think we should have decent unit test coverage over the Arrow to Page conversions. I also think we can add a dependency on presto-tests, and run AbstractTestIntegrationSmokeTest using your new query runner. Can you give it a try?

Sure, I've added. :)

steveburnett · 2024-09-27T15:18:16Z

Thank you for your input; all of your suggestions have been incorporated into the changes. I agree that it would be beneficial to add a link to the MaxCompute connector in the Presto documentation repository. However, this should be done in a separate PR, potentially initiated after this current PR has been merged.

Thank you! Some of my comments may have been hidden by GitHub. When you have time, would you please recheck? They may be hidden under "8 hidden conversations".

dingxin-tech added 6 commits May 9, 2024 16:28

init maxcompute-presto

c49f501

init maxcompute-presto

79b5f9f

feat: support array type and add doc

57dadea

doc: fix doc

a4f24ec

style: update gitignore

687ce0b

fix: result error and add some logs

03527e8

tdcmeehan self-assigned this May 9, 2024

dingxin-tech added 2 commits June 5, 2024 16:29

fix: upper case column name result error

60ddab3

test: add maxcompute-emulator and some e2e test

6e9a176

dingxin-tech added 2 commits June 17, 2024 17:10

update version to 0.287

ac00a7e

feat: support read partitioned table

093a999

tdcmeehan requested changes Jul 2, 2024

View reviewed changes

src/main/java/com/facebook/presto/maxcompute/MaxComputeInputSplit.java Outdated Show resolved Hide resolved

src/main/resources/META-INF/services/com.facebook.presto.spi.Plugin Outdated Show resolved Hide resolved

dingxin-tech and others added 2 commits July 9, 2024 19:51

style: apply suggested change

5611895

Co-authored-by: Timothy Meehan <[email protected]>

elharo reviewed Sep 12, 2024

View reviewed changes

steveburnett suggested changes Sep 12, 2024

View reviewed changes

dingxin-tech and others added 5 commits September 27, 2024 11:56

chore: Add Maven build process configuration

af44ed7

chore(pom.xml): update dependency version number

95a6e99

- Upgraded `odps.sdk.version` from 0.48.5-public to 0.49.0-public - Upgrade guava version from 26.0-jre to 33.3.1-jre

refactor(ArrowUtils): remove unused RuntimeException

809d37c

refactor(CommonUtils): remove unused function

3fad27e

Update README.md

6a41ca0

Co-authored-by: Steve Burnett <[email protected]>

dingxin-tech and others added 8 commits September 27, 2024 13:56

Update README.md

5d121cf

Co-authored-by: Steve Burnett <[email protected]>

Update README.md

d062e43

Co-authored-by: Steve Burnett <[email protected]>

Update README.md

d9b22b7

Co-authored-by: Steve Burnett <[email protected]>

Update README.md

814f671

Co-authored-by: Steve Burnett <[email protected]>

Update README.md

8087ffc

Co-authored-by: Steve Burnett <[email protected]>

Update README.md

9142e22

Co-authored-by: Steve Burnett <[email protected]>

Update README.md

b363cb2

Co-authored-by: Steve Burnett <[email protected]>

update readme.md

8fc87b4

update test to adapt new sdk version

efd4fce

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add Read Support for Presto-MaxCompute Connector #4

feat: Add Read Support for Presto-MaxCompute Connector #4

dingxin-tech commented May 9, 2024

tdcmeehan commented May 9, 2024

dingxin-tech commented May 10, 2024

tdcmeehan commented May 22, 2024

dingxin-tech commented May 23, 2024

tdcmeehan commented Jun 5, 2024

dingxin-tech commented Jun 6, 2024

tdcmeehan left a comment

dingxin-tech commented Aug 14, 2024

tdcmeehan commented Sep 12, 2024

elharo left a comment

elharo Sep 12, 2024

dingxin-tech Sep 27, 2024

elharo Sep 12, 2024

dingxin-tech Sep 27, 2024

elharo Sep 12, 2024

dingxin-tech Sep 27, 2024

elharo Sep 12, 2024

dingxin-tech Sep 27, 2024

elharo Sep 12, 2024

dingxin-tech Sep 27, 2024

tdcmeehan commented Sep 12, 2024

steveburnett left a comment

steveburnett Sep 12, 2024

dingxin-tech commented Sep 27, 2024

dingxin-tech commented Sep 27, 2024

steveburnett commented Sep 27, 2024


		private CommonUtils() {}

		public static String serialize(Serializable object)

feat: Add Read Support for Presto-MaxCompute Connector #4

Are you sure you want to change the base?

feat: Add Read Support for Presto-MaxCompute Connector #4

Conversation

dingxin-tech commented May 9, 2024

tdcmeehan commented May 9, 2024

dingxin-tech commented May 10, 2024

tdcmeehan commented May 22, 2024

dingxin-tech commented May 23, 2024

tdcmeehan commented Jun 5, 2024

dingxin-tech commented Jun 6, 2024

tdcmeehan left a comment

Choose a reason for hiding this comment

dingxin-tech commented Aug 14, 2024

tdcmeehan commented Sep 12, 2024

elharo 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

tdcmeehan commented Sep 12, 2024

steveburnett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dingxin-tech commented Sep 27, 2024

dingxin-tech commented Sep 27, 2024

steveburnett commented Sep 27, 2024