Skip to content

Commit b12b7bb

Browse files
author
Tim Santos
authored
Adding controller config for disabling Groovy in ingestionConfig (#8169)
Introduced new config for disabling Groovy in ingestionConfig: `controller.disable.ingestion.groovy`. If not defined, defaults to false
1 parent 13b4533 commit b12b7bb

File tree

6 files changed

+63
-13
lines changed

6 files changed

+63
-13
lines changed

‎pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -251,6 +251,7 @@ private static long getRandomInitialDelayInSeconds() {
251251
private static final String DEFAULT_DIM_TABLE_MAX_SIZE = "200M";
252252

253253
private static final String DEFAULT_PINOT_FS_FACTORY_CLASS_LOCAL = LocalPinotFS.class.getName();
254+
public static final String DISABLE_GROOVY = "controller.disable.ingestion.groovy";
254255

255256
public ControllerConf() {
256257
super(new HashMap<>());
@@ -841,6 +842,13 @@ public String getControllerResourcePackages() {
841842
return getProperty(CONTROLLER_RESOURCE_PACKAGES, DEFAULT_CONTROLLER_RESOURCE_PACKAGES);
842843
}
843844

845+
/**
846+
* @return true if Groovy functions are disabled in controller config, otherwise returns false.
847+
*/
848+
public boolean isDisableIngestionGroovy() {
849+
return getProperty(DISABLE_GROOVY, false);
850+
}
851+
844852
private long convertPeriodToUnit(String period, TimeUnit timeUnitToConvertTo) {
845853
return timeUnitToConvertTo.convert(TimeUtils.convertPeriodToMillis(period), TimeUnit.MILLISECONDS);
846854
}

‎pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -175,7 +175,7 @@ public SuccessResponse addTable(
175175
TableConfigTunerUtils.applyTunerConfigs(_pinotHelixResourceManager, tableConfig, schema, Collections.emptyMap());
176176

177177
// TableConfigUtils.validate(...) is used across table create/update.
178-
TableConfigUtils.validate(tableConfig, schema, typesToSkip);
178+
TableConfigUtils.validate(tableConfig, schema, typesToSkip, _controllerConf.isDisableIngestionGroovy());
179179
// TableConfigUtils.validateTableName(...) checks table name rules.
180180
// So it won't effect already created tables.
181181
TableConfigUtils.validateTableName(tableConfig);
@@ -446,7 +446,7 @@ public SuccessResponse updateTableConfig(
446446
try {
447447
tableConfig = JsonUtils.stringToObject(tableConfigString, TableConfig.class);
448448
Schema schema = _pinotHelixResourceManager.getSchemaForTableConfig(tableConfig);
449-
TableConfigUtils.validate(tableConfig, schema, typesToSkip);
449+
TableConfigUtils.validate(tableConfig, schema, typesToSkip, _controllerConf.isDisableIngestionGroovy());
450450
} catch (Exception e) {
451451
String msg = String.format("Invalid table config: %s with error: %s", tableName, e.getMessage());
452452
throw new ControllerApplicationException(LOGGER, msg, Response.Status.BAD_REQUEST, e);
@@ -533,7 +533,7 @@ private String validateConfig(TableConfig tableConfig, Schema schema, @Nullable
533533
if (schema == null) {
534534
throw new SchemaNotFoundException("Got empty schema");
535535
}
536-
TableConfigUtils.validate(tableConfig, schema, typesToSkip);
536+
TableConfigUtils.validate(tableConfig, schema, typesToSkip, _controllerConf.isDisableIngestionGroovy());
537537
ObjectNode tableConfigValidateStr = JsonUtils.newObjectNode();
538538
if (tableConfig.getTableType() == TableType.OFFLINE) {
539539
tableConfigValidateStr.set(TableType.OFFLINE.name(), tableConfig.toJsonNode());

‎pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/TableConfigsRestletResource.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -293,6 +293,7 @@ public SuccessResponse updateConfig(
293293
tableConfigs = JsonUtils.stringToObject(tableConfigsStr, TableConfigs.class);
294294
Preconditions.checkState(tableConfigs.getTableName().equals(tableName),
295295
"'tableName' in TableConfigs: %s must match provided tableName: %s", tableConfigs.getTableName(), tableName);
296+
296297
validateConfig(tableConfigs, typesToSkip);
297298
} catch (Exception e) {
298299
throw new ControllerApplicationException(LOGGER, String.format("Invalid TableConfigs: %s", tableName),
@@ -396,14 +397,14 @@ private String validateConfig(TableConfigs tableConfigs, @Nullable String typesT
396397
Preconditions.checkState(offlineRawTableName.equals(rawTableName),
397398
"Name in 'offline' table config: %s must be equal to 'tableName': %s", offlineRawTableName, rawTableName);
398399
TableConfigUtils.validateTableName(offlineTableConfig);
399-
TableConfigUtils.validate(offlineTableConfig, schema, typesToSkip);
400+
TableConfigUtils.validate(offlineTableConfig, schema, typesToSkip, _controllerConf.isDisableIngestionGroovy());
400401
}
401402
if (realtimeTableConfig != null) {
402403
String realtimeRawTableName = TableNameBuilder.extractRawTableName(realtimeTableConfig.getTableName());
403404
Preconditions.checkState(realtimeRawTableName.equals(rawTableName),
404405
"Name in 'realtime' table config: %s must be equal to 'tableName': %s", realtimeRawTableName, rawTableName);
405406
TableConfigUtils.validateTableName(realtimeTableConfig);
406-
TableConfigUtils.validate(realtimeTableConfig, schema, typesToSkip);
407+
TableConfigUtils.validate(realtimeTableConfig, schema, typesToSkip, _controllerConf.isDisableIngestionGroovy());
407408
}
408409
if (offlineTableConfig != null && realtimeTableConfig != null) {
409410
TableConfigUtils.verifyHybridTableConfigs(rawTableName, offlineTableConfig, realtimeTableConfig);

‎pinot-segment-local/src/main/java/org/apache/pinot/segment/local/function/FunctionEvaluatorFactory.java‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,13 +97,20 @@ public static FunctionEvaluator getExpressionEvaluator(FieldSpec fieldSpec) {
9797
}
9898

9999
public static FunctionEvaluator getExpressionEvaluator(String transformExpression) {
100-
if (transformExpression.startsWith(GroovyFunctionEvaluator.getGroovyExpressionPrefix())) {
100+
if (isGroovyExpression(transformExpression)) {
101101
return new GroovyFunctionEvaluator(transformExpression);
102102
} else {
103103
return new InbuiltFunctionEvaluator(transformExpression);
104104
}
105105
}
106106

107+
/**
108+
* @return true if the given transform function is a groovy expression, otherwise returns false
109+
*/
110+
public static boolean isGroovyExpression(String transformExpression) {
111+
return transformExpression.startsWith(GroovyFunctionEvaluator.getGroovyExpressionPrefix());
112+
}
113+
107114
private static String getDefaultMapKeysTransformExpression(String mapColumnName) {
108115
return String.format("Groovy({%s.sort()*.key}, %s)", mapColumnName, mapColumnName);
109116
}

‎pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java‎

Lines changed: 21 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -90,10 +90,10 @@ private TableConfigUtils() {
9090
private static final String KINESIS_STREAM_TYPE = "kinesis";
9191

9292
/**
93-
* @see TableConfigUtils#validate(TableConfig, Schema, String)
93+
* @see TableConfigUtils#validate(TableConfig, Schema, String, boolean)
9494
*/
9595
public static void validate(TableConfig tableConfig, @Nullable Schema schema) {
96-
validate(tableConfig, schema, null);
96+
validate(tableConfig, schema, null, false);
9797
}
9898

9999
/**
@@ -106,7 +106,8 @@ public static void validate(TableConfig tableConfig, @Nullable Schema schema) {
106106
*
107107
* TODO: Add more validations for each section (e.g. validate conditions are met for aggregateMetrics)
108108
*/
109-
public static void validate(TableConfig tableConfig, @Nullable Schema schema, @Nullable String typesToSkip) {
109+
public static void validate(TableConfig tableConfig, @Nullable Schema schema, @Nullable String typesToSkip,
110+
boolean disableGroovy) {
110111
Set<ValidationType> skipTypes = parseTypesToSkipString(typesToSkip);
111112
if (tableConfig.getTableType() == TableType.REALTIME) {
112113
Preconditions.checkState(schema != null, "Schema should not be null for REALTIME table");
@@ -116,7 +117,7 @@ public static void validate(TableConfig tableConfig, @Nullable Schema schema, @N
116117
// skip all validation if skip type ALL is selected.
117118
if (!skipTypes.contains(ValidationType.ALL)) {
118119
validateValidationConfig(tableConfig, schema);
119-
validateIngestionConfig(tableConfig, schema);
120+
validateIngestionConfig(tableConfig, schema, disableGroovy);
120121
validateTierConfigList(tableConfig.getTierConfigsList());
121122
validateIndexingConfig(tableConfig.getIndexingConfig(), schema);
122123
validateFieldConfigList(tableConfig.getFieldConfigList(), tableConfig.getIndexingConfig(), schema);
@@ -234,6 +235,11 @@ private static void validateValidationConfig(TableConfig tableConfig, @Nullable
234235
validateRetentionConfig(tableConfig);
235236
}
236237

238+
@VisibleForTesting
239+
public static void validateIngestionConfig(TableConfig tableConfig, @Nullable Schema schema) {
240+
validateIngestionConfig(tableConfig, schema, false);
241+
}
242+
237243
/**
238244
* Validates the following:
239245
* 1. validity of filter function
@@ -244,7 +250,7 @@ private static void validateValidationConfig(TableConfig tableConfig, @Nullable
244250
* 6. ingestion type for dimension tables
245251
*/
246252
@VisibleForTesting
247-
public static void validateIngestionConfig(TableConfig tableConfig, @Nullable Schema schema) {
253+
public static void validateIngestionConfig(TableConfig tableConfig, @Nullable Schema schema, boolean disableGroovy) {
248254
IngestionConfig ingestionConfig = tableConfig.getIngestionConfig();
249255

250256
if (ingestionConfig != null) {
@@ -292,6 +298,10 @@ public static void validateIngestionConfig(TableConfig tableConfig, @Nullable Sc
292298
if (filterConfig != null) {
293299
String filterFunction = filterConfig.getFilterFunction();
294300
if (filterFunction != null) {
301+
if (disableGroovy && FunctionEvaluatorFactory.isGroovyExpression(filterFunction)) {
302+
throw new IllegalStateException(
303+
"Groovy filter functions are disabled for table config. Found '" + filterFunction + "'");
304+
}
295305
try {
296306
FunctionEvaluatorFactory.getExpressionEvaluator(filterFunction);
297307
} catch (Exception e) {
@@ -319,11 +329,16 @@ public static void validateIngestionConfig(TableConfig tableConfig, @Nullable Sc
319329
throw new IllegalStateException("Duplicate transform config found for column '" + columnName + "'");
320330
}
321331
FunctionEvaluator expressionEvaluator;
332+
if (disableGroovy && FunctionEvaluatorFactory.isGroovyExpression(transformFunction)) {
333+
throw new IllegalStateException(
334+
"Groovy transform functions are disabled for table config. Found '" + transformFunction
335+
+ "' for column '" + columnName + "'");
336+
}
322337
try {
323338
expressionEvaluator = FunctionEvaluatorFactory.getExpressionEvaluator(transformFunction);
324339
} catch (Exception e) {
325340
throw new IllegalStateException(
326-
"Invalid transform function '" + transformFunction + "' for column '" + columnName + "'");
341+
"Invalid transform function '" + transformFunction + "' for column '" + columnName + "'", e);
327342
}
328343
List<String> arguments = expressionEvaluator.getArguments();
329344
if (arguments.contains(columnName)) {

‎pinot-segment-local/src/test/java/org/apache/pinot/segment/local/utils/TableConfigUtilsTest.java‎

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -312,6 +312,25 @@ public void validateIngestionConfig() {
312312
new TransformConfig("transformedCol", "Groovy({x+y}, x, y)")), null)).build();
313313
TableConfigUtils.validate(tableConfig, schema);
314314

315+
// invalid transform config since Groovy is disabled
316+
try {
317+
TableConfigUtils.validate(tableConfig, schema, null, true);
318+
Assert.fail("Should fail when Groovy functions disabled but found in transform config");
319+
} catch (IllegalStateException e) {
320+
// expected
321+
}
322+
323+
// invalid filter config since Groovy is disabled
324+
tableConfig = new TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME).setIngestionConfig(
325+
new IngestionConfig(null, null, new FilterConfig("Groovy({timestamp > 0}, timestamp)"), null, null))
326+
.build();
327+
try {
328+
TableConfigUtils.validate(tableConfig, schema, null, true);
329+
Assert.fail("Should fail when Groovy functions disabled but found in filter config");
330+
} catch (IllegalStateException e) {
331+
// expected
332+
}
333+
315334
// null transform column name
316335
tableConfig = new TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME).setIngestionConfig(
317336
new IngestionConfig(null, null, null, Lists.newArrayList(new TransformConfig(null, "reverse(anotherCol)")),
@@ -1229,7 +1248,7 @@ public void testTaskConfig() {
12291248
Assert.assertTrue(e.getMessage().contains("RealtimeToOfflineTask doesn't support upsert table"));
12301249
}
12311250
// validate that TASK config will be skipped with skip string.
1232-
TableConfigUtils.validate(tableConfig, schema, "TASK,UPSERT");
1251+
TableConfigUtils.validate(tableConfig, schema, "TASK,UPSERT", false);
12331252

12341253
// invalid period
12351254
HashMap<String, String> invalidPeriodConfig = new HashMap<>(realtimeToOfflineTaskConfig);

0 commit comments

Comments
 (0)