Copilot commented on code in PR #16575:
URL: https://github.com/apache/iotdb/pull/16575#discussion_r2425568032
##########
iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/pipe/source/IoTDBNonDataRegionSource.java:
##########
@@ -76,7 +77,11 @@ public void customize(
throws Exception {
super.customize(parameters, configuration);
- final TreePattern pattern =
TreePattern.parsePipePatternFromSourceParameters(parameters);
+ TreePattern pattern =
TreePattern.parsePipePatternFromSourceParameters(parameters);
+ // TODO
+ if (pattern instanceof UnionTreePattern) {
+ pattern = ((UnionTreePattern) pattern).getFirstPattern();
Review Comment:
Silently collapsing a UnionTreePattern to its first element discards
additional patterns and breaks multi-pattern semantics introduced in this PR.
Either propagate the UnionTreePattern and adjust downstream logic to evaluate
against all member patterns, or fail fast with a clear exception until this
source supports unions; do not silently drop patterns.
```suggestion
// Fail fast if a UnionTreePattern is encountered, as union patterns are
not supported.
if (pattern instanceof UnionTreePattern) {
throw new IllegalArgumentException(
"UnionTreePattern is not supported for IoTDBNonDataRegionSource. "
+
"Please specify a single prefix or full path pattern.");
```
##########
iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/pipe/datastructure/pattern/UnionTreePattern.java:
##########
@@ -0,0 +1,135 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iotdb.commons.pipe.datastructure.pattern;
+
+import org.apache.tsfile.file.metadata.IDeviceID;
+
+import java.util.List;
+import java.util.stream.Collectors;
+
+/**
+ * Represents a union of multiple {@link TreePattern}s. A path is considered
to match if it matches
+ * any of the patterns in the collection.
+ */
+public class UnionTreePattern extends TreePattern {
+
+ private final List<TreePattern> patterns;
+
+ /**
+ * Constructs a {@link UnionTreePattern} with a list of {@link TreePattern}s.
+ *
+ * @param patterns A list of {@link TreePattern}s. Assumes all patterns
share the same value for
+ * isTreeModelDataAllowedToBeCaptured.
+ */
+ public UnionTreePattern(final List<TreePattern> patterns) {
+ super(patterns.stream().anyMatch(pattern ->
pattern.isTreeModelDataAllowedToBeCaptured));
+ this.patterns = patterns;
+ }
+
+ public TreePattern getFirstPattern() {
+ return patterns.get(0);
+ }
Review Comment:
getFirstPattern() will throw IndexOutOfBoundsException when patterns is
empty (which can occur if parseMultiplePatterns receives only
commas/whitespace). Add a defensive check or enforce non-empty patterns in the
constructor (e.g., Objects.requireNonNull and a non-empty assertion) to make
this safe.
##########
iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/pipe/datastructure/pattern/TreePattern.java:
##########
@@ -44,72 +47,134 @@ public abstract class TreePattern {
private static final Logger LOGGER =
LoggerFactory.getLogger(TreePattern.class);
protected final boolean isTreeModelDataAllowedToBeCaptured;
- protected final String pattern;
- protected TreePattern(final boolean isTreeModelDataAllowedToBeCaptured,
final String pattern) {
+ protected TreePattern(final boolean isTreeModelDataAllowedToBeCaptured) {
this.isTreeModelDataAllowedToBeCaptured =
isTreeModelDataAllowedToBeCaptured;
- this.pattern = pattern != null ? pattern : getDefaultPattern();
}
public boolean isTreeModelDataAllowedToBeCaptured() {
return isTreeModelDataAllowedToBeCaptured;
}
- public String getPattern() {
- return pattern;
- }
-
- public boolean isRoot() {
- return Objects.isNull(pattern) ||
this.pattern.equals(this.getDefaultPattern());
- }
-
/**
* Interpret from source parameters and get a {@link TreePattern}.
*
* @return The interpreted {@link TreePattern} which is not {@code null}.
*/
public static TreePattern parsePipePatternFromSourceParameters(
final PipeParameters sourceParameters) {
+ final Function<List<TreePattern>, TreePattern> listToTreePattern =
+ list -> list.size() == 1 ? list.get(0) : new UnionTreePattern(list);
+
final boolean isTreeModelDataAllowedToBeCaptured =
isTreeModelDataAllowToBeCaptured(sourceParameters);
final String path = sourceParameters.getStringByKeys(EXTRACTOR_PATH_KEY,
SOURCE_PATH_KEY);
+ final String pattern =
+ sourceParameters.getStringByKeys(EXTRACTOR_PATTERN_KEY,
SOURCE_PATTERN_KEY);
- // 1. If "source.path" is specified, it will be interpreted as an
IoTDB-style path,
- // ignoring the other 2 parameters.
- if (path != null) {
- return new IoTDBTreePattern(isTreeModelDataAllowedToBeCaptured, path);
+ // 1. If both "source.path" and "source.pattern" are specified, their
union will be used.
+ if (path != null && pattern != null) {
+ final List<TreePattern> result = new ArrayList<>();
+ // Parse "source.path" as IoTDB-style path.
+ result.addAll(
+ parseMultiplePatterns(
+ path, p -> new
IoTDBTreePattern(isTreeModelDataAllowedToBeCaptured, p)));
+ // Parse "source.pattern" using the helper method.
+ result.addAll(
+ parsePatternsFromPatternParameter(
+ pattern, sourceParameters, isTreeModelDataAllowedToBeCaptured));
+ return listToTreePattern.apply(result);
}
- final String pattern =
- sourceParameters.getStringByKeys(EXTRACTOR_PATTERN_KEY,
SOURCE_PATTERN_KEY);
+ // 2. If only "source.path" is specified, it will be interpreted as an
IoTDB-style path.
+ if (path != null) {
+ return listToTreePattern.apply(
+ parseMultiplePatterns(
+ path, p -> new
IoTDBTreePattern(isTreeModelDataAllowedToBeCaptured, p)));
+ }
- // 2. Otherwise, If "source.pattern" is specified, it will be interpreted
- // according to "source.pattern.format".
+ // 3. If only "source.pattern" is specified, parse it using the helper
method.
if (pattern != null) {
- final String patternFormat =
- sourceParameters.getStringByKeys(EXTRACTOR_PATTERN_FORMAT_KEY,
SOURCE_PATTERN_FORMAT_KEY);
+ return listToTreePattern.apply(
+ parsePatternsFromPatternParameter(
+ pattern, sourceParameters, isTreeModelDataAllowedToBeCaptured));
+ }
- // If "source.pattern.format" is not specified, use prefix format by
default.
- if (patternFormat == null) {
- return new PrefixTreePattern(isTreeModelDataAllowedToBeCaptured,
pattern);
- }
+ // 4. If neither "source.path" nor "source.pattern" is specified,
+ // this pipe source will match all data.
+ return listToTreePattern.apply(
+ Collections.singletonList(new
IoTDBTreePattern(isTreeModelDataAllowedToBeCaptured, null)));
+ }
- switch (patternFormat.toLowerCase()) {
- case EXTRACTOR_PATTERN_FORMAT_IOTDB_VALUE:
- return new IoTDBTreePattern(isTreeModelDataAllowedToBeCaptured,
pattern);
- case EXTRACTOR_PATTERN_FORMAT_PREFIX_VALUE:
- return new PrefixTreePattern(isTreeModelDataAllowedToBeCaptured,
pattern);
- default:
- LOGGER.info(
- "Unknown pattern format: {}, use prefix matching format by
default.", patternFormat);
- return new PrefixTreePattern(isTreeModelDataAllowedToBeCaptured,
pattern);
+ /**
+ * A private helper method to parse a list of {@link TreePattern}s from the
"pattern" parameter,
+ * considering its "format".
+ *
+ * @param pattern The pattern string to parse.
+ * @param sourceParameters The source parameters to read the format from.
+ * @param isTreeModelDataAllowedToBeCaptured A boolean flag passed to the
TreePattern constructor.
+ * @return A list of parsed {@link TreePattern}s.
+ */
+ private static List<TreePattern> parsePatternsFromPatternParameter(
+ final String pattern,
+ final PipeParameters sourceParameters,
+ final boolean isTreeModelDataAllowedToBeCaptured) {
+ final String patternFormat =
+ sourceParameters.getStringByKeys(EXTRACTOR_PATTERN_FORMAT_KEY,
SOURCE_PATTERN_FORMAT_KEY);
+
+ // If "source.pattern.format" is not specified, use prefix format by
default.
+ if (patternFormat == null) {
+ return parseMultiplePatterns(
+ pattern, p -> new
PrefixTreePattern(isTreeModelDataAllowedToBeCaptured, p));
+ }
+
+ switch (patternFormat.toLowerCase()) {
+ case EXTRACTOR_PATTERN_FORMAT_IOTDB_VALUE:
+ return parseMultiplePatterns(
+ pattern, p -> new
IoTDBTreePattern(isTreeModelDataAllowedToBeCaptured, p));
+ case EXTRACTOR_PATTERN_FORMAT_PREFIX_VALUE:
+ return parseMultiplePatterns(
+ pattern, p -> new
PrefixTreePattern(isTreeModelDataAllowedToBeCaptured, p));
+ default:
+ LOGGER.info(
+ "Unknown pattern format: {}, use prefix matching format by
default.", patternFormat);
+ return parseMultiplePatterns(
+ pattern, p -> new
PrefixTreePattern(isTreeModelDataAllowedToBeCaptured, p));
+ }
+ }
+
+ private static List<TreePattern> parseMultiplePatterns(
+ final String pattern, final Function<String, TreePattern>
patternSupplier) {
+ if (pattern.isEmpty()) {
+ return Collections.singletonList(patternSupplier.apply(pattern));
+ }
+
+ final List<TreePattern> patterns = new ArrayList<>();
+ final StringBuilder currentPattern = new StringBuilder();
+ boolean inBackticks = false;
+
+ for (final char c : pattern.toCharArray()) {
+ if (c == '`') {
+ inBackticks = !inBackticks;
+ currentPattern.append(c);
+ } else if (c == ',' && !inBackticks) {
+ final String singlePattern = currentPattern.toString().trim();
+ if (!singlePattern.isEmpty()) {
+ patterns.add(patternSupplier.apply(singlePattern));
+ }
+ currentPattern.setLength(0);
+ } else {
+ currentPattern.append(c);
}
}
- // 3. If neither "source.path" nor "source.pattern" is specified,
- // this pipe source will match all data.
- return new IoTDBTreePattern(isTreeModelDataAllowedToBeCaptured, null);
+ final String lastPattern = currentPattern.toString().trim();
+ if (!lastPattern.isEmpty()) {
+ patterns.add(patternSupplier.apply(lastPattern));
+ }
+
Review Comment:
If the input consists only of delimiters/whitespace (e.g., ' , , '), this
returns an empty list, which can produce a UnionTreePattern with no elements
and trigger IndexOutOfBoundsException when getFirstPattern() is called or lead
to unexpected behavior. Handle blank inputs by returning a default/single
pattern (e.g., patternSupplier.apply(null)) or by throwing an
IllegalArgumentException; alternatively, after building, if patterns.isEmpty(),
map to a single default.
```suggestion
// If no valid patterns were found, return a default pattern (e.g.,
patternSupplier.apply(null))
if (patterns.isEmpty()) {
return Collections.singletonList(patternSupplier.apply(null));
}
```
##########
iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/pipe/datastructure/pattern/UnionTreePattern.java:
##########
@@ -0,0 +1,135 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iotdb.commons.pipe.datastructure.pattern;
+
+import org.apache.tsfile.file.metadata.IDeviceID;
+
+import java.util.List;
+import java.util.stream.Collectors;
+
+/**
+ * Represents a union of multiple {@link TreePattern}s. A path is considered
to match if it matches
+ * any of the patterns in the collection.
+ */
+public class UnionTreePattern extends TreePattern {
+
+ private final List<TreePattern> patterns;
+
+ /**
+ * Constructs a {@link UnionTreePattern} with a list of {@link TreePattern}s.
+ *
+ * @param patterns A list of {@link TreePattern}s. Assumes all patterns
share the same value for
+ * isTreeModelDataAllowedToBeCaptured.
+ */
+ public UnionTreePattern(final List<TreePattern> patterns) {
+ super(patterns.stream().anyMatch(pattern ->
pattern.isTreeModelDataAllowedToBeCaptured));
Review Comment:
Javadoc states all patterns 'share the same value' for
isTreeModelDataAllowedToBeCaptured, but the implementation uses anyMatch, which
permits mixed values and sets the union flag to true if any child allows
capture. Either update the Javadoc to reflect the actual behavior, or enforce
the invariant (e.g., validate all flags are equal and throw if not). Also
consider guarding against null/empty lists in the constructor.
```suggestion
* @param patterns A non-null, non-empty list of {@link TreePattern}s. All
patterns must share the same value for
* isTreeModelDataAllowedToBeCaptured.
* @throws IllegalArgumentException if patterns is null, empty, or
contains mixed values for isTreeModelDataAllowedToBeCaptured.
*/
public UnionTreePattern(final List<TreePattern> patterns) {
if (patterns == null || patterns.isEmpty()) {
throw new IllegalArgumentException("patterns must be non-null and
non-empty");
}
boolean flag = patterns.get(0).isTreeModelDataAllowedToBeCaptured;
for (TreePattern pattern : patterns) {
if (pattern.isTreeModelDataAllowedToBeCaptured != flag) {
throw new IllegalArgumentException("All patterns must share the same
value for isTreeModelDataAllowedToBeCaptured");
}
}
super(flag);
```
--
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]
