Support for filter filtering of int type values #7636
Conversation
|
Already for merge |
You should fix CI. Then we will do the review. |
There was a problem hiding this comment.
LGTM. Wait for other reviewers
@YczYanchengzhe Have you really added ClientCpm = from(ServiceInstanceRelation.*).filter(componentId == 7).cpm(); in the OAP for the runtime, is everything working as expected?
| import org.apache.skywalking.oap.server.core.analysis.metrics.annotation.FilterMatcher; | ||
|
|
||
| @FilterMatcher | ||
| public class IntMatch { |
There was a problem hiding this comment.
Can we use match(Number left, Number right) so that they apply to other primitives like long, double, etc.
There was a problem hiding this comment.
I think this is not working. Actually from my understanding, only int left, int right works. Because auto boxing/unboxing is compiling level, which is not available in javaassist compiler.
|
|
||
| @Test | ||
| public void integerShouldEqualWhenLargerThan128() { | ||
| Integer a = 334; | ||
| Integer b = 334; | ||
| boolean match = new EqualMatch().match(a, b); |
There was a problem hiding this comment.
From these tests, the original marcher should already support numeric types, just need to wrap primitives such as int to boxed type Integrr or add method signatures with primitive types
There was a problem hiding this comment.
Same reason as #7636 (comment). And discussed at #7517
There was a problem hiding this comment.
Am I trying below whether adding the method signature can be directly supported
There was a problem hiding this comment.
There is a question about what the regression tests need to do every time you modify the oap part of the code
There was a problem hiding this comment.
Am I trying below whether adding the method signature can be directly supported
What is this? A question or something?
There is a question about what the regression tests need to do every time you modify the oap part of the code
There is no guidance or standards about this, because this is rarely to change, and has been changed for a long time.
There was a problem hiding this comment.
For this problem kezhenxu94 raised a better solution I m trying
There was a problem hiding this comment.
As kezhenxu94 said this can be solved by simply the matching rules support numbers in stringmatch I think this is a better solution and is the class name restored to EqualMatch, I think EqualMatch is more reasonable in design and functionality
There was a problem hiding this comment.
whether it is working or not, is not important at all. If you want to more method, consider int and long. But you would not compare number, especially double and float. Because simply in Java 1.01 == 1.01 is not guaranteed.
| return left == right; | ||
| } | ||
|
|
||
| public boolean match(Integer left, Integer right) { |
There was a problem hiding this comment.
My point is this is not working. Consider to remove this and run test.
There was a problem hiding this comment.
The problem is the automatic binning, which is fine under my test
There was a problem hiding this comment.
This could work, but it still requires you to change g4 file, because it was stringMatch, which doesn't fit the case.
There was a problem hiding this comment.
My point is NumberEqual and IntEqual both make sense and clear. You could add match(long left, long right in case for the future. StringMatch should be kept in String only.
| public boolean match(float left, float right) { | ||
| return left == right; | ||
| } | ||
|
|
||
| public boolean match(Number left, Number right) { | ||
| return left.equals(right); | ||
| } |
There was a problem hiding this comment.
| public boolean match(float left, float right) { | |
| return left == right; | |
| } | |
| public boolean match(Number left, Number right) { | |
| return left.equals(right); | |
| } |
Number doesn't include float like I mentioned.
numberConditionValue
: NUMBER_LITERAL
;
NUMBER_LITERAL : Digits+;
fragment Digits
: [0-9] ([0-9_]* [0-9])?
;
And Number object doesn't make sense too.
There was a problem hiding this comment.
@YczYanchengzhe I think supporting long / Long, int / Integer should be enough in our cases, we only have 1 double typed field in Source scope and its name is usePercent, I don't think it makes sense to compare == between doubles
2. delete number object
|
Thank you so much for your help, I learned a lot of interesting things, I look to see what more ~ than I can do |
CHANGESlog.