Upgrade Pinot Connector to use 0.10.0 libraries

[elonazoulay]

Description

Is this change a fix, improvement, new feature, refactoring, or other?

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

How would you describe this change to a non-technical end user or system administrator?

Related issues, pull requests, and links

Documentation

( ) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
( ) Release notes entries required with the following suggested text:

# Pinot
* Add support for Pinot 0.10. ({issue}`11475`)

[hashhar]

Last commit only since others are reviewed in different PRs.

A question about compatibility.

[hashhar]

Do we have something like a smoke test for older version still? Or do we again plan to drop support?

[elonazoulay]

cc @xiangfu0 - wdyt?

[xiangfu0]

pinot keeps compatible support for the previous minor version upgrade. So typically 0.10.0 clients should work with 0.9.3 and 0.10.0 pinot. 0.8.0 pinot is not guaranteed.

[hashhar]

Thanks.

@elonazoulay Can you add two subclasses to AbstractPinotIntegrationSmokeTest - one that tests 0.9 and other that tests 0.10.

That way we can maintain tests and know the range of versions that the connector works with.
A similar idea like we have in some JDBC connectors.

[elonazoulay]

Sure! Will do.

[hashhar]

Looks good, please squash the commits now.

Would be good to have tests for both 0.9 and 0.10 now because it seems we can support both now.

[hashhar]

trino/plugin/trino-sqlserver/pom.xml

Lines 19 to 25 in 513d193

	<!--
	Project's default for air.test.parallel is 'methods'. By design, 'classes' makes TestNG run tests from one class in a single thread.
	As a side effect, it prevents TestNG from initializing multiple test instances upfront, which happens with 'methods'.
	A potential downside can be long tail single-threaded execution of a single long test class.
	TODO (https://github.com/trinodb/trino/issues/11294) remove when we upgrade to surefire with https://issues.apache.org/jira/browse/SUREFIRE-1967
	-->
	<air.test.parallel>classes</air.test.parallel>

is the canonical fix for now until Surefire is updated.

[elonazoulay]

This worked, thanks! I used instances, classes had the same errors that methods had. Let me know if it's ok to squash the commits.

[hashhar]

Looks good to squash. Thanks.

[hashhar]

Looks good % comment.

I'm not sure if we need 2 versions for the "additional" tests like secured vs without auth. A single test class that exercises most things on 2 versions is sufficient. Rest can be tested with just the older version (since things like auth are generally not version sensitive).

[hashhar]

@ebyhr Do you want to take a look at tests?

[elonazoulay]

Updated, lmk when you have a chance.

[hashhar]

In other connectors the default is the oldest version. I think it makes more sense since generally version updates add things so we should have wider coverage for older versions.

[elonazoulay]

Sounds good, I also renamed TestPinotWithoutAuthenticationIntegrationSmokeTestPreviousVersion to TestPinotWithoutAuthenticationIntegrationSmokeTestLatestVersion.

[hashhar]

LGTM % comment.

Sorry for the delay here.

[hashhar]

Also, please squash the commits now - seems like a single logical change.

[elonazoulay]

Thanks @hashhar, appreciate all the time you spent reviewing this! lmk if the test name change and default version change look good.

[hashhar]

Can you also update pinot.rst to fix the "Requirements" section. It still lists 0.8.0 as the minimum supported version which doesn't work anymore AFAIK? Needs to be bumped to 0.9.3?

[elonazoulay]

Updated - pinot-0.8.0 will work also, but there are a few things which didn't work in pinot-0.8.0 that now work (related to column alias issues) - should I put a note that pinot-0.8.0 will mostly work or just leave it as is?

[hashhar]

Let's leave as it is in that case. Since we don't test 0.8.0 it doesn't make sense to make claims about it. It's kinda implied, everything outside the specified range may or may not work.