Add pinot query options to query #21902

naman-patel · 2024-05-09T21:18:38Z

Description

Currently no Pinot query options can be passed along with query. This means Pinot null handling cant be leveraged. This PR provides a way to provide these query options.

Additional context and related issues

The same PR was also implemented for Presto. Here is the reference to that.

Fixes #21897

Release notes

(x) Release notes are required, with the following suggested text:

# Pinot
* Allow users to configure Pinot query options. ({21897}`issuenumber`)

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/PinotPageSourceProvider.java

.

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/PinotConfig.java

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/PinotPageSourceProvider.java

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/PinotSessionProperties.java

ebyhr · 2024-05-10T01:28:17Z

plugin/trino-pinot/src/test/java/io/trino/plugin/pinot/TestPinotSessionProperties.java

@@ -44,4 +44,18 @@ public void testConnectionTimeoutParsedProperly()
                .build();
        assertThat(PinotSessionProperties.getConnectionTimeout(session)).isEqualTo(new Duration(0.25, TimeUnit.MINUTES));
    }
+


Please add a new assertion to BasePinotConnectorSmokeTest#testQueryOptions with query_options session property.

We can define the variable like below and use it in assertion method.

Session queryOptions = Session.builder(getQueryRunner().getDefaultSession()) .setCatalogSessionProperty("pinot", "query_options", "...") .build();

Please refer to BasePinotConnectorSmokeTest.testAggregationPushdown.

This is connector config. I tried session catalog property. It complains its not a recognized session property.

I can use the above code. How did you try?

Exacty what you have there. It compiles but since that is not catalog session property, I see a runtime exception.

Session queryOptions = Session.builder(getQueryRunner().getDefaultSession()) .setCatalogSessionProperty("pinot", "query_options", "skipUpsert=true") .build(); assertThat(query(queryOptions,"SELECT city, \"sum(long_number)\" FROM" + " \"SET skipUpsert = 'true';" + " SET numReplicaGroupsToQuery = '1';" + " SELECT city, SUM(long_number)" + " FROM my_table" + " GROUP BY city" + " HAVING SUM(long_number) > 10000\"")) .matches("VALUES (VARCHAR 'Los Angeles', DOUBLE '50000.0'), (VARCHAR 'New York', DOUBLE '20000.0')") .isFullyPushedDown();

The session value skipUpsert=true should be skipUpsert:true.

Gentle reminder.

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/query/PinotQueryBuilder.java

plugin/trino-pinot/src/test/java/io/trino/plugin/pinot/TestPinotSessionProperties.java

naman-patel · 2024-05-10T17:44:47Z

@ebyhr addrressed your feedback except for the last one as I dont see a way to add those session properties in test framework.

findinpath · 2024-05-23T15:34:34Z

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/query/PinotQueryBuilder.java

+    public static Optional<String> getQueryOptionsString(String options)
+    {
+        if (isNullOrEmpty(options)) {
+            return Optional.empty();
+        }
+
+        Map<String, String> queryOptionsMap = ImmutableMap.copyOf(MAP_SPLITTER.split(options));
+        return getQueryOptions(queryOptionsMap);
+    }
+
+    public static Optional<String> getQueryOptions(Map<String, String> queryOptionsMap)
+    {
+        if (queryOptionsMap.isEmpty()) {
+            return Optional.empty();
+        }
+        StringBuilder result = new StringBuilder();
+        for (Map.Entry<String, String> entry : queryOptionsMap.entrySet()) {
+            result.append("SET ")
+                    .append(entry.getKey())
+                    .append(" = ")
+                    .append(entry.getValue())
+                    .append(";\n");
+        }
+        return Optional.of(result.toString());
+    }


Use

trino/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHttpMetastoreConfig.java

Lines 83 to 113 in df454db

public Map<String, String> getAdditionalHeaders()

{

return additionalHeaders;

}

@Config("hive.metastore.http.client.additional-headers")

@ConfigDescription("Comma separated key:value pairs to be send to metastore as additional headers")

public ThriftHttpMetastoreConfig setAdditionalHeaders(String httpHeaders)

{

try {

// we allow escaping the delimiters like , and : using back-slash.

// To support that we create a negative lookbehind of , and : which

// are not preceded by a back-slash.

String headersDelim = "(?<!\\\\),";

String kvDelim = "(?<!\\\\):";

Map<String, String> temp = new HashMap<>();

if (httpHeaders != null) {

for (String kv : httpHeaders.split(headersDelim)) {

String key = kv.split(kvDelim, 2)[0].trim();

String val = kv.split(kvDelim, 2)[1].trim();

temp.put(key, val);

}

this.additionalHeaders = ImmutableMap.copyOf(temp);

}

}

catch (IndexOutOfBoundsException e) {

throw new IllegalArgumentException(String.format("Invalid format for 'hive.metastore.http.client.additional-headers'. " +

"Value provided is %s", httpHeaders), e);

}

return this;

}

as PoC on how to deal with a config value containing a map of values.

@findinpath Done.

@naman-patel I was suggesting returning a Map<String, String>

But then everywhere there needs to be code to translate the settings to SET statements. That feels like redundant code everywhere.

This config getter is called from PinotSessionProperties and it expects String type. Returning Optional<String> seems acceptable. We should still validate if the value can be converted to map though.

ebyhr · 2024-05-23T23:36:17Z

Could you rebase on master to resolve conflicts?

naman-patel · 2024-05-24T17:37:08Z

Could you rebase on master to resolve conflicts?

@ebyhr Done

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/query/PinotQueryBuilder.java

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/PinotConfig.java

ebyhr · 2024-05-28T23:08:15Z

Please fix CI failure and add a query based test to BasePinotConnectorSmokeTest

Error:    TestDynamicTable.testQueryOptions:469 
expected: 
  "SET useMultistageEngine = true;
  SET skipUpsert = true;
  SELECT "FlightNum" FROM realtimeOnly_REALTIME LIMIT 50"
 but was: 
  "SET useMultistageEngine = true;
  SET skipUpsert = true;SELECT "FlightNum" FROM realtimeOnly_REALTIME LIMIT 50"

ebyhr · 2024-05-28T23:11:19Z

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/PinotConfig.java

@@ -57,7 +57,6 @@ public class PinotConfig

    private int estimatedSizeInBytesForNonNumericColumn = 20;
    private Duration metadataCacheExpiry = new Duration(2, TimeUnit.MINUTES);
-


Revert unrelated change.

ebyhr · 2024-05-28T23:45:34Z

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/PinotConfig.java

+    @Config("pinot.query-options")
+    @ConfigDescription("Comma separated list of query options. Each option should be in the format key:value. " +
+            "For example, enableNullHandling:true,skipUpsert:true,varcharOption:'value'")
+    public PinotConfig setQueryOptions(String options)
+    {
+        if (options == null) {
+            queryOptions = Optional.empty();
+        }
+        else {
+            queryOptions = Optional.of(options);
+        }
+        return this;
+    }


Move to before validate method.

ebyhr · 2024-05-28T23:46:02Z

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/PinotSessionProperties.java

                        false));
    }

+    public static String getQueryOptions(ConnectorSession session)


Move to the bottom for the same order as fields. Also, please return Optinal<String>. We should avoid returning null as much as possible.

ebyhr · 2024-05-28T23:46:12Z

plugin/trino-pinot/src/test/java/io/trino/plugin/pinot/TestPinotSessionProperties.java

+        Set<String> expected = Arrays.stream("SET enableNullHandling = true;\nSET skipUpsert = true;\nSET varcharOption = 'value';\n"
+                .split("\n")).collect(Collectors.toSet());
+        Set<String> options = Arrays.stream(PinotQueryBuilder.getQueryOptionsString(queryOptions).orElse("").split("\n"))
+                .collect(Collectors.toSet());
+        assertThat(options).isEqualTo(expected);


Remove redundant Arrays.stream method:

assertThat(PinotQueryBuilder.getQueryOptionsString(queryOptions)) .isEqualTo(Optional.of(""" SET enableNullHandling = true; SET skipUpsert = true; SET varcharOption = 'value';"""));

ebyhr · 2024-05-28T23:46:18Z

plugin/trino-pinot/src/test/java/io/trino/plugin/pinot/query/TestQueryOptionsParsing.java

+
+import static org.assertj.core.api.Assertions.assertThat;
+
+public class TestQueryOptionsParsing


Remove public, add final and rename to TestPinotQueryBuilder.

Test classes should be defined as package-private and final.
https://trino.io/docs/current/develop/tests.html#conventions-and-recommendations

ebyhr · 2024-05-28T23:46:22Z

plugin/trino-pinot/src/test/java/io/trino/plugin/pinot/query/TestQueryOptionsParsing.java

+        assertThat(parssedOptions.get("limitForSegmentQueries")).isEqualTo("1000");
+        assertThat(parssedOptions.get("limitForBrokerQueries")).isEqualTo("1000");
+        assertThat(parssedOptions.get("targetSegmentPageSizeBytes")).isEqualTo("1000");


Use containsExactly(entry(...), ... entry(...)) instead. Same for others.

ebyhr · 2024-05-28T23:46:23Z

plugin/trino-pinot/src/test/java/io/trino/plugin/pinot/query/TestQueryOptionsParsing.java

+    {
+        String options = "";
+        Map<String, String> parssedOptions = PinotQueryBuilder.parseQueryOptions(options);
+        assertThat(parssedOptions.isEmpty()).isEqualTo(true);


Use assertThat(parssedOptions).isEmpty(); instead.

ebyhr · 2024-05-28T23:47:26Z

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/query/PinotQueryBuilder.java

+            // are not preceded by a back-slash.
+            String headersDelim = "(?<!\\\\),";
+            String kvDelim = "(?<!\\\\):";
+            Map<String, String> temp = new HashMap<>();


Use ImmutableMap.Builder and rename to queryOptions.

ebyhr · 2024-05-28T23:47:50Z

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/query/PinotQueryBuilder.java

+                String val = kv.split(kvDelim, 2)[1].trim();
+                temp.put(key, val);
+            }
+            return ImmutableMap.copyOf(temp);


ImmutableMap.copyOf is redundant once we change to ImmutableMap.Builder.

ebyhr · 2024-05-28T23:49:04Z

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/query/PinotQueryBuilder.java

+            throw new IllegalArgumentException(String.format("Invalid format for 'pinot.query-options'. " +
+                    "Value provided is %s", options), e);


No need to use String.format in this case: https://github.com/trinodb/trino/blob/master/.github/DEVELOPMENT.md#prefer-string-formatting

cla-bot bot added the cla-signed label May 9, 2024

naman-patel requested review from ebyhr and elonazoulay May 9, 2024 21:47

ebyhr previously requested changes May 9, 2024

View reviewed changes

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/PinotPageSourceProvider.java Outdated Show resolved Hide resolved

naman-patel force-pushed the enable-pinot-query-options branch from 8650993 to fa7aa97 Compare May 10, 2024 01:11

ebyhr reviewed May 10, 2024

View reviewed changes

elonazoulay requested a review from xiangfu0 May 10, 2024 04:39

naman-patel force-pushed the enable-pinot-query-options branch from 5683c06 to 0d5cd9e Compare May 10, 2024 17:13

naman-patel force-pushed the enable-pinot-query-options branch from 42ec6de to 25cc52f Compare May 10, 2024 19:12

findinpath reviewed May 23, 2024

View reviewed changes

naman-patel force-pushed the enable-pinot-query-options branch from dafbc22 to 6917290 Compare May 24, 2024 17:36

naman-patel force-pushed the enable-pinot-query-options branch from 0bee91b to 46ed9af Compare May 24, 2024 20:02

Add pinot query options to query

5c06c66

naman-patel force-pushed the enable-pinot-query-options branch from 46ed9af to 5c06c66 Compare May 24, 2024 20:33

findinpath requested a review from ebyhr May 26, 2024 04:58

findinpath reviewed May 26, 2024

View reviewed changes

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/query/PinotQueryBuilder.java Outdated Show resolved Hide resolved

findinpath reviewed May 26, 2024

View reviewed changes

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/PinotConfig.java Show resolved Hide resolved

github-actions bot added the docs label May 28, 2024

Code review feedback from @findinpath

f88f69f

naman-patel force-pushed the enable-pinot-query-options branch from b457b56 to f88f69f Compare May 28, 2024 22:32

ebyhr reviewed May 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pinot query options to query #21902

Add pinot query options to query #21902

naman-patel commented May 9, 2024 •

edited by ebyhr

ebyhr May 10, 2024

ebyhr May 10, 2024 •

edited

naman-patel May 16, 2024

ebyhr May 16, 2024

naman-patel May 17, 2024 •

edited

ebyhr May 17, 2024

ebyhr May 24, 2024

naman-patel commented May 10, 2024

findinpath May 23, 2024

naman-patel May 24, 2024 •

edited

findinpath May 26, 2024

naman-patel May 28, 2024

ebyhr May 28, 2024

ebyhr commented May 23, 2024

naman-patel commented May 24, 2024 •

edited

ebyhr commented May 28, 2024

ebyhr May 28, 2024

ebyhr May 28, 2024

ebyhr May 28, 2024

ebyhr May 28, 2024

ebyhr May 28, 2024

ebyhr May 28, 2024

ebyhr May 28, 2024

ebyhr May 28, 2024

ebyhr May 28, 2024

ebyhr May 28, 2024

	public Map<String, String> getAdditionalHeaders()
	{
	return additionalHeaders;
	}

	@Config("hive.metastore.http.client.additional-headers")
	@ConfigDescription("Comma separated key:value pairs to be send to metastore as additional headers")
	public ThriftHttpMetastoreConfig setAdditionalHeaders(String httpHeaders)
	{
	try {
	// we allow escaping the delimiters like , and : using back-slash.
	// To support that we create a negative lookbehind of , and : which
	// are not preceded by a back-slash.
	String headersDelim = "(?<!\\\\),";
	String kvDelim = "(?<!\\\\):";
	Map<String, String> temp = new HashMap<>();
	if (httpHeaders != null) {
	for (String kv : httpHeaders.split(headersDelim)) {
	String key = kv.split(kvDelim, 2)[0].trim();
	String val = kv.split(kvDelim, 2)[1].trim();
	temp.put(key, val);
	}
	this.additionalHeaders = ImmutableMap.copyOf(temp);
	}
	}
	catch (IndexOutOfBoundsException e) {
	throw new IllegalArgumentException(String.format("Invalid format for 'hive.metastore.http.client.additional-headers'. " +
	"Value provided is %s", httpHeaders), e);
	}
	return this;
	}

		@@ -57,7 +57,6 @@ public class PinotConfig

		private int estimatedSizeInBytesForNonNumericColumn = 20;
		private Duration metadataCacheExpiry = new Duration(2, TimeUnit.MINUTES);


		import static org.assertj.core.api.Assertions.assertThat;

		public class TestQueryOptionsParsing

		throw new IllegalArgumentException(String.format("Invalid format for 'pinot.query-options'. " +
		"Value provided is %s", options), e);

Add pinot query options to query #21902

Are you sure you want to change the base?

Add pinot query options to query #21902

Conversation

naman-patel commented May 9, 2024 • edited by ebyhr

Description

Additional context and related issues

Release notes

Choose a reason for hiding this comment

ebyhr May 10, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naman-patel May 17, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naman-patel commented May 10, 2024

Choose a reason for hiding this comment

naman-patel May 24, 2024 • edited

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

ebyhr commented May 23, 2024

naman-patel commented May 24, 2024 • edited

ebyhr commented May 28, 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

naman-patel commented May 9, 2024 •

edited by ebyhr

ebyhr May 10, 2024 •

edited

naman-patel May 17, 2024 •

edited

naman-patel May 24, 2024 •

edited

naman-patel commented May 24, 2024 •

edited