Copilot commented on code in PR #17091:
URL: https://github.com/apache/iotdb/pull/17091#discussion_r2732423401
##########
integration-test/src/test/java/org/apache/iotdb/pipe/it/dual/treemodel/auto/basic/IoTDBTreePatternFormatIT.java:
##########
@@ -443,11 +434,10 @@ public void testMultipleHybridPatternRealtimeData()
throws Exception {
}
@Test
- @Ignore("Disabled: multi/exclusion tree patterns are blocked in this branch")
public void testPrefixPatternWithExclusionHistoricalData() throws Exception {
final Map<String, String> sourceAttributes = new HashMap<>();
// Inclusion: Match everything under root.db.d1 and root.db.d2
- sourceAttributes.put("source.pattern", "root.db.d1, root.db.d2");
+ sourceAttributes.put("source.pattern.inclusion", "root.db.d1.**,
root.db.d2.**");
// Exclusion: Exclude anything with the prefix root.db.d1.s1
Review Comment:
The comment says "Exclude anything with the prefix root.db.d1.s1" but the
pattern uses IoTDB syntax, not prefix syntax. Since the new
source.pattern.inclusion and source.pattern.exclusion keys only support IoTDB
patterns, the exclusion "root.db.d1.s1" will match exactly "root.db.d1.s1" or
"root.db.d1.s1.**" (depending on how IoTDB pattern matching works), not
"anything with the prefix root.db.d1.s1". This comment should be corrected to
reflect IoTDB pattern matching semantics.
```suggestion
// Exclusion: Exclude the timeseries root.db.d1.s1
```
##########
integration-test/src/test/java/org/apache/iotdb/pipe/it/dual/treemodel/auto/basic/IoTDBTreePatternFormatIT.java:
##########
@@ -302,10 +300,9 @@ public void testMultiplePrefixPatternHistoricalData()
throws Exception {
}
@Test
- @Ignore("Disabled: multi/exclusion tree patterns are blocked in this branch")
public void testMultiplePrefixPatternRealtimeData() throws Exception {
final Map<String, String> sourceAttributes = new HashMap<>();
- sourceAttributes.put("source.pattern", "root.db.d1.s, root.db2.d1.s");
+ sourceAttributes.put("source.pattern.inclusion", "root.db.d1.s*,
root.db2.d1.s");
Review Comment:
Similar to testMultiplePrefixPatternHistoricalData, this test name is
misleading as it uses IoTDB pattern syntax with wildcards rather than prefix
patterns.
##########
iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/pipe/datastructure/pattern/TreePattern.java:
##########
@@ -143,34 +145,49 @@ public static <T> List<T> applyIndexesOnList(
*/
public static TreePattern parsePipePatternFromSourceParameters(
final PipeParameters sourceParameters) {
- final TreePattern treePattern =
parsePipePatternFromSourceParametersInternal(sourceParameters);
- if (!treePattern.isSingle()) {
- final String msg =
- String.format(
- "Pipe: The provided pattern should be single now. " +
"Inclusion: %s, Exclusion: %s",
- sourceParameters.getStringByKeys(EXTRACTOR_PATTERN_KEY,
SOURCE_PATTERN_KEY),
- sourceParameters.getStringByKeys(
- EXTRACTOR_PATTERN_EXCLUSION_KEY,
SOURCE_PATTERN_EXCLUSION_KEY));
- LOGGER.warn(msg);
- throw new PipeException(msg);
- }
- return treePattern;
+ return parsePipePatternFromSourceParametersInternal(sourceParameters);
}
public static TreePattern parsePipePatternFromSourceParametersInternal(
final PipeParameters sourceParameters) {
final boolean isTreeModelDataAllowedToBeCaptured =
isTreeModelDataAllowToBeCaptured(sourceParameters);
+ final boolean hasPatternInclusionKey =
+ sourceParameters.hasAnyAttributes(
+ EXTRACTOR_PATTERN_INCLUSION_KEY, SOURCE_PATTERN_INCLUSION_KEY);
+ final boolean hasLegacyPathKey =
+ sourceParameters.hasAnyAttributes(EXTRACTOR_PATH_KEY, SOURCE_PATH_KEY);
+ final boolean hasLegacyPatternKey =
+ sourceParameters.hasAnyAttributes(EXTRACTOR_PATTERN_KEY,
SOURCE_PATTERN_KEY);
+
+ if (hasPatternInclusionKey && (hasLegacyPathKey || hasLegacyPatternKey)) {
+ final String msg =
+ String.format(
+ "Pipe: %s cannot be used together with %s or %s.",
+ SOURCE_PATTERN_INCLUSION_KEY, SOURCE_PATTERN_KEY,
SOURCE_PATH_KEY);
+ LOGGER.warn(msg);
+ throw new PipeException(msg);
+ }
Review Comment:
There is no test coverage for the mutual exclusivity validation that
prevents using source.pattern.inclusion together with source.pattern or
source.path (lines 164-171). Consider adding a test that verifies a
PipeException is thrown when these incompatible parameters are used together.
##########
integration-test/src/test/java/org/apache/iotdb/pipe/it/dual/treemodel/auto/basic/IoTDBTreePatternFormatIT.java:
##########
@@ -475,10 +465,9 @@ public void testPrefixPatternWithExclusionHistoricalData()
throws Exception {
}
@Test
- @Ignore("Disabled: multi/exclusion tree patterns are blocked in this branch")
public void testPrefixPatternWithExclusionRealtimeData() throws Exception {
final Map<String, String> sourceAttributes = new HashMap<>();
- sourceAttributes.put("source.pattern", "root.db.d1, root.db.d2");
+ sourceAttributes.put("source.pattern.inclusion", "root.db.d1.**,
root.db.d2.**");
Review Comment:
Similar to the historical data test, this test name is misleading as it uses
IoTDB pattern syntax rather than prefix patterns.
##########
iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/pipe/datastructure/pattern/TreePattern.java:
##########
@@ -143,34 +145,49 @@ public static <T> List<T> applyIndexesOnList(
*/
public static TreePattern parsePipePatternFromSourceParameters(
final PipeParameters sourceParameters) {
- final TreePattern treePattern =
parsePipePatternFromSourceParametersInternal(sourceParameters);
- if (!treePattern.isSingle()) {
- final String msg =
- String.format(
- "Pipe: The provided pattern should be single now. " +
"Inclusion: %s, Exclusion: %s",
- sourceParameters.getStringByKeys(EXTRACTOR_PATTERN_KEY,
SOURCE_PATTERN_KEY),
- sourceParameters.getStringByKeys(
- EXTRACTOR_PATTERN_EXCLUSION_KEY,
SOURCE_PATTERN_EXCLUSION_KEY));
- LOGGER.warn(msg);
- throw new PipeException(msg);
- }
- return treePattern;
+ return parsePipePatternFromSourceParametersInternal(sourceParameters);
}
public static TreePattern parsePipePatternFromSourceParametersInternal(
final PipeParameters sourceParameters) {
final boolean isTreeModelDataAllowedToBeCaptured =
isTreeModelDataAllowToBeCaptured(sourceParameters);
+ final boolean hasPatternInclusionKey =
+ sourceParameters.hasAnyAttributes(
+ EXTRACTOR_PATTERN_INCLUSION_KEY, SOURCE_PATTERN_INCLUSION_KEY);
+ final boolean hasLegacyPathKey =
+ sourceParameters.hasAnyAttributes(EXTRACTOR_PATH_KEY, SOURCE_PATH_KEY);
+ final boolean hasLegacyPatternKey =
+ sourceParameters.hasAnyAttributes(EXTRACTOR_PATTERN_KEY,
SOURCE_PATTERN_KEY);
+
+ if (hasPatternInclusionKey && (hasLegacyPathKey || hasLegacyPatternKey)) {
+ final String msg =
+ String.format(
+ "Pipe: %s cannot be used together with %s or %s.",
+ SOURCE_PATTERN_INCLUSION_KEY, SOURCE_PATTERN_KEY,
SOURCE_PATH_KEY);
+ LOGGER.warn(msg);
+ throw new PipeException(msg);
+ }
+
// 1. Parse INCLUSION patterns into a list
List<TreePattern> inclusionPatterns =
- parsePatternList(
- sourceParameters,
- isTreeModelDataAllowedToBeCaptured,
- EXTRACTOR_PATH_KEY,
- SOURCE_PATH_KEY,
- EXTRACTOR_PATTERN_KEY,
- SOURCE_PATTERN_KEY);
+ hasPatternInclusionKey
+ ? parseIoTDBPatternList(
+ sourceParameters.getStringByKeys(
+ EXTRACTOR_PATTERN_INCLUSION_KEY,
SOURCE_PATTERN_INCLUSION_KEY),
+ isTreeModelDataAllowedToBeCaptured,
+ true,
+ SOURCE_PATTERN_INCLUSION_KEY)
Review Comment:
The new pattern inclusion keys always parse patterns as IoTDBTreePattern,
completely ignoring the source.pattern.format parameter. This is inconsistent
with the legacy behavior where source.pattern respects the format parameter
(lines 634-678 in parsePatternsFromPatternParameter). Users who want to use
prefix format patterns with the new inclusion keys will not be able to do so.
Consider either: 1) Supporting source.pattern.format for the new keys, or 2)
Documenting this as a breaking change and intentional design decision.
##########
iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/pipe/datastructure/pattern/TreePattern.java:
##########
@@ -181,14 +198,34 @@ public static TreePattern
parsePipePatternFromSourceParametersInternal(
}
// 2. Parse EXCLUSION patterns into a list
+ if (hasPatternInclusionKey
+ && sourceParameters.hasAnyAttributes(
+ EXTRACTOR_PATH_EXCLUSION_KEY, SOURCE_PATH_EXCLUSION_KEY)) {
+ final String msg =
+ String.format(
+ "Pipe: %s cannot be used together with %s.",
+ SOURCE_PATTERN_INCLUSION_KEY, SOURCE_PATH_EXCLUSION_KEY);
Review Comment:
The error message only mentions SOURCE_PATH_EXCLUSION_KEY but should also
mention EXTRACTOR_PATH_EXCLUSION_KEY for consistency with other error messages.
Both keys are checked in the condition but only one is shown in the error
message. Consider updating the message to include both keys or use a more
generic description.
```suggestion
"Pipe: %s cannot be used together with %s or %s.",
SOURCE_PATTERN_INCLUSION_KEY, EXTRACTOR_PATH_EXCLUSION_KEY,
SOURCE_PATH_EXCLUSION_KEY);
```
##########
iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/pipe/datastructure/pattern/TreePattern.java:
##########
@@ -143,34 +145,49 @@ public static <T> List<T> applyIndexesOnList(
*/
public static TreePattern parsePipePatternFromSourceParameters(
final PipeParameters sourceParameters) {
- final TreePattern treePattern =
parsePipePatternFromSourceParametersInternal(sourceParameters);
- if (!treePattern.isSingle()) {
- final String msg =
- String.format(
- "Pipe: The provided pattern should be single now. " +
"Inclusion: %s, Exclusion: %s",
- sourceParameters.getStringByKeys(EXTRACTOR_PATTERN_KEY,
SOURCE_PATTERN_KEY),
- sourceParameters.getStringByKeys(
- EXTRACTOR_PATTERN_EXCLUSION_KEY,
SOURCE_PATTERN_EXCLUSION_KEY));
- LOGGER.warn(msg);
- throw new PipeException(msg);
- }
- return treePattern;
+ return parsePipePatternFromSourceParametersInternal(sourceParameters);
}
public static TreePattern parsePipePatternFromSourceParametersInternal(
final PipeParameters sourceParameters) {
final boolean isTreeModelDataAllowedToBeCaptured =
isTreeModelDataAllowToBeCaptured(sourceParameters);
+ final boolean hasPatternInclusionKey =
+ sourceParameters.hasAnyAttributes(
+ EXTRACTOR_PATTERN_INCLUSION_KEY, SOURCE_PATTERN_INCLUSION_KEY);
+ final boolean hasLegacyPathKey =
+ sourceParameters.hasAnyAttributes(EXTRACTOR_PATH_KEY, SOURCE_PATH_KEY);
+ final boolean hasLegacyPatternKey =
+ sourceParameters.hasAnyAttributes(EXTRACTOR_PATTERN_KEY,
SOURCE_PATTERN_KEY);
+
+ if (hasPatternInclusionKey && (hasLegacyPathKey || hasLegacyPatternKey)) {
+ final String msg =
+ String.format(
+ "Pipe: %s cannot be used together with %s or %s.",
+ SOURCE_PATTERN_INCLUSION_KEY, SOURCE_PATTERN_KEY,
SOURCE_PATH_KEY);
Review Comment:
The error message only mentions SOURCE_PATTERN_INCLUSION_KEY,
SOURCE_PATTERN_KEY, and SOURCE_PATH_KEY but should also mention their
EXTRACTOR_* equivalents for consistency. The condition checks for
EXTRACTOR_PATTERN_INCLUSION_KEY, EXTRACTOR_PATH_KEY, and EXTRACTOR_PATTERN_KEY
but these are not included in the error message. Consider updating the message
to include all checked keys or use more generic descriptions.
```suggestion
"Pipe: %s or %s cannot be used together with %s, %s, %s or
%s.",
EXTRACTOR_PATTERN_INCLUSION_KEY,
SOURCE_PATTERN_INCLUSION_KEY,
EXTRACTOR_PATTERN_KEY,
SOURCE_PATTERN_KEY,
EXTRACTOR_PATH_KEY,
SOURCE_PATH_KEY);
```
##########
integration-test/src/test/java/org/apache/iotdb/pipe/it/dual/treemodel/auto/basic/IoTDBTreePatternFormatIT.java:
##########
@@ -271,11 +270,10 @@ private void testPipeWithMultiplePatterns(
}
@Test
- @Ignore("Disabled: multi/exclusion tree patterns are blocked in this branch")
public void testMultiplePrefixPatternHistoricalData() throws Exception {
Review Comment:
The test name "testMultiplePrefixPatternHistoricalData" is misleading
because it uses IoTDB pattern syntax with wildcards ("root.db.d1.s*") rather
than prefix patterns. The new source.pattern.inclusion key only supports IoTDB
pattern format and ignores source.pattern.format. Either rename this test to
reflect that it uses IoTDB patterns, or if prefix patterns were intended, this
test will not work as expected.
```suggestion
public void testMultipleIoTDBPatternHistoricalData() throws Exception {
```
##########
iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/pipe/datastructure/pattern/TreePattern.java:
##########
@@ -181,14 +198,34 @@ public static TreePattern
parsePipePatternFromSourceParametersInternal(
}
// 2. Parse EXCLUSION patterns into a list
+ if (hasPatternInclusionKey
+ && sourceParameters.hasAnyAttributes(
+ EXTRACTOR_PATH_EXCLUSION_KEY, SOURCE_PATH_EXCLUSION_KEY)) {
+ final String msg =
+ String.format(
+ "Pipe: %s cannot be used together with %s.",
+ SOURCE_PATTERN_INCLUSION_KEY, SOURCE_PATH_EXCLUSION_KEY);
+ LOGGER.warn(msg);
+ throw new PipeException(msg);
+ }
Review Comment:
There is no test coverage for the mutual exclusivity validation that
prevents using source.pattern.inclusion together with source.path.exclusion
(lines 201-210). Consider adding a test that verifies a PipeException is thrown
when these incompatible parameters are used together.
##########
integration-test/src/test/java/org/apache/iotdb/pipe/it/dual/treemodel/auto/basic/IoTDBTreePatternFormatIT.java:
##########
@@ -443,11 +434,10 @@ public void testMultipleHybridPatternRealtimeData()
throws Exception {
}
@Test
- @Ignore("Disabled: multi/exclusion tree patterns are blocked in this branch")
public void testPrefixPatternWithExclusionHistoricalData() throws Exception {
Review Comment:
The test name "testPrefixPatternWithExclusionHistoricalData" is misleading
because it uses IoTDB pattern syntax with wildcards ("root.db.d1.**",
"root.db.d2.**") rather than prefix patterns. Consider renaming to
"testIoTDBPatternWithExclusionHistoricalData" or similar to accurately reflect
the pattern type being tested.
```suggestion
public void testIoTDBPatternWithExclusionHistoricalData() throws Exception
{
```
--
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]
