metron-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Otto Fowler <ottobackwa...@gmail.com>
Subject METRON-378 - variables that don't exist with STELLAR built in arithmetic functions
Date Fri, 28 Oct 2016 14:07:52 GMT
I was looking at METRON-378, and was wondering if we could discuss a little
bit.

This issue, as stated is that if your stellar expression references
variables that are not passed in at runtime, the call still succeeds and
passes back a value.

>From the issue:

  @Test(expected = ParseException.class)
  public void testMissingVariables() {
    String query = "1 + cannot + add + missing + variables";
    run(query, new HashMap<>());
  }


This works, because getDouble is implemented thusly:

private Double getDouble(Token<?> token) {
  Number n = (Number) token.getValue();
  if (n == null) {
    return 0d;
  } else {
    return n.doubleValue();
  }
}

Is the STELLAR function suite, we have to do the work to handle missing
parameters and variables, but here, for things handled in the
StellarCompiler we do not.

So the first question is if we want to change this behavior to be
consistent with the rest of the library, that is to throw an exception with
invalid parameters.

Changing the above function to return null will accomplish that, with the
side effect of having to refactor the validation calls.  As we have
discussed before, having validation implemented by passing in a known
failure case and expecting an error is not really ideal, since it muddies
the purpose of what you are validating ( syntactical correctness of the
expression vs logical runtime correctness ) among other things.  With a
change like this, validation will get an exception, not the return we
currently expect in the tests etc.

Maybe we don’t want to handle this in the Compiler at all?  Maybe
StellarParser’s parse() function should validate parameters?  This would, I
think, require a refactoring or new version of validate so that running
validate inside of parse is not expensive and uses the existing constructs
built in the call.  This would give ‘global’ validation to parameters I
believe, and allow for some cleanup in the existing functions, at the cost
of the a per function default handling.

Anyways,  looking for some sage feedback, hopefully from someone who knows
the reasoning behind 'return 0d if null ‘.  Maybe this is a documentation
issue, or can be treated as such?

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message