This is an automated email from the ASF dual-hosted git repository.
Jackie-Jiang pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/pinot.git
The following commit(s) were added to refs/heads/master by this push:
new 6a2ab1221db Reject input file segment name templates that reference no
match group (#19755)
6a2ab1221db is described below
commit 6a2ab1221db93def68f1ea6a4bae7d826f3827f5
Author: Xiaotian (Jackie) Jiang <[email protected]>
AuthorDate: Mon Oct 5 15:58:49 2026 -0700
Reject input file segment name templates that reference no match group
(#19755)
---
.../creator/name/FixedSegmentNameGenerator.java | 1 -
.../name/InputFileSegmentNameGenerator.java | 35 ++++++++++++++--------
.../name/NormalizedDateSegmentNameGenerator.java | 1 -
.../creator/name/SimpleSegmentNameGenerator.java | 1 -
.../name/InputFileSegmentNameGeneratorTest.java | 12 ++++++++
5 files changed, 35 insertions(+), 15 deletions(-)
diff --git
a/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/name/FixedSegmentNameGenerator.java
b/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/name/FixedSegmentNameGenerator.java
index 59d52b2c01b..8c68c132eca 100644
---
a/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/name/FixedSegmentNameGenerator.java
+++
b/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/name/FixedSegmentNameGenerator.java
@@ -23,7 +23,6 @@ import javax.annotation.Nullable;
/// Fixed segment name generator which always returns the fixed segment name.
-@SuppressWarnings("serial")
public class FixedSegmentNameGenerator implements SegmentNameGenerator {
private final String _segmentName;
diff --git
a/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/name/InputFileSegmentNameGenerator.java
b/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/name/InputFileSegmentNameGenerator.java
index de8c5d2b93b..6dbe425648e 100644
---
a/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/name/InputFileSegmentNameGenerator.java
+++
b/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/name/InputFileSegmentNameGenerator.java
@@ -31,13 +31,10 @@ import org.slf4j.LoggerFactory;
/// Segment name generator that supports defining the segment name based on
the input file name and path, via a pattern
-/// (matched against the input file URI) and a template (currently only
supports ${filePathPattern:\N}, where N is the
-/// group match number from the regex).
-@SuppressWarnings("serial")
+/// (matched against the input file URI) and a template (currently only
supports `${filePathPattern:\N}`, where N is the
+/// group match number from the regex). The template must reference at least
one match group of the pattern.
public class InputFileSegmentNameGenerator implements SegmentNameGenerator {
-
private static final Logger LOGGER =
LoggerFactory.getLogger(InputFileSegmentNameGenerator.class);
-
private static final String PARAMETER_TEMPLATE = "${filePathPattern:\\%d}";
private final Pattern _filePathPattern;
@@ -59,8 +56,12 @@ public class InputFileSegmentNameGenerator implements
SegmentNameGenerator {
throw new IllegalArgumentException(
String.format("Invalid filePathPattern: %s for
InputFileSegmentNameGenerator", filePathPattern), e);
}
- Preconditions
- .checkArgument(segmentNameTemplate != null, "Missing
segmentNameTemplate for InputFileSegmentNameGenerator");
+ Preconditions.checkArgument(segmentNameTemplate != null,
+ "Missing segmentNameTemplate for InputFileSegmentNameGenerator");
+ Preconditions.checkArgument(referencesMatchGroup(segmentNameTemplate,
_filePathPattern),
+ "Invalid segmentNameTemplate: %s for InputFileSegmentNameGenerator, it
must reference a match group of "
+ + "filePathPattern: %s via ${filePathPattern:\\N} (in a table
config, escape it as $${filePathPattern:\\N} "
+ + "to skip environment variable resolution)", segmentNameTemplate,
filePathPattern);
_segmentNameTemplate = segmentNameTemplate;
try {
_inputFileUri = new URI(inputFileUri);
@@ -72,13 +73,23 @@ public class InputFileSegmentNameGenerator implements
SegmentNameGenerator {
_segmentName = makeSegmentName();
}
+ private static boolean referencesMatchGroup(String segmentNameTemplate,
Pattern filePathPattern) {
+ int groupCount = filePathPattern.matcher("").groupCount();
+ for (int i = 1; i <= groupCount; i++) {
+ if (segmentNameTemplate.contains(String.format(PARAMETER_TEMPLATE, i))) {
+ return true;
+ }
+ }
+ return false;
+ }
+
private String makeSegmentName() {
String inputFilePath = _inputFileUri.getPath();
Matcher m = _filePathPattern.matcher(inputFilePath);
String segmentName;
if (!m.matches()) {
- LOGGER.warn(String.format("No match for pattern '%s' in '%s'",
_filePathPattern, inputFilePath));
+ LOGGER.warn("No match for pattern '{}' in '{}'", _filePathPattern,
inputFilePath);
segmentName = safeConvertPathToFilename(inputFilePath);
} else {
segmentName = _segmentNameTemplate;
@@ -99,10 +110,10 @@ public class InputFileSegmentNameGenerator implements
SegmentNameGenerator {
}
private String safeConvertPathToFilename(String inputFilePath) {
- // Strip of leading '/' characters
- inputFilePath = inputFilePath.replaceFirst("^[/]+", "");
- // Try to create a valid filename by removing any '/' or '.' chars with '_'
- return inputFilePath.replaceAll("[/\\.]", "_");
+ // Strip off leading '/' characters
+ inputFilePath = inputFilePath.replaceFirst("^/+", "");
+ // Try to create a valid filename by replacing any '/' or '.' chars with
'_'
+ return inputFilePath.replaceAll("[/.]", "_");
}
@Override
diff --git
a/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/name/NormalizedDateSegmentNameGenerator.java
b/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/name/NormalizedDateSegmentNameGenerator.java
index 741642d2c1b..2eecc71e801 100644
---
a/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/name/NormalizedDateSegmentNameGenerator.java
+++
b/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/name/NormalizedDateSegmentNameGenerator.java
@@ -30,7 +30,6 @@ import org.joda.time.DateTime;
/// Segment name generator that normalizes the date to human readable format.
-@SuppressWarnings("serial")
public class NormalizedDateSegmentNameGenerator implements
SegmentNameGenerator {
// TODO: This we defined in CommonConstants in common module. SPI should
depend on common, so copying here for now,
// we will need to create a new top level module for such constants and
define them there.
diff --git
a/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/name/SimpleSegmentNameGenerator.java
b/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/name/SimpleSegmentNameGenerator.java
index 8dddc7e2937..e23176ca360 100644
---
a/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/name/SimpleSegmentNameGenerator.java
+++
b/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/name/SimpleSegmentNameGenerator.java
@@ -32,7 +32,6 @@ import javax.annotation.Nullable;
/// - Maximum time value
/// - Segment name postfix
/// - Sequence id
-@SuppressWarnings("serial")
public class SimpleSegmentNameGenerator implements SegmentNameGenerator {
private final String _segmentNamePrefix;
private final String _segmentNamePostfix;
diff --git
a/pinot-segment-spi/src/test/java/org/apache/pinot/segment/spi/creator/name/InputFileSegmentNameGeneratorTest.java
b/pinot-segment-spi/src/test/java/org/apache/pinot/segment/spi/creator/name/InputFileSegmentNameGeneratorTest.java
index 11836de846d..4eba3a1cc59 100644
---
a/pinot-segment-spi/src/test/java/org/apache/pinot/segment/spi/creator/name/InputFileSegmentNameGeneratorTest.java
+++
b/pinot-segment-spi/src/test/java/org/apache/pinot/segment/spi/creator/name/InputFileSegmentNameGeneratorTest.java
@@ -18,9 +18,11 @@
*/
package org.apache.pinot.segment.spi.creator.name;
+import java.util.List;
import org.testng.annotations.Test;
import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertThrows;
import static org.testng.Assert.fail;
@@ -52,6 +54,16 @@ public class InputFileSegmentNameGeneratorTest {
validateName("file:///my/path/to/segmentname", "segmentname", pattern,
template);
}
+ @Test
+ public void testTemplateWithoutMatchGroup() {
+ String pattern = ".+/(.+)\\.csv";
+ String inputFileUri = "file:///my/path/to/segmentname.csv";
+ for (String template : List.of("\\1", "segmentname",
"${filePathPattern:\\2}")) {
+ assertThrows(IllegalArgumentException.class,
+ () -> new InputFileSegmentNameGenerator(pattern, template,
inputFileUri));
+ }
+ }
+
private void validateCSVFileName(String inputFileUriAsStr, String
segmentName) {
String pattern = ".+/(.+)\\.csv";
String template = "${filePathPattern:\\1}";
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
