[GitHub] [skywalking] kezhenxu94 commented on a change in pull request #5390: Support IN filter expressions in OAL

[GitBox <gi...@apache.org>]

kezhenxu94 commented on a change in pull request #5390:
URL: https://github.com/apache/skywalking/pull/5390#discussion_r477256306



##########
File path: oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/analysis/metrics/expression/LikeMatch.java
##########
@@ -23,14 +23,18 @@
 @FilterMatcher
 public class LikeMatch {
     public boolean match(String left, String right) {
-        if (left == null || right == null) {
+        if (right == null || left == null) {

Review comment:
       What's the difference

##########
File path: oap-server/oal-grammar/src/main/antlr4/org/apache/skywalking/oal/rt/grammar/OALLexer.g4
##########
@@ -61,6 +61,26 @@ SRC_PROFILE_TASK: 'profile_task';
 SRC_PROFILE_TASK_LOG: 'profile_task_log';
 SRC_PROFILE_THREAD_SHANPSHOT: 'profile_task_segment_snapshot';
 
+// Constructors symbols
+
+DOT:                                 '.';
+LR_BRACKET:                          '(';
+RR_BRACKET:                          ')';
+LR_SQUARE_BRACKET:                   '[';
+RR_SQUARE_BRACKET:                   ']';

Review comment:
       I think the original `LR_BRACKET` means `Left Round Bracket`, so the new one can be simplified `LS_BRACKET`, or if you like `L_SQUARE_BRACKET`

##########
File path: oap-server/oal-rt/src/main/java/org/apache/skywalking/oal/rt/parser/ConditionExpression.java
##########
@@ -18,18 +18,41 @@
 
 package org.apache.skywalking.oal.rt.parser;
 
-import lombok.AllArgsConstructor;
+import java.util.LinkedList;
+import java.util.List;
 import lombok.Getter;
 import lombok.NoArgsConstructor;
 import lombok.Setter;
 
 @Getter
 @Setter
 @NoArgsConstructor
-@AllArgsConstructor
 public class ConditionExpression {
     // original from script
     private String expressionType;
     private String attribute;
     private String value;
+    private List<String> values;

Review comment:
       The minimal change is to annotate this field with `@Builder.Default` and give it a default value (`null` for example)

##########
File path: oap-server/oal-grammar/src/main/antlr4/org/apache/skywalking/oal/rt/grammar/OALLexer.g4
##########
@@ -61,6 +61,26 @@ SRC_PROFILE_TASK: 'profile_task';
 SRC_PROFILE_TASK_LOG: 'profile_task_log';
 SRC_PROFILE_THREAD_SHANPSHOT: 'profile_task_segment_snapshot';
 
+// Constructors symbols

Review comment:
       I don't see any difference between the `like` expression and others, and can be solved by simply moving these symbols, can you check why it (doesn't) work(s) (before) after moving these?

##########
File path: oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/analysis/metrics/expression/LikeMatch.java
##########
@@ -23,14 +23,18 @@
 @FilterMatcher
 public class LikeMatch {
     public boolean match(String left, String right) {
-        if (left == null || right == null) {
+        if (right == null || left == null) {
             return false;
         }
-        if (left.startsWith("%") && left.endsWith("%")) { // %keyword%
-            return right.contains(left.substring(1, left.length() - 1));
+        if (right.startsWith("\"") && right.endsWith("\"")) {
+            right = right.substring(1, right.length() - 1);
         }
-        return (left.startsWith("%") && right.endsWith(left.substring(1)))  // %suffix
-            || (left.endsWith("%") && right.startsWith(left.substring(0, left.length() - 1))) // prefix%
+
+        if (right.startsWith("%") && right.endsWith("%")) { // %keyword%
+            return left.contains(right.substring(1, right.length() - 1));

Review comment:
       OK I made a mistake here before :(




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org