[GitHub] metron issue #687: METRON-1090 Add Assignment to Stellar Language
Github user cestella commented on the issue: https://github.com/apache/metron/pull/687 So, I was wrong when I initially looked at this PR, this adds variable updates to the resolver. Should we perhaps separate parsing assignments (e.g. move `StellarAssignment.from` into the language like you've done here) from actually updating the variables. I think I'd like to see Enrichments and the REPL refactored to not hand-code the variable update logic when we get around to updating the resolver to allow updates. Thoughts? ---
[GitHub] metron issue #687: METRON-1090 Add Assignment to Stellar Language
Github user mattf-horton commented on the issue: https://github.com/apache/metron/pull/687 @ottobackwards And yet this discussion primarily relates to whether the assignment operation as defined in Enrichment should be unified with the assignment operation being added to core stellar, presumably due to user demand. ---
[GitHub] metron issue #687: METRON-1090 Add Assignment to Stellar Language
Github user ottobackwards commented on the issue: https://github.com/apache/metron/pull/687 also @mattf-horton, those other things, to be generally useful and not restrictive and assumptive of the host's use cases would have to be generalized _so_ much, I think from an api pov would should wait until there is some demand. ---
[GitHub] metron issue #687: METRON-1090 Add Assignment to Stellar Language
Github user ottobackwards commented on the issue: https://github.com/apache/metron/pull/687 As far as `hosting`. ---
[GitHub] metron issue #687: METRON-1090 Add Assignment to Stellar Language
Github user ottobackwards commented on the issue: https://github.com/apache/metron/pull/687 So, I would say then that we restrict to ONLY core at the moment. And then we start creating interfaces and other api surfaces ( most likely extracted from what we have ) as a follow on. ---
[GitHub] metron issue #687: METRON-1090 Add Assignment to Stellar Language
Github user mattf-horton commented on the issue: https://github.com/apache/metron/pull/687 @ottobackwards , that's inarguably true. We can define the language to be just the Stellar Core. We could also define it to include at least some of those very useful other things, which would be a whole lot more useful to users. ---
[GitHub] metron issue #687: METRON-1090 Add Assignment to Stellar Language
Github user ottobackwards commented on the issue: https://github.com/apache/metron/pull/687 the details of how we configure transforms and enrichments, and organize the stellar statements etc, are not required to implement or host stellar. ---
[GitHub] metron issue #687: METRON-1090 Add Assignment to Stellar Language
Github user mattf-horton commented on the issue: https://github.com/apache/metron/pull/687 > I think we need to keep everything in it's separate pile. We have people asking to host in not metron as recently as today. My interpretation is that, in order to enable hosting Stellar outside of Metron, those items which ordinary users think of as "part of Stellar" but we have placed in five separate piles need to be united into one external-izable pile. Surely it was Metron's intent to have one DSL, not five, right? The fact the current implementation consists of five piles is an implementation detail. ---
[GitHub] metron issue #687: METRON-1090 Add Assignment to Stellar Language
Github user ottobackwards commented on the issue: https://github.com/apache/metron/pull/687 I vote to get rid of `:=` if we want to make it uniform. I'll also straighten the scope stuff out etc. the whole nine yards if we want to do that. I think that might make this a feature branch, but that would be good as it will be easier for @cestella or whomever wanted to contribute to get in on it. ---
[GitHub] metron issue #687: METRON-1090 Add Assignment to Stellar Language
Github user mattf-horton commented on the issue: https://github.com/apache/metron/pull/687 If one insists that ':=' in Enrichments is a Metron add-on not Stellar, that is equivalent to stating that Enrichment doesn't use Stellar but rather a custom DSL derived from Stellar. The xml stuff wrapped around Stellar by Profiler, Enrichments, etc, can reasonably be viewed as external to Stellar, but the ':=' token is clearly part of the Enrichment DSL. The ':=' token is also part of the Stellar REPL, which again we've wrapped with caveats that it's got additions to Stellar that aren't part of Stellar. Sure quacks like a duck to me, but if it's not a Duck then it's a non-Duck, according to Aristotle :-) Fact is, to any user not immersed in implementation details, it's part of Stellar. Why don't we be nice and make it all uniform? ---
[GitHub] metron issue #687: METRON-1090 Add Assignment to Stellar Language
Github user ottobackwards commented on the issue: https://github.com/apache/metron/pull/687 the top level 'scope' being passed in kind of as it is now. ---
[GitHub] metron issue #687: METRON-1090 Add Assignment to Stellar Language
Github user ottobackwards commented on the issue: https://github.com/apache/metron/pull/687 the scope should be managed by the StellarExpression as a separate stack of linked scope nodes and not in the resolvers. ---
[GitHub] metron issue #687: METRON-1090 Add Assignment to Stellar Language
Github user ottobackwards commented on the issue: https://github.com/apache/metron/pull/687 @mattf-horton > As for := vs =, it's not so terrible to have both as exact synonyms. This gives you both your preferred aesthetics and backward compatibility. Not to keep harping on it, but there is a difference between the Language and Stellar and the external constructs that metron has built around it. `:=` is an external thing that metron does, separate from the language or the engine. It is coincidental that we use that same construct in the shell technically. The way we have done the configurations, and structuring their logical meanings is also external to stellar, that is metron. I think we need to keep everything in it's separate pile. We have people asking to host in `not` metron as recently as today. ---
[GitHub] metron issue #687: METRON-1090 Add Assignment to Stellar Language
Github user cestella commented on the issue: https://github.com/apache/metron/pull/687 @nickwallen Yes, you are absolutely correct. Right now we do not have an approach to take a list of stellar expressions that are assignments, a variable resolver, a function resolver and execute that list of statements in a way that variables are updated. At the time we punted on it due to limitations in the VariableResolver to update (also, it's not clear how to do that generally or even specifically in all circumstances). All that being said, it might be nice to add a method to do that in `StellarAssignment` that presumes variable containers are maps. Anyway, that aside, this PR seems to not *force* that change since it's really just moving variable assignment into the language and not updating the resolver explicitly. I'd suggest we tackle that as a follow-on and make a stellar-generic approach to update variable resolvers with new variables. ---
[GitHub] metron issue #687: METRON-1090 Add Assignment to Stellar Language
Github user nickwallen commented on the issue: https://github.com/apache/metron/pull/687 > @cestella: No, both the REPL and enrichments use the same method of assignment, the StellarAssignment class. It's just that assignment in stellar is available in multiple places. Ah, yes, I am familiar with `StellarAssignment`. Ok, but `StellarAssignment` just takes an expression as input and returns the variable and expression. It gets us to the red zone, but not the end zone. To really perform assignment, you still need to (1) execute the expression and (2) add the variable to a `VariableResolver`. Those extra steps are performed currently in `AssignmentCommand` for the REPL and they must also be performed elsewhere for Enrichment, etc. I was thinking that we would have all of those steps performed in the same place. ---
[GitHub] metron issue #687: METRON-1090 Add Assignment to Stellar Language
Github user ottobackwards commented on the issue: https://github.com/apache/metron/pull/687 I am not opposed to changing the resolvers, or continuing work as it comes out of this discussion ---
[GitHub] metron issue #687: METRON-1090 Add Assignment to Stellar Language
Github user ottobackwards commented on the issue: https://github.com/apache/metron/pull/687 @nickwallen pending this discussion yes, it just adds the capability to the language and stellar common, but not to _*Metron*_. As stated in the PR description: > This PR is to get the concept as a proposal out for review and start the discussion. ---
[GitHub] metron issue #687: METRON-1090 Add Assignment to Stellar Language
Github user cestella commented on the issue: https://github.com/apache/metron/pull/687 @nickwallen No, both the REPL and enrichments use the same method of assignment, the `StellarAssignment` class. It's just that assignment in stellar is available in multiple places. ---
[GitHub] metron issue #687: METRON-1090 Add Assignment to Stellar Language
Github user nickwallen commented on the issue: https://github.com/apache/metron/pull/687 > @ottobackwards : This is opt-in for the resolvers. The MapVariableResolver does NOT support update, so there is no change to Enrichment, Parsers, Profile, and Shell ( beyond what has been mentioned ). If this PR does not change Parsers, Enrichment, Profiler, and the REPL, then what does it change? Where can I use assignment based on this PR? Does this just give us the 'capability' to add assignment to each of those? We would then need to change the `VariableResolver`s used in each of those areas to use the assignment as you've implemented it here? ---
[GitHub] metron issue #687: METRON-1090 Add Assignment to Stellar Language
Github user ottobackwards commented on the issue:
https://github.com/apache/metron/pull/687
OK,
The others can comment, but I think proper scope would be implemented
within the execution of the call stack, not arbitrarily by the resolvers. Each
resolver should be a 'scope' and new scopes should be created.
A. Because it is a resolver thing, where would you document it in a way
that makes sense? I have been thinking that we should refactor to explicitly
support scoping in the processor/evaluations and then have the resolvers _used_
in scope as scope implementations. So the nesting of scopes would not be done
in the resolver.
B. As noted in the PR description, I was hoping for discussion ( THIS
discussion as a matter of fact ). Having had some issues with my other PRs and
scope, I chose to explicitly NOT go that far until we talked it through. I did
not feel all the way back then that I had enough of a grasp of the different
resolvers to change their behavior and understand the consequences either way.
C. The LambaExpression actually delegates to the state variable resolver:
```java
VariableResolver variableResolver = new DefaultVariableResolver(
variable -> lambdaVariables.getOrDefault(variable
, state.variableResolver.resolve(variable)
), variable -> true,
(variable, value) -> {
if (state.variableResolver.exists(variable)) {
state.variableResolver.update(variable, value);
}
});
```
D.
- I think I answered above.
- I *think* so, maybe @cestella or @nickwallen can answer that
- Again, I stopped short until I got some kind of feedback.
- I have changed StellarAssignment, I don't think we need both `=` and
`:=`, and if there are `:=` floating around they will work, unless we remove
StellarAssignment
---
[GitHub] metron issue #687: METRON-1090 Add Assignment to Stellar Language
Github user nickwallen commented on the issue: https://github.com/apache/metron/pull/687 > @cestella: No, that's not correct. Stellar enrichments can contain :=. Consider the enrichments here in one of our use-cases. Enrichments can either take a map of enrichments Thanks for clarifying, but 'ugh'. I'll have to look at how that is implemented. I didn't realize we had a separate implementation of assignment. Does this PR address that? Shouldn't we have one implementation of the assignment mechanism? Good commentary from @mattf-horton . I know there is a reason why I didn't want to touch this. :) ---
[GitHub] metron issue #687: METRON-1090 Add Assignment to Stellar Language
Github user mattf-horton commented on the issue: https://github.com/apache/metron/pull/687 So a bunch more comments came in while I was writing the above. Nick's comment about enrichments using assignment already of course modifies my comment about Profilers, etc. Just making it uniform would give users a lot less to wonder about. As for := vs =, it's not so terrible to have both as exact synonyms. This gives you both your preferred aesthetics and backward compatibility. ---
[GitHub] metron issue #687: METRON-1090 Add Assignment to Stellar Language
Github user mattf-horton commented on the issue: https://github.com/apache/metron/pull/687 As a professed language nut, when you mention the word "variable", I think "scope". And it seems to me that by placing the responsibility for variable assignment/update in the VariableResolver, you've actually tied into a very natural definition of scope given the way MapVariableResolver already works in association with a target dictionary or stack of dictionaries. A. I think you should explicitly discuss scope in the documentation. There are several scopes already being used in practice, and I think users need to understand how it all works together, otherwise they will misunderstand and be bewildered. I am, no doubt, a good example, so dense that I have to ask the questions below :-) B. I don't understand why you don't go ahead and enable update in MapVariableResolver, since IIRC that's the resolver for essentially everything interesting that uses variables in Stellar. C. Do I understand correctly from the patch, that variable assignment IS enabled in lambda expressions? Presumably the scope will be the same instance dictionary where the input variables are stored, thus making them local, correct? Can new variables be created as well as input variables be updated? D. The following scopes come to mind, with associated need for clarification: - Lambdas save their input variables in their ephemeral instance context, initialized by call syntax. - Comment: questions already asked above - The REPL used one global dict for scope of its variables, defined by ':='. - Comment: If I understand correctly, when in the REPL, '=' and ':=' assignments will go to the same global context dictionary, is that correct? - Profilers, transformations, and several other xml-defined Stellar-using features, have a persistent instance context, with variables initialized and updated via an xml syntax. - Comment: Apparently here we've specifically exclude assignment, even though this seems to be where it's needed most. Why is that? The instance context seems ready-made for local variables. ---
[GitHub] metron issue #687: METRON-1090 Add Assignment to Stellar Language
Github user cestella commented on the issue: https://github.com/apache/metron/pull/687 @nickwallen No, that's not correct. Stellar enrichments can contain `:=`. Consider the enrichments [here](https://github.com/apache/metron/tree/master/use-cases/geographic_login_outliers#enrich-authentication-events) in one of our use-cases. Enrichments can either take a map of enrichments or a list of strings. If you pass a list of strings (which you would need to do to create temporary variables that you then remove before passing through, like in the example), you'd use an assignment, which is currently `:=`. ---
[GitHub] metron issue #687: METRON-1090 Add Assignment to Stellar Language
Github user ottobackwards commented on the issue: https://github.com/apache/metron/pull/687 @cestella @nickwallen I have changed stellar assignment to cover the := already. I think it will continue to work. ---
[GitHub] metron issue #687: METRON-1090 Add Assignment to Stellar Language
Github user nickwallen commented on the issue: https://github.com/apache/metron/pull/687 > @cestella: What about users with existing stellar enrichments using := ... Oh, and one clarification. No users will have production code (like an enrichment) that uses `:=`. That has only ever been available in the REPL. So that should not be a concern, right? The only concern would be documented uses of the REPL itself; like READMEs. ---
[GitHub] metron issue #687: METRON-1090 Add Assignment to Stellar Language
Github user cestella commented on the issue: https://github.com/apache/metron/pull/687 hah, well @nickwallen, laziness is often a good trait in a developer. Honestly, I am ok with *not* migrating here in favor of more intuitive approaches. I chose `:=` because I needed the functionality and worried about making the language more complex (there's something different about a language that has assignments versus an expression language. I wanted to keep it an expression language at the time.), so I was forced to choose a character sequence that didn't exist as a lexable token. Now, honestly, we're already halfway down that road, so probably the right thing to do *is* deprecate it and rip the bandaid fast. I honestly could be convinced either way. ---
[GitHub] metron issue #687: METRON-1090 Add Assignment to Stellar Language
Github user nickwallen commented on the issue: https://github.com/apache/metron/pull/687 > @cestella Migration, sadly sometimes, is also part of user experience. :) Very true. That's why I offered up the ["Alternative Option"](https://github.com/apache/metron/pull/687#issuecomment-358754026). Only my own laziness makes me "mildly against" that :) . But based on user experience (like you mention), I think we should probably do it. ---
[GitHub] metron issue #687: METRON-1090 Add Assignment to Stellar Language
Github user cestella commented on the issue: https://github.com/apache/metron/pull/687 Yeah, I'm totally cool with going with `=`, but I will point out one snag. What about users with existing stellar enrichments using `:=`, would we provide a migration path beyond saying, "Hey, move your stuff over" in the Upgrading docs? Migration, sadly sometimes, is also part of user experience. :) ---
[GitHub] metron issue #687: METRON-1090 Add Assignment to Stellar Language
Github user nickwallen commented on the issue: https://github.com/apache/metron/pull/687 I have not looked at the code yet, but wanted to comment on the assignment discussion. IMO focusing on user experience is the way to go. To that end... * We should use `=` because that is just more obvious. * We should remove `:=` from the REPL environment (very easy, just delete `AssignmentCommand`). * All references in the docs to `:=` need to be changed to `=`. (Ugh) All of that can happen in this PR or as multiple PRs (however @ottobackwards sees fit), but that is the right end state. **Alternative Option**: The only other reasonable option IMO is to support both `=` and `:=` (for some undetermined period of time.) The only snafu is that both should have the same implementation. So support for `:=` would have to be added to the 'language' and we should NOT rely on the `AssignmentCommand` implementation in the REPL. I am mildly against the "Alternative Option", but wanted to offer it up for discussion. ---
[GitHub] metron issue #687: METRON-1090 Add Assignment to Stellar Language
Github user ottobackwards commented on the issue: https://github.com/apache/metron/pull/687 also, we aren't consistent, in some places ( transformations ) we use the configuration logical structure to imply assignment, in enrichment it is `:=`, but those are configuration mechanisms not stellar mechanisms ---
[GitHub] metron issue #687: METRON-1090 Add Assignment to Stellar Language
Github user ottobackwards commented on the issue: https://github.com/apache/metron/pull/687 By punt you mean leave this implementation that doesn't implement update in the resolvers? ---
[GitHub] metron issue #687: METRON-1090 Add Assignment to Stellar Language
Github user cestella commented on the issue: https://github.com/apache/metron/pull/687 Agreed, this is a POV issue. Though from the user perspective they aren't distinguishing between "this is in the REPL" vs "this is in the enrichment topology", so I think we do need to manage the experience if we're moving functionality from the MACHINE to the LANGUAGE to ensure it's consistent. Beyond that, it's unclear whether we need an resolver `update` function as part of THIS PR, but I think we very much *will* need it if we decide to make stellar multi-expression. I suspect we should punt on it until that effort. ---
[GitHub] metron issue #687: METRON-1090 Add Assignment to Stellar Language
Github user ottobackwards commented on the issue: https://github.com/apache/metron/pull/687 POV issue I think. Stellar has the LANGUAGE ( grammar ), the default implementation, and the 'host' execution mechanics. `:=` is part of the host execution mechanics. The issue may not be being the same as much as the necessity for `:=` as a host construct at all if things are done inside stellar. Does that make sense? ---
[GitHub] metron issue #687: METRON-1090 Add Assignment to Stellar Language
Github user cestella commented on the issue: https://github.com/apache/metron/pull/687 well, `:=` is in the shell and also in the enrichment configuration (see [here](https://github.com/apache/metron/tree/master/metron-platform/metron-enrichment#stellar-enrichment-configuration). Seems like if we're promoting this to the language, we should make things consistent in one way or the other. ---
[GitHub] metron issue #687: METRON-1090 Add Assignment to Stellar Language
Github user ottobackwards commented on the issue: https://github.com/apache/metron/pull/687 @cestella also: := is in the shell not the language. The mechanics of what is LANG and what is MACHINE is part of the issue. ---
[GitHub] metron issue #687: METRON-1090 Add Assignment to Stellar Language
Github user ottobackwards commented on the issue: https://github.com/apache/metron/pull/687 I just removed the conflicts and changed StellarAssignment to account for this ---
[GitHub] metron issue #687: METRON-1090 Add Assignment to Stellar Language
Github user ottobackwards commented on the issue: https://github.com/apache/metron/pull/687 Reviewer Note: Would a default method on the interface be better for update()? ---
[GitHub] metron issue #687: METRON-1090 Add Assignment to Stellar Language
Github user ottobackwards commented on the issue: https://github.com/apache/metron/pull/687 @cestella ---
[GitHub] metron issue #687: METRON-1090 Add Assignment to Stellar Language
Github user ottobackwards commented on the issue: https://github.com/apache/metron/pull/687 It is fair to say that this PR is more for conceptual review than anything else. ---
[GitHub] metron issue #687: METRON-1090 Add Assignment to Stellar Language
Github user ottobackwards commented on the issue: https://github.com/apache/metron/pull/687 should be set @JonZeolla ---
[GitHub] metron issue #687: METRON-1090 Add Assignment to Stellar Language
Github user JonZeolla commented on the issue: https://github.com/apache/metron/pull/687 Is this ready for review? If so, can you deconflict? ---
