Jackie-Jiang commented on code in PR #19652:
URL: https://github.com/apache/pinot/pull/19652#discussion_r4090150397
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/function/NotUdf.java:
##########
@@ -40,7 +40,7 @@ public class NotUdf extends Udf.FromAnnotatedMethod {
public NotUdf()
throws NoSuchMethodException {
- super(LogicalFunctions.class.getMethod("not", boolean.class));
+ super(LogicalFunctions.class.getMethod("not", Boolean.class));
Review Comment:
[MAJOR, C6.3] This fixes a real constructor failure, but the new lane
excludes UdfTest, the only test that loads Udf.class. No selected test
constructs NotUdf, so this regression remains invisible to CI. Please add a
focused test that constructs NotUdf and checks its signature; it would fail on
the base commit and pass here.
##########
pinot-integration-tests/pom.xml:
##########
@@ -445,6 +323,25 @@
<exclude>org/apache/pinot/integration/tests/RealtimeConsumptionRateLimiterClusterIntegrationTest.java</exclude>
</excludes>
</configuration>
+ <executions>
+ <execution>
+ <id>logical-table-integration-test-suite</id>
+ <phase>test</phase>
+ <goals>
+ <goal>test</goal>
+ </goals>
+ <configuration>
+
<skipTests>${pinot.integration.test.skip.named.suites}</skipTests>
+ <reportNameSuffix>logical-table</reportNameSuffix>
+ <excludes combine.self="override">
+ <exclude>none</exclude>
+ </excludes>
+ <includes combine.self="override">
+
<include>org/apache/pinot/integration/tests/suites/LogicalTableSuite.java</include>
Review Comment:
[MAJOR] This suite now runs testMaxServerResponseSizeTableConfig, which
still updates the logical-table config and immediately queries at each of its
three steps. The broker reads the limit from its asynchronously updated
logical-table cache, so the first query can see no limit and the second can see
the prior 1000-byte limit. Please use the new applyQueryConfigAndAwait helper
in that test too.
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/logicaltable/BaseLogicalTableIntegrationTest.java:
##########
@@ -528,65 +548,58 @@ private void setProtoSegmentList(boolean enabled)
@Test
public void testDisableGroovyQueryTableConfigOverride()
throws Exception {
- QueryConfig queryConfig = new QueryConfig(null, false, null, null, null,
null);
LogicalTableConfig logicalTableConfig =
getLogicalTableConfig(getLogicalTableName());
- logicalTableConfig.setQueryConfig(queryConfig);
- updateLogicalTableConfig(logicalTableConfig);
-
String groovyQuery = "SELECT
GROOVY('{\"returnType\":\"STRING\",\"isSingleValue\":true}', "
+ "'arg0 + arg1', FlightNum, Origin) FROM mytable";
- // Query should not throw exception
- postQuery(groovyQuery);
-
- // Disable groovy explicitly
- queryConfig = new QueryConfig(null, true, null, null, null, null);
-
- logicalTableConfig.setQueryConfig(queryConfig);
- updateLogicalTableConfig(logicalTableConfig);
+ // Enable groovy for this logical table: the query must stop failing.
+ applyQueryConfigAndAwait(logicalTableConfig, new QueryConfig(null, false,
null, null, null, null),
+ () -> {
+ postQuery(groovyQuery);
+ return true;
+ }, "Groovy query kept failing after groovy was enabled");
- // grpc and http throw different exceptions. So only check error message.
- Exception athrows = expectThrows(Exception.class, () ->
postQuery(groovyQuery));
- assertTrue(athrows.getMessage().contains("Groovy transform functions are
disabled for queries"));
+ // Disable groovy explicitly.
+ applyQueryConfigAndAwait(logicalTableConfig, new QueryConfig(null, true,
null, null, null, null),
+ () -> failsWithGroovyDisabled(groovyQuery), "Groovy query kept
succeeding after groovy was disabled");
- // Remove query config
- logicalTableConfig.setQueryConfig(null);
- updateLogicalTableConfig(logicalTableConfig);
+ // Removing the query config falls back to the cluster default, which also
disables groovy.
Review Comment:
[MINOR, C6.5] This reset wait can return before the broker applies the
removal: Groovy is disabled under both the preceding explicit config and the
default. The response-size and timeout reset predicates likewise match their
preceding high-limit configs. Please make each reset transition observably
different or inspect the broker's applied config.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
