xiangfu0 commented on code in PR #19652:
URL: https://github.com/apache/pinot/pull/19652#discussion_r4091501546
##########
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:
Agreed, the fix had no CI-visible guard. Commit 4d11e01 (not yet pushed)
adds `UdfServiceLoaderTest` in `pinot-query-runtime`.
Rather than only constructing `NotUdf`, it iterates the SPI, because that is
the failure mode the stale lookup actually produces: `ServiceLoader` wraps the
constructor throw in a `ServiceConfigurationError`, which breaks every caller
that iterates `Udf.class` rather than just the provider at fault. Iterating
also covers implementations added later without listing them.
A second test pins why the boxed lookup is required:
`LogicalFunctions.not(Boolean)` exists and the primitive `not(boolean)` the old
lookup asked for does not.
Checked both directions — reintroducing the old lookup fails them with
`ServiceConfigurationError` and `NoSuchMethodException` respectively, and both
pass with the fix.
One correction to my first draft: I had also asserted `getScalarFunction()`
was non-null, which fails. `NotUdf` deliberately returns null there since it is
transform-backed, which `Udf` explicitly permits, so that assertion is gone.
##########
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:
You are right, I missed it — it is the `maxServerResponseSizeBytes` field,
so it did not turn up when I was looking at
`testMaxQueryResponseSizeTableConfig`. Same race at all three of its steps.
Converted to `applyQueryConfigAndAwait` in commit 4d11e01 (not yet pushed).
##########
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:
Correct, and it applied to all four tests, not only the Groovy one — each
reset waited for an outcome that already held under the config it was
replacing, so the wait returned immediately.
Commit 4d11e01 (not yet pushed) restores the restrictive config before
clearing it in each test, so every transition flips the observable outcome: 100
bytes then high then 100 bytes then cleared, and likewise for the server limit
and the 1 ms timeout. Groovy is checked from the enabled state, since the
cluster default disables it exactly like the explicit override does — that was
the case where no reset outcome differs.
I went with observably different transitions rather than inspecting the
broker config, since it needs no new endpoint and asserts the behavior the
tests are about.
--
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]
