Skip to content
This repository has been archived by the owner on May 12, 2021. It is now read-only.

METRON-656: Make Stellar 'in' closer to functioning like python #416

Closed
wants to merge 9 commits into from

Conversation

cestella
Copy link
Member

We have an in operator in stellar, but it could be much better. This should bring it at parity with the in operator in python:

  • in should support string contains e.g. 'foo' in 'foobar'
  • in should support Collection contains e.g. 'foo' in [ 'foo', 'bar' ]. Legacy was to only support lists
  • in should support map contains e.g. 'foo' in { 'foo' : 5 }

@@ -418,6 +417,33 @@ public void testList() throws Exception {
}

@Test
public void testInMap() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a couple test cases for String values?

runPredicate("'foo' in { 'foo' : 5 }"...
bar = 'foo'
runPredicate("'foo' in { bar : 5 }"...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing. Done, let me know what you think about them.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! +1

@mmiklavc
Copy link
Contributor

I like the semantics here. One small comment on the tests, +1 pending that adjustment and Travis.

@jjmeyer0
Copy link
Contributor

I think this is a great feature. I do have a few suggestions/questions:

  1. Does it make sense to start moving away from StellarTest and break out into specific test classes (eg. StellarArithmeticTest, StellarPredicateProcessorTest, etc.)? Or Should this be a task in of itself to design a good structure?
  2. I think the tests that are there for in should also be run against not in as well.
  3. Update the README.md to include a description of how in and not in should work. For example, I'm not really sure what I should expect from the expression: 1 in 1111.

@jjmeyer0
Copy link
Contributor

Disregard number 2. It was an oversight on my part. Sorry about that.

@cestella
Copy link
Member Author

@jjmeyer0 Thanks so much for the comments!

  1. Yeah, definitely, but this is a language feature rather than a function that we're testing, so I put it in the StellarTest
  2. I think I got that one covered, but let me know if I botched it. I did add the cases late after the PR was submitted.
  3. Good catch, I'll do that now.

@jjmeyer0
Copy link
Contributor

I found one additional corner case that may need to be addressed. It looks the like expression, null in [ null, 'something' ], returns false, but should return true.

@cestella
Copy link
Member Author

@jjmeyer0 Oh good one!

1. `in` supports string contains. e.g. `'foo' in 'foobar' == true`
2. `in` supports collection contains. e.g. `'foo' in [ 'foo', 'bar' ] == true`
3. `in` supports map key contains. e.g. `'foo' in { 'foo' : 5} == true`
4. `not in` is the negation of the in expression. e.g. `'grok' not in 'foobar' == true`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, one last comment. I was a bit curious as to how the expression 'grok' not in 'foobar' == true would be evaluated by Stellar. I wasn't sure if it would be '(grok' not in 'foobar') == true or 'grok' not in ('foobar' == true). Unfortunately when I tried to run a test it said it is not a valid expression. I think this may be an issue in the Stellar grammar. It is probably outside the scope of this ticket, but I thought I should mention it here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's a stellar language bug. ('grok' not in 'foobar') == true should work.

@jjmeyer0
Copy link
Contributor

jjmeyer0 commented Jan 13, 2017

+1 (non-binding). I'll create a Jira based on the last comments between @cestella and me. (https://issues.apache.org/jira/browse/METRON-658)

@asfgit asfgit closed this in 2db6e35 Jan 16, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants