-
Notifications
You must be signed in to change notification settings - Fork 505
METRON-1277 Add match statement to Stellar language #814
Conversation
| Scala's match. | ||
|
|
||
| The syntax is: | ||
| * `match{ logical_expression1 : evaluation expression1, logical_expression2 : evaluation_expression2` : A match expression with no default |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a missing } here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, thanks
| NULL : 'null' | 'NULL'; | ||
| NAN : 'NaN'; | ||
|
|
||
| AS : 'as' | 'AS'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this may be fairly unlikely, but technically this is a breaking change for Stellar. I believe if anyone had a variable called as, AS, etc. it would fail with a parse exception. I think we should explicitly call this out. Also, is this something we technically have to wait for a major version release to add?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, AS was for the match(longVarName as shortName){ x > 5 : shortName is available }
case, but since I'm not doing that, i should remove it.
| @Ignore | ||
| public void testMatchMAPEvaluation() { | ||
|
|
||
| // NOTE: THIS IS BROKEN RIGHT NOW. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you delete this comment and add it to @Ignore("NOTE: THIS IS BROKEN RIGHT NOW.") instead? Maybe also mention that it's broken because the use of MAP is not supported right now. It may even be better to just delete the test, as it will probably be written when that feature is implemented. Also, this may just be forgotten about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually wanted to leave this in so @cestella might help me figure out why it isn't working with map. How about I leave it for now, and you hold your plus +1 until it is removed. I would got to commit with this, but I want it there for the review.
|
|
||
| @Test | ||
| @SuppressWarnings("unchecked") | ||
| public void testShortCircut() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor spelling error (Circut -> Circuit).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks
| @SuppressWarnings("unchecked") | ||
| public void testMatchErrorNoDefault() { | ||
|
|
||
| boolean caught = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't use a boolean flag to determine if an exception was thrown. If you just want to expect an exception we should use @Test(expected = ParseException.class). For more indepth testing we should use @Rule public ExpectedException thrown = ExpectedException.none();.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. When I was developing, I just had one big test function that I was adding test after test too, so I used this to catch the exception and keep going. When I did the PR I refactored the tests, but forgot to clean this up. Thanks!
|
|
||
| Where: | ||
|
|
||
| * `logical_expression` is a Stellar expression that evaluates to true or false. For instance `var > 0` or `var > 0 AND var2 == 'foo'` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems as if the below stellar always throws an exception. Is it by design that default clauses are always evaluated? If so can you mention that here?
foo := 500
match{ foo < 100 : THROW('oops'), foo > 200 : 'ok', default : THROW('exception thrown') }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please note that functions are loading lazily in the background and will be unavailable until loaded fully.
[Stellar]>>> Functions loaded, you may refer to functions now...
[Stellar]>>> foo := 500
[Stellar]>>> match{ foo < 100 : THROW('oops'), foo > 200 : 'ok', default : THROW('exception thrown') }
ok
[Stellar]>>>I don't see that.
Also, if you check the last test in TestMatch you will see I have a test for this kind of thing.
| } | ||
| if (skipMatchClauses && (token.getUnderlyingType() == MatchClauseEnd.class | ||
| || token.getUnderlyingType() == MatchClauseCheckExpr.class)) { | ||
| while (it.hasNext()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will only skip one match clause after a true one is found. See below test for a stellar statement that fails with a ParseException.
@Test
@SuppressWarnings("unchecked")
public void moreThanTwoConsecutiveTrueCasesWillResultInFailure() {
Assert.assertEquals("ok", run("match{ foo < 100 : THROW('oops'), foo > 200 : 'ok', foo > 300 : 'ok', default : 'works' }",
new HashMap() {{
put("foo", 500);
}}));
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is another issue with validate
| transformation_expr | ||
| ; | ||
|
|
||
| match_clause_check : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this extra expression needed? Could we get rid of it? Instead of this just have what's below?
match_clause_action :
transformation_expr #MatchClauseAction
;
match_clause_check :
logical_expr #MatchClauseCheckExpr
;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try that again. I don't remember the specific reason why I ended up with it factored like this. But this is what ended up working ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| ### Stellar Language Match Expression | ||
|
|
||
| Stellar provides the capability to write match expressions, which are similar to switch statements commonly found in c like languages, but more like | ||
| Scala's match. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be a bit hesitant to say this is similar to Scala's match. Our match statement doesn't really support pattern matching.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have any suggestions for a better way to describe it? I'm not super happy with this either, I'll take suggestions ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ottobackwards sure. Right now, I think our match statement syntax is just a nicer syntax for if then else blocks. I would just state something along those lines. I know in the future it will be more.
|
Also, I don't think there is an issue with the map function directly. It's actually an issue with the logical_expression // Here there is a null on the stack
@Test
@SuppressWarnings("unchecked")
public void test1FailsWithMoreOnStack() {
Assert.assertEquals("a", run("match{ foo : 'a' }",
new HashMap() {{
put("foo", true);
}}));
}// Here it's an empty stack when we expect 'a' i believe. @Test
@SuppressWarnings("unchecked")
public void test1FailsEmptyStack() {
Assert.assertEquals("a", run("match{ foo == true : 'a' }",
new HashMap() {{
put("foo", true);
}}));
}// This is *not* properly handled
@Test
@SuppressWarnings("unchecked")
public void test1Passes() {
Assert.assertEquals("a", run("match{ foo : 'a', default: 'b' }",
new HashMap() {{
put("foo", true);
}}));
}// This is properly handled
@Test
@SuppressWarnings("unchecked")
public void test1Passes() {
Assert.assertEquals("a", run("match{ foo == true : 'a', default: 'b' }",
new HashMap() {{
put("foo", true);
}}));
}// Example where map works
@Test
@SuppressWarnings("unchecked")
public void workingMatchWithMap() {
Assert.assertEquals(Arrays.asList("OK", "HAHA"), run("match{ foo > 100 : THROW('oops'), foo > 200 : THROW('oh no'), foo >= 50 : MAP(['ok', 'haha'], (a) -> TO_UPPER(a)), default: 'a' }",
new HashMap() {{
put("foo", 50);
}}));
} |
|
Thanks, I added those tests to the test class, and these fail: @Test
@SuppressWarnings("unchecked")
public void testVariableOnlyNoDefault() {
Assert.assertEquals("a", run("match{ foo : 'a' }",
new HashMap() {{
put("foo", true);
}}));
}
@Test
@SuppressWarnings("unchecked")
public void testVariableEqualsCheckNoDefault() {
Assert.assertEquals("a", run("match{ foo == true : 'a' }",
new HashMap() {{
put("foo", true);
}}));
}
@Test
@SuppressWarnings("unchecked")
public void testVariableOnlyCheckWithDefault() {
Assert.assertEquals("a", run("match{ foo : 'a', default: 'b' }",
new HashMap() {{
put("foo", true);
}}));
}I will take a look. I feel that I have similar tests to these that are working, so it is strange. |
|
@jjmeyer0 thanks for the tests. The issue is that we call validate and then parse. When we call validate, we resolve all vars to NULL. This means that the clauses in these failures are false. As stated in the readme, something in the match must be true or you get an error. In this case, we have an issue where validate is forcing us into that condition and we don't have a default, so it is an error. |
|
So for example: /*
curr is the current value on the stack. This is the non-deferred actual evaluation for this expression
and with the current context.
*/
Token<?> curr = instanceDeque.peek();
if (curr != null && curr.getValue() != null && curr.getValue() instanceof Boolean
&& ShortCircuitOp.class.isAssignableFrom(token.getUnderlyingType())) {
//if we have a boolean as the current value and the next non-contextual token is a short circuit op
//then we need to short circuit possibly
if (token.getUnderlyingType() == BooleanArg.class) {with this function: @Test
@SuppressWarnings("unchecked")
public void testVariableOnlyCheckWithDefault() {
Assert.assertEquals("a", run("match{ foo : 'a', default : 'b' }",
new HashMap() {{
put("foo", true);
}}));
}When we get to that point, because of the validation of vars to null |
|
Because it is possible in validate() for a boolean to be null instead of true or false. |
…ests that throw exceptions. Also partially addressed issues around validation during tests resulting in empty stacks or other stack issues because of null variable resolution. The fix is not complete. I have added tests to cover the discovered failing cases, and they are all resolved but one. This one is ignored for the moment while there is review discussion around validation
|
@jjmeyer0 @cestella : Ok. I feel as if all of these things are wrong, because the validation by executing with null variables is itself logically wrong. We have discussed this before, but this is really evident here. At least I feel it is. I think that we should use compilation instead of validation. We can decide to make it a separate option when running tests, or to make it the way we do all tests etc. But if compilation does what I think it does, then I think it is more correct. I would like to do that, and remove the work around I have introduced here ( basically detecting that we are validating and have a single var that is null because of validation ) since changing validation would be the real complete answer. |
|
for example: @Test
public void testValidation() {
Object value = run("IF x == null THEN THROW('it cannot be null') ELSE 'it is ok'", new HashMap(){{
put("x","something");
}} );
}This is a valid test, but it fails on validation not parse. |
1. Require default statement 2. change the action separator from : to => so that... 3. we can use conditional_exp and logical_exp ( can check for EXISTS() with IF etc )
|
OK, the validation issues are a big big deal. I have changed the match functionality so that they work in that context, supporting guard expressions and requiring the default operation.
I still have to do the hack for the match{ var1 => 'ok' , default => 'notOk' } So the new syntax is match { logical_or_conditional_expr => transform_exp , default => transform_exp } |
|
the issue with MAP() is resolved as well |
|
@ottobackwards something is still going on with this. I'm seeing the following behavior: [Stellar]>>> foo := 500
[Stellar]>>> match{ foo > 100 => ['oops'], foo > 200 => ['oh no'], foo >= 500 => MAP(['ok', 'haha'], (a) -> TO_UPPER(a)), default => ['a']}
[!] Invalid parse, found [OK, HAHA]
org.apache.metron.stellar.dsl.ParseException: Invalid parse, found [OK, HAHA]
at org.apache.metron.stellar.common.StellarCompiler$Expression.apply(StellarCompiler.java:210)
at org.apache.metron.stellar.common.BaseStellarProcessor.parse(BaseStellarProcessor.java:152)
at org.apache.metron.stellar.common.shell.StellarExecutor.execute(StellarExecutor.java:292)
at org.apache.metron.stellar.common.shell.StellarShell.handleStellar(StellarShell.java:282)
at org.apache.metron.stellar.common.shell.StellarShell.execute(StellarShell.java:514)
at org.jboss.aesh.console.AeshProcess.run(AeshProcess.java:53)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
at java.lang.Thread.run(Thread.java:748)
[Stellar]>>> |
|
@jjmeyer0 I have fixed the issue. I had lost short circuit to the end of all clauses, fixing that fixes the case. I added tests for your issues. I think I'm going to refactor the tests, but you should be able to check where it is in the mean time. |
|
That fix also fixes the issue where IF THEN ELSE in the action clause was not working |
|
Bump? |
jjmeyer0
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry about the delay. Things seems to be working now. I made one last comment.
| | EXISTS LPAREN IDENTIFIER RPAREN #ExistsFunc | ||
| | LPAREN conditional_expr RPAREN #condExpr_paren | ||
| | functions #func | ||
| | DEFAULT #Default |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just noticed this. Why do we have default here? This would allow default to be used in some unintended places. Is there some reasoning behind this?
|
Great catch! Moved it out. |
|
@ottobackwards this looks good to me now. I re-built everything, ran the tests, and played around in stellar shell. The only question I'm still not sure about is the one that deals with backwards compatibility. I believe with the new keywords ( |
|
Great! I am not sure on that. @cestella what do you think? |
|
Maybe we need a tool that tests stellar scripts or configurations that the user can run and report? |
|
@jjmeyer0 @ottobackwards Yeah, it's definitely possible to break expressions that include variables named We probably DO want a script as @ottobackwards suggests, but I don't think it's required here for this PR. All of this is my $0.02 |
|
Ok, I'll update the upgrading doc. |
|
Ok, please have a look at the doc. I'll note that we have not added any notes previously for this type of thing. Also: METRON-1339: Stellar shellShould have a way to validate deployed functions has been created. I may have a PR for that soon |
|
once travis build completes successfully this looks good +1. |
|
@cestella any last words before I pull the trigger on this? Any comment? |
|
Yeah, let me run this up once more in the REPL and take a final look at the docs, but I've been monitoring and I like what I see so far. |
|
PR: #856 adds capability to use the stellar shell to validate stellar statements at rest out of ZK |
|
Alright, +1 otto, you did great work here. I'm very, very impressed. Thanks to @jjmeyer0 for the careful review and assistance. Open source dev at its finest. |
|
actually, hold on there, I found one more bug: I'm +1 after that's fixed. |
|
Ok, what was is_alert in this test? |
|
sorry, it's not set, so it's null. |
|
Ok, I'll check it out after i do my full dev test on validate |
|
@cestella new test and fix is in |
|
Ok, my +1 stands |
as discussed on the dev list: Stellar support for switch/case style conditionals
This PR adds 'match' capability to the stellar language. This is very similar to the Scala match feature.
FROM THE README:
Stellar Language Match Expression
Stellar provides the capability to write match expressions, which are similar to switch statements commonly found in c like languages, but more like
Scala's match.
The syntax is:
match{ logical_expression1 => evaluation expression1, logical_expression2 => evaluation_expression2, default => default_expression}Where:
logical_expressionis a Stellar expression that evaluates to true or false. For instancevar > 0orvar > 0 AND var2 == 'foo'orIF ... THEN ... ELSEevaluation_expressionis any Stellar Expressiondefaultis a required default return value, should no logical expression matchReview items
This PR does not
Testing
from the Stellar shell execute various statements involving match such as:
For all changes:
For code changes:
Have you included steps to reproduce the behavior or problem that is being changed or addressed?
Have you included steps or a guide to how the change may be verified and tested manually?
Have you ensured that the full suite of tests and checks have been executed in the root metron folder via:
Have you written or updated unit tests and or integration tests to verify your changes?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
Have you verified the basic functionality of the build by building and running locally with Vagrant full-dev environment or the equivalent?
For documentation related changes:
Have you ensured that format looks appropriate for the output in which it is rendered by building and verifying the site-book? If not then run the following commands and the verify changes via
site-book/target/site/index.html: