feat(plugin-pinot): Upgrade Pinot version to 1.3.0 & move off of presto-pinot-driver #25785

infvg · 2025-08-14T09:36:43Z

Description

If implemented this will upgrade the Pinot API version to 1.3.0. It will also move off of the shaded presto-pinot-driver dependency.

Motivation and Context

We are currently on an older version of pinot

Impact

No impact

Test Plan

Tested with a local pinot server & UTs

Contributor checklist

Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
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 ==
Pinot Connector Changes
* Upgrade Pinot version to 1.3.0

imjalpreet

@infvg, thank you for the upgrade!

I did a first pass and have a few suggestions.

imjalpreet · 2025-10-09T08:43:55Z

presto-main-base/pom.xml

        <dependency>
            <groupId>org.apache.lucene</groupId>
            <artifactId>lucene-analyzers-common</artifactId>
+            <version>8.11.3</version>


nit: let's add a property tag for this version

Sure, added the property tag here since we're overriding to 8.11.3 just for presto-main-base since it's building with java 8

imjalpreet · 2025-10-09T08:54:09Z

presto-pinot-toolkit/src/main/java/com/facebook/presto/pinot/PinotSegmentPageSource.java

                markDataFetchExceptionsAsRetriable ? PINOT_DATA_FETCH_EXCEPTION : PINOT_EXCEPTION,
                split.getSegmentPinotQuery(),
-                String.format("Encountered %d pinot exceptions for split %s: %s", exceptions.size(), split, exceptions));
+                String.format("Encountered %d pinot exceptions for split %s: %s", exceptions.size(), split, dataTable.getExceptions()));


nit: Should we just use the exceptions list here since we already created an object earlier?

imjalpreet · 2025-10-09T08:56:35Z

presto-pinot-toolkit/src/main/java/com/facebook/presto/pinot/PinotStreamingQueryClient.java

+/**
+ * Grpc based Pinot query client.
+ */


Let's document the reason why this class is introduced. If it's a fork of a Pinot class, comment why we needed a fork? I see that earlier it was coming from the Pinot dependency.

So, is there no alternative that they have introduced after removing it?

imjalpreet · 2025-10-09T08:59:13Z

presto-pinot-toolkit/src/main/java/com/facebook/presto/pinot/PinotUtils.java

+    public static ByteString toByteString(byte[] bytes)
+    {
+        return ByteString.copyFrom(bytes);
+    }


Is this only required for tests?

yep, moved to the test & made private

imjalpreet · 2025-10-09T08:59:27Z

presto-pinot-toolkit/src/main/java/com/facebook/presto/pinot/PinotUtils.java

+    public static InstanceConfig toInstanceConfig(String instanceId)
+    {
+        return InstanceConfig.toInstanceConfig(instanceId);
+    }


Is this being used anywhere?

No, removed

imjalpreet · 2025-10-09T09:07:31Z

presto-pinot-toolkit/pom.xml

+        <dependency>
+            <groupId>org.apache.helix</groupId>
+            <artifactId>helix-core</artifactId>
+            <version>1.4.3</version>


nit: let's add a property tag for this

imjalpreet · 2025-10-09T09:09:19Z

presto-pinot-toolkit/pom.xml

+                <exclusion>
+                    <groupId>commons-lang</groupId>
+                    <artifactId>commons-lang</artifactId>
+                </exclusion>


Just wanted to understand the reason for this exclusion.

Removed the exclusion

imjalpreet · 2025-10-09T09:12:20Z

presto-pinot-toolkit/pom.xml

Just to clarify, are all the exclusions made because those dependencies are already being brought in from other sources and were causing conflicts, or is there another reason?

Yep, there's a lot of dependency conflicts & some versions were causing runtime issues so I had to find the right dependency to pull in for each.

imjalpreet · 2025-10-09T09:14:37Z

presto-pinot/pom.xml

+            <dependency>
+                <groupId>org.apache.datasketches</groupId>
+                <artifactId>datasketches-memory</artifactId>
+                <version>3.0.2</version>
+            </dependency>
+            <dependency>
+                <groupId>org.apache.datasketches</groupId>
+                <artifactId>datasketches-java</artifactId>
+                <version>6.1.1</version>
+            </dependency>
+            <dependency>
+                <groupId>jakarta.xml.bind</groupId>
+                <artifactId>jakarta.xml.bind-api</artifactId>
+                <version>4.0.2</version>
+            </dependency>
+            <dependency>
+                <groupId>org.roaringbitmap</groupId>
+                <artifactId>RoaringBitmap</artifactId>
+                <version>1.3.0</version>
+            </dependency>
+            <dependency>
+                <groupId>it.unimi.dsi</groupId>
+                <artifactId>fastutil</artifactId>
+                <version>8.5.15</version>
+            </dependency>


nit: let's add property tags

These dependencies versions are being overridden from the master pom but they don't have property tags in the root pom. Should we just add tags to avoid repetition?

Are you suggesting adding tags in the root POM and overriding just the tags in this POM? If yes, let's do that.

imjalpreet · 2025-10-09T09:17:15Z

pom.xml

nit: please add property tags for all versions we are changing in this PR

Should we add property tags for dependencies even if their version isn't dependent or repeated?

No, as we discussed offline, if it's one-off usage, we don't need to add the version tag.

imjalpreet

Thanks, @infvg. I have a few more comments.

imjalpreet · 2025-10-16T23:29:20Z

presto-pinot/pom.xml

+            <dependency>
+                <groupId>org.apache.datasketches</groupId>
+                <artifactId>datasketches-memory</artifactId>
+                <version>3.0.2</version>
+            </dependency>
+            <dependency>
+                <groupId>org.apache.datasketches</groupId>
+                <artifactId>datasketches-java</artifactId>
+                <version>6.1.1</version>
+            </dependency>
+            <dependency>
+                <groupId>jakarta.xml.bind</groupId>
+                <artifactId>jakarta.xml.bind-api</artifactId>
+                <version>4.0.2</version>
+            </dependency>
+            <dependency>
+                <groupId>org.roaringbitmap</groupId>
+                <artifactId>RoaringBitmap</artifactId>
+                <version>1.3.0</version>
+            </dependency>
+            <dependency>
+                <groupId>it.unimi.dsi</groupId>
+                <artifactId>fastutil</artifactId>
+                <version>8.5.15</version>
+            </dependency>


Are you suggesting adding tags in the root POM and overriding just the tags in this POM? If yes, let's do that.

imjalpreet · 2025-10-16T23:31:03Z

presto-pinot-toolkit/src/main/java/com/facebook/presto/pinot/PinotStreamingQueryClient.java

+/**
+ * Grpc based Pinot query client.
+ */


So, is there no alternative that they have introduced after removing it?

imjalpreet · 2025-10-16T23:33:30Z

presto-pinot-toolkit/pom.xml

+        <!-- This overrides bsh version despite the groupId being changed-->
+        <dependency>
+            <groupId>org.apache-extras.beanshell</groupId>
+            <artifactId>bsh</artifactId>
+            <version>2.0b6</version>
+        </dependency>


Just for my understanding, can you elaborate a bit on the comment?

Modified it so it's clearer

infvg · 2025-10-19T13:33:51Z

@imjalpreet for the PinotStreamingQueryClient, this was a part of the presto-pinot-toolkit which was built with JDK8 & uses the jdk 8 version of the pinot dependencies. The latest pinot libraries don't include the jdk 8 version & the presto driver was removed (probably due to the JDK8 versions being removed). My PR brings in the code from the driver, along with its jdk17 equivalent dependencies.

For the overrides, none of the dependencies have versions that are reused or really used anywhere else so IMO we shouldn't add tags for those (I removed RoaringBitmap since it was no longer necessary to override).

imjalpreet

@infvg Thanks, overall looks good to me, but I have one suggestion.

For dependencies already defined in the root POM’s <dependencyManagement>, instead of re-adding the entire dependency in a submodule's <dependencyManagement> just to override the version, it’s cleaner to define a property in the root POM and override that property in the submodule.

For example, in the root POM, you could add:

<dep.datasketches.version>2.2.0</dep.datasketches.version>

<dependency>
    <groupId>org.apache.datasketches</groupId>
    <artifactId>datasketches-memory</artifactId>
    <version>${dep.datasketches.version}</version>
</dependency>

Then, in Pinot, simply override the property:

<dep.datasketches.version>3.0.2</dep.datasketches.version>

If implemented this will upgrade the Pinot API version to 1.3.0. It will also move off of the shaded presto-pinot-driver dependency.

infvg · 2025-11-02T07:50:04Z

@imjalpreet added the dependency properties

imjalpreet

Thank you, @infvg. LGTM.

tdcmeehan

Thanks a lot @infvg

… of presto-pinot-driver (prestodb#25785)" This reverts commit 267f823.

prestodb-ci added the from:IBM PR from IBM label Aug 14, 2025

infvg force-pushed the pinot-toolkit-upgrade branch from 184ce68 to 5ceabb5 Compare August 14, 2025 10:39

infvg marked this pull request as ready for review August 14, 2025 10:42

infvg requested a review from a team as a code owner August 14, 2025 10:42

prestodb-ci requested review from a team, Joe-Abraham and namya28 and removed request for a team August 14, 2025 10:42

infvg force-pushed the pinot-toolkit-upgrade branch from 5ceabb5 to 348918e Compare August 18, 2025 10:11

infvg requested a review from pratyakshsharma August 18, 2025 12:52

infvg force-pushed the pinot-toolkit-upgrade branch 4 times, most recently from d2ae4ce to 6112114 Compare August 25, 2025 05:40

infvg force-pushed the pinot-toolkit-upgrade branch 3 times, most recently from a5302ee to 8cdb81f Compare August 31, 2025 07:28

infvg requested a review from imjalpreet August 31, 2025 08:48

imjalpreet mentioned this pull request Aug 31, 2025

Upgrade helix-core to 1.4.3 to address CVE-2023-38647 #25812

Closed

6 tasks

infvg force-pushed the pinot-toolkit-upgrade branch 2 times, most recently from 9805a1b to b7207d3 Compare September 1, 2025 10:55

imjalpreet requested changes Oct 9, 2025

View reviewed changes

infvg force-pushed the pinot-toolkit-upgrade branch from b7207d3 to a81a7db Compare October 12, 2025 09:08

infvg changed the title ~~Upgrade Pinot version to 1.3.0 & move off of presto-pinot-driver~~ feat(pinot): Upgrade Pinot version to 1.3.0 & move off of presto-pinot-driver Oct 12, 2025

infvg force-pushed the pinot-toolkit-upgrade branch 2 times, most recently from b6d5218 to 93f9ee2 Compare October 13, 2025 08:58

tdcmeehan self-assigned this Oct 13, 2025

infvg force-pushed the pinot-toolkit-upgrade branch 2 times, most recently from 2c0213a to 4e10d70 Compare October 16, 2025 11:07

imjalpreet reviewed Oct 16, 2025

View reviewed changes

infvg force-pushed the pinot-toolkit-upgrade branch from 4e10d70 to fc45e84 Compare October 19, 2025 13:34

infvg changed the title ~~feat(pinot): Upgrade Pinot version to 1.3.0 & move off of presto-pinot-driver~~ feat(connector): Upgrade Pinot version to 1.3.0 & move off of presto-pinot-driver Oct 19, 2025

infvg requested a review from imjalpreet October 20, 2025 09:14

tdcmeehan changed the title ~~feat(connector): Upgrade Pinot version to 1.3.0 & move off of presto-pinot-driver~~ feat(plugin-pinot): Upgrade Pinot version to 1.3.0 & move off of presto-pinot-driver Oct 21, 2025

imjalpreet reviewed Oct 30, 2025

View reviewed changes

Upgrade Pinot version to 1.3.0 & move off of presto-pinot-driver

efb085a

If implemented this will upgrade the Pinot API version to 1.3.0. It will also move off of the shaded presto-pinot-driver dependency.

infvg force-pushed the pinot-toolkit-upgrade branch from fc45e84 to efb085a Compare November 2, 2025 07:48

imjalpreet approved these changes Nov 3, 2025

View reviewed changes

imjalpreet requested a review from tdcmeehan November 3, 2025 14:14

tdcmeehan approved these changes Nov 3, 2025

View reviewed changes

imjalpreet merged commit 267f823 into prestodb:master Nov 3, 2025
81 of 82 checks passed

tdcmeehan added a commit to tdcmeehan/presto that referenced this pull request Nov 4, 2025

Revert "feat(plugin-pinot): Upgrade Pinot version to 1.3.0 & move off…

a76af92

… of presto-pinot-driver (prestodb#25785)" This reverts commit 267f823.

tdcmeehan mentioned this pull request Nov 4, 2025

[DNR] Revert "feat(plugin-pinot): Upgrade Pinot version to 1.3.0 & move off… tdcmeehan/presto#13

Closed

7 tasks

tdcmeehan added a commit to tdcmeehan/presto that referenced this pull request Nov 5, 2025

Revert "feat(plugin-pinot): Upgrade Pinot version to 1.3.0 & move off…

5bdd3ac

… of presto-pinot-driver (prestodb#25785)" This reverts commit 267f823.

unidevel mentioned this pull request Nov 14, 2025

Add release notes for 0.296 unix280/presto#47

Merged

15 tasks

This was referenced Nov 24, 2025

Add release notes for 0.296 unix280/presto#48

Merged

Add release notes for 0.296 unix280/presto#49

Merged

feat(plugin-pinot): Upgrade Pinot version to 1.3.0 & move off of presto-pinot-driver #25785

feat(plugin-pinot): Upgrade Pinot version to 1.3.0 & move off of presto-pinot-driver #25785

Conversation

infvg commented Aug 14, 2025 • 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!

imjalpreet 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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imjalpreet 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!

infvg commented Oct 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

imjalpreet left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

infvg commented Nov 2, 2025

Uh oh!

imjalpreet left a comment

infvg commented Aug 14, 2025 •

edited

Loading

infvg commented Oct 19, 2025 •

edited

Loading

imjalpreet left a comment •

edited

Loading