I really don't like equating missing variable to 0 as 0 has meaning that does not imply it is missing. What I'd rather see is a return of Double.NaN for all situations where arguments are not a number. Just my $0.02.
On Fri, Oct 28, 2016 at 10:07 AM, Otto Fowler <[email protected]> wrote: > 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? >
