New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add pinot query options to query #21902
base: master
Are you sure you want to change the base?
Conversation
plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/PinotPageSourceProvider.java
Outdated
Show resolved
Hide resolved
8650993
to
fa7aa97
Compare
plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/PinotConfig.java
Outdated
Show resolved
Hide resolved
plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/PinotConfig.java
Outdated
Show resolved
Hide resolved
plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/PinotPageSourceProvider.java
Outdated
Show resolved
Hide resolved
plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/PinotSessionProperties.java
Outdated
Show resolved
Hide resolved
@@ -44,4 +44,18 @@ public void testConnectionTimeoutParsedProperly() | |||
.build(); | |||
assertThat(PinotSessionProperties.getConnectionTimeout(session)).isEqualTo(new Duration(0.25, TimeUnit.MINUTES)); | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a new assertion to BasePinotConnectorSmokeTest#testQueryOptions
with query_options
session property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is connector config. I tried session catalog property. It complains its not a recognized session property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can use the above code. How did you try?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The session value skipUpsert=true
should be skipUpsert:true
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gentle reminder.
plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/query/PinotQueryBuilder.java
Outdated
Show resolved
Hide resolved
plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/query/PinotQueryBuilder.java
Outdated
Show resolved
Hide resolved
plugin/trino-pinot/src/test/java/io/trino/plugin/pinot/TestPinotSessionProperties.java
Show resolved
Hide resolved
5683c06
to
0d5cd9e
Compare
@ebyhr addrressed your feedback except for the last one as I dont see a way to add those session properties in test framework. |
42ec6de
to
25cc52f
Compare
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()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use
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; | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@findinpath Done.
Could you rebase on master to resolve conflicts? |
dafbc22
to
6917290
Compare
@ebyhr Done |
0bee91b
to
46ed9af
Compare
46ed9af
to
5c06c66
Compare
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: