From fd04b410116177e688f7afd987677fbebdeeb09f Mon Sep 17 00:00:00 2001 From: Louis Bergelson Date: Thu, 8 Dec 2016 14:25:52 -0500 Subject: [PATCH 1/8] refactoring jexl --- .../htsjdk/variant/variantcontext/JEXLMap.java | 65 +++++++++++----------- 1 file changed, 32 insertions(+), 33 deletions(-) diff --git a/src/main/java/htsjdk/variant/variantcontext/JEXLMap.java b/src/main/java/htsjdk/variant/variantcontext/JEXLMap.java index b8e13c75b..39d0c23cc 100644 --- a/src/main/java/htsjdk/variant/variantcontext/JEXLMap.java +++ b/src/main/java/htsjdk/variant/variantcontext/JEXLMap.java @@ -5,11 +5,7 @@ import org.apache.commons.jexl2.JexlException; import org.apache.commons.jexl2.MapContext; -import java.util.Collection; -import java.util.Collections; -import java.util.HashMap; -import java.util.Map; -import java.util.Set; +import java.util.*; /** * This is an implementation of a Map of {@link JexlVCMatchExp} to true or false values. @@ -17,21 +13,23 @@ */ class JEXLMap implements Map { + + // our variant context and/or Genotype private final VariantContext vc; private final Genotype g; - // our context - private JexlContext jContext = null; - /** * our mapping from {@link JexlVCMatchExp} to {@link Boolean}s, which will be set to {@code NULL} * for previously un-cached {@link JexlVCMatchExp}. */ - private Map jexl; + private final Map jexl; + + // our context + private JexlContext jContext = null; public JEXLMap(final Collection jexlCollection, final VariantContext vc, final Genotype g) { - initialize(jexlCollection); + this.jexl = initialize(jexlCollection); this.vc = vc; this.g = g; } @@ -43,23 +41,24 @@ public JEXLMap(final Collection jexlCollection, final VariantCon /** * Note: due to laziness, this accessor actually modifies the instance by possibly forcing evaluation of an Jexl expression. * - * @throws IllegalArgumentException when {@code o} is {@code null} or + * @throws IllegalArgumentException when {@code key} is {@code null} or * when any of the JexlVCMatchExp (i.e. keys) contains invalid Jexl expressions. */ - public Boolean get(Object o) { - if (o == null) { + public Boolean get(Object key) { + if (key == null) { throw new IllegalArgumentException("Query key is null"); } // if we've already determined the value, return it - if (jexl.containsKey(o) && jexl.get(o) != null) { - return jexl.get(o); + if (jexl.containsKey(key) && jexl.get(key) != null) { + return jexl.get(key); } // otherwise cast the expression and try again - final JexlVCMatchExp e = (JexlVCMatchExp) o; - evaluateExpression(e); - return jexl.get(e); + final JexlVCMatchExp exp = (JexlVCMatchExp) key; + final boolean matches = evaluateExpression(exp); + jexl.put(exp, matches); + return matches; } /** @@ -87,9 +86,7 @@ public Boolean get(Object o) { */ public Collection values() { for (final JexlVCMatchExp exp : jexl.keySet()) { - if (jexl.get(exp) == null) { - evaluateExpression(exp); - } + jexl.computeIfAbsent(exp, k -> evaluateExpression(exp)); } return jexl.values(); } @@ -115,11 +112,13 @@ public void putAll(Map map) { * Initializes all keys with null values indicating that they have not yet been evaluated. * The actual value will be computed only when the key is requested via {@link #get(Object)} or {@link #values()}. */ - private void initialize(Collection jexlCollection) { - jexl = new HashMap<>(); + private static Map initialize(final Collection jexlCollection) { + final Map jexlMap = new HashMap<>(jexlCollection.size()); for (final JexlVCMatchExp exp: jexlCollection) { - jexl.put(exp, null); + jexlMap.put(exp, null); } + + return jexlMap; } /** @@ -131,19 +130,19 @@ private void initialize(Collection jexlCollection) { * when the Jexl expression in {@code exp} fails to evaluate the JexlContext * constructed with the input VC or genotype. */ - private void evaluateExpression(final JexlVCMatchExp exp) { + private boolean evaluateExpression(final JexlVCMatchExp exp) { // if the context is null, we need to create it to evaluate the JEXL expression if (this.jContext == null) { - createContext(); + jContext = createContext(); } try { final Boolean value = (Boolean) exp.exp.evaluate(jContext); // treat errors as no match - jexl.put(exp, value == null ? false : value); + return value == null ? false : value; } catch (final JexlException.Variable e) { // if exception happens because variable is undefined (i.e. field in expression is not present), evaluate to FALSE - jexl.put(exp,false); + return false; } catch (final JexlException e) { // todo - might be better if no exception is caught here but let's user decide how to deal with them; note this will propagate to get() and values() throw new IllegalArgumentException(String.format("Invalid JEXL expression detected for %s", exp.name), e); @@ -154,13 +153,13 @@ private void evaluateExpression(final JexlVCMatchExp exp) { * Create the internal JexlContext, only when required. * This code is where new JEXL context variables should get added. */ - private void createContext() { + private JexlContext createContext() { if (vc == null) { - jContext = new MapContext(Collections.emptyMap()); + return new MapContext(Collections.emptyMap()); } else if (g == null) { - jContext = new VariantJEXLContext(vc); + return new VariantJEXLContext(vc); } else { - jContext = new GenotypeJEXLContext(vc, g); + return new GenotypeJEXLContext(vc, g); } } @@ -181,7 +180,7 @@ public Boolean remove(Object o) { public Set> entrySet() { - throw new UnsupportedOperationException("clear() not supported on a JEXLMap"); + throw new UnsupportedOperationException("entrySet() not supported on a JEXLMap"); } // nope From 108bbe79cdda863471d422c5396ab1d8de5d3754 Mon Sep 17 00:00:00 2001 From: Louis Bergelson Date: Thu, 8 Dec 2016 16:44:10 -0500 Subject: [PATCH 2/8] add ability to treat missing values differently --- .../htsjdk/variant/variantcontext/JEXLMap.java | 38 ++++++++++++++++++---- 1 file changed, 32 insertions(+), 6 deletions(-) diff --git a/src/main/java/htsjdk/variant/variantcontext/JEXLMap.java b/src/main/java/htsjdk/variant/variantcontext/JEXLMap.java index 39d0c23cc..b9f666c08 100644 --- a/src/main/java/htsjdk/variant/variantcontext/JEXLMap.java +++ b/src/main/java/htsjdk/variant/variantcontext/JEXLMap.java @@ -6,6 +6,7 @@ import org.apache.commons.jexl2.MapContext; import java.util.*; +import java.util.function.Supplier; /** * This is an implementation of a Map of {@link JexlVCMatchExp} to true or false values. @@ -14,11 +15,29 @@ class JEXLMap implements Map { + public enum MissingValuesTreatment { + NO_MATCH(() -> false), + MATCH(() -> true), + THROW(() -> {throw new IllegalArgumentException("Jexl Expression couldn't be evaluated because there was a missing value.");}); + + private final Supplier resultSupplier; + + MissingValuesTreatment(final Supplier resultSupplier){ + this.resultSupplier = resultSupplier; + } + + public boolean getMissingValue(){ + return resultSupplier.get(); + } + + } // our variant context and/or Genotype private final VariantContext vc; private final Genotype g; + private final MissingValuesTreatment howToTreatMissingValues; + /** * our mapping from {@link JexlVCMatchExp} to {@link Boolean}s, which will be set to {@code NULL} * for previously un-cached {@link JexlVCMatchExp}. @@ -28,14 +47,23 @@ // our context private JexlContext jContext = null; - public JEXLMap(final Collection jexlCollection, final VariantContext vc, final Genotype g) { + public JEXLMap(final Collection jexlCollection, final VariantContext vc, final Genotype g, final MissingValuesTreatment howToTreatMissingValues) { this.jexl = initialize(jexlCollection); this.vc = vc; this.g = g; + this.howToTreatMissingValues = howToTreatMissingValues; + } + + public JEXLMap(final Collection jexlCollection, final VariantContext vc, final Genotype g) { + this(jexlCollection, vc, g, MissingValuesTreatment.NO_MATCH); + } + + public JEXLMap(final Collection jexlCollection, final VariantContext vc, final MissingValuesTreatment howToTreatMissingValues) { + this(jexlCollection, vc, null, howToTreatMissingValues); } public JEXLMap(final Collection jexlCollection, final VariantContext vc) { - this(jexlCollection, vc, null); + this(jexlCollection, vc, MissingValuesTreatment.NO_MATCH); } /** @@ -138,11 +166,9 @@ private boolean evaluateExpression(final JexlVCMatchExp exp) { try { final Boolean value = (Boolean) exp.exp.evaluate(jContext); - // treat errors as no match - return value == null ? false : value; + return value == null ? howToTreatMissingValues.getMissingValue() : value; } catch (final JexlException.Variable e) { - // if exception happens because variable is undefined (i.e. field in expression is not present), evaluate to FALSE - return false; + return howToTreatMissingValues.getMissingValue(); } catch (final JexlException e) { // todo - might be better if no exception is caught here but let's user decide how to deal with them; note this will propagate to get() and values() throw new IllegalArgumentException(String.format("Invalid JEXL expression detected for %s", exp.name), e); From d88c5c68778f2b40635c239ed34ff3812a28d4a4 Mon Sep 17 00:00:00 2001 From: Louis Bergelson Date: Fri, 9 Dec 2016 12:47:57 -0500 Subject: [PATCH 3/8] adding overloads to match to allow control of missing value behavior --- .../htsjdk/variant/variantcontext/JEXLMap.java | 30 ++---------- .../variantcontext/VariantContextUtils.java | 53 +++++++++++++++++++++- .../variantcontext/VariantJEXLContextUnitTest.java | 21 ++++++++- 3 files changed, 74 insertions(+), 30 deletions(-) diff --git a/src/main/java/htsjdk/variant/variantcontext/JEXLMap.java b/src/main/java/htsjdk/variant/variantcontext/JEXLMap.java index b9f666c08..cadc2bc5e 100644 --- a/src/main/java/htsjdk/variant/variantcontext/JEXLMap.java +++ b/src/main/java/htsjdk/variant/variantcontext/JEXLMap.java @@ -6,7 +6,6 @@ import org.apache.commons.jexl2.MapContext; import java.util.*; -import java.util.function.Supplier; /** * This is an implementation of a Map of {@link JexlVCMatchExp} to true or false values. @@ -15,28 +14,11 @@ class JEXLMap implements Map { - public enum MissingValuesTreatment { - NO_MATCH(() -> false), - MATCH(() -> true), - THROW(() -> {throw new IllegalArgumentException("Jexl Expression couldn't be evaluated because there was a missing value.");}); - - private final Supplier resultSupplier; - - MissingValuesTreatment(final Supplier resultSupplier){ - this.resultSupplier = resultSupplier; - } - - public boolean getMissingValue(){ - return resultSupplier.get(); - } - - } - // our variant context and/or Genotype private final VariantContext vc; private final Genotype g; - private final MissingValuesTreatment howToTreatMissingValues; + private final VariantContextUtils.JexlMissingValueTreatment howToTreatMissingValues; /** * our mapping from {@link JexlVCMatchExp} to {@link Boolean}s, which will be set to {@code NULL} @@ -47,7 +29,7 @@ public boolean getMissingValue(){ // our context private JexlContext jContext = null; - public JEXLMap(final Collection jexlCollection, final VariantContext vc, final Genotype g, final MissingValuesTreatment howToTreatMissingValues) { + public JEXLMap(final Collection jexlCollection, final VariantContext vc, final Genotype g, final VariantContextUtils.JexlMissingValueTreatment howToTreatMissingValues) { this.jexl = initialize(jexlCollection); this.vc = vc; this.g = g; @@ -55,15 +37,11 @@ public JEXLMap(final Collection jexlCollection, final VariantCon } public JEXLMap(final Collection jexlCollection, final VariantContext vc, final Genotype g) { - this(jexlCollection, vc, g, MissingValuesTreatment.NO_MATCH); - } - - public JEXLMap(final Collection jexlCollection, final VariantContext vc, final MissingValuesTreatment howToTreatMissingValues) { - this(jexlCollection, vc, null, howToTreatMissingValues); + this(jexlCollection, vc, g, VariantContextUtils.JexlMissingValueTreatment.NO_MATCH); } public JEXLMap(final Collection jexlCollection, final VariantContext vc) { - this(jexlCollection, vc, MissingValuesTreatment.NO_MATCH); + this(jexlCollection, vc, null, VariantContextUtils.JexlMissingValueTreatment.NO_MATCH); } /** diff --git a/src/main/java/htsjdk/variant/variantcontext/VariantContextUtils.java b/src/main/java/htsjdk/variant/variantcontext/VariantContextUtils.java index 96eaa64e3..29310d081 100644 --- a/src/main/java/htsjdk/variant/variantcontext/VariantContextUtils.java +++ b/src/main/java/htsjdk/variant/variantcontext/VariantContextUtils.java @@ -46,6 +46,7 @@ import java.util.List; import java.util.Map; import java.util.Set; +import java.util.function.Supplier; public class VariantContextUtils { private static Set MISSING_KEYS_WARNED_ABOUT = new HashSet(); @@ -223,6 +224,23 @@ public JexlVCMatchExp(String name, Expression exp) { } } + public enum JexlMissingValueTreatment { + NO_MATCH(() -> false), + MATCH(() -> true), + THROW(() -> {throw new IllegalArgumentException("Jexl Expression couldn't be evaluated because there was a missing value.");}); + + private final Supplier resultSupplier; + + JexlMissingValueTreatment(final Supplier resultSupplier){ + this.resultSupplier = resultSupplier; + } + + public boolean getMissingValue(){ + return resultSupplier.get(); + } + + } + /** * Method for creating JexlVCMatchExp from input walker arguments names and exps. These two arrays contain * the name associated with each JEXL expression. initializeMatchExps will parse each expression and return @@ -307,6 +325,7 @@ public static boolean match(VariantContext vc, JexlVCMatchExp exp) { * This the best way to apply JEXL expressions to {@link VariantContext} records. * Use the various {@code initializeMatchExps()}'s to create the list of {@link JexlVCMatchExp} expressions. * + * Expressions that contain literals not available in the VariantContext or Genotype will be treated as not matching * @param vc variant context * @param exps expressions * @return true if there is a match @@ -324,7 +343,20 @@ public static boolean match(VariantContext vc, JexlVCMatchExp exp) { * @return true if there is a match */ public static boolean match(VariantContext vc, Genotype g, JexlVCMatchExp exp) { - return match(vc,g, Collections.singletonList(exp)).get(exp); + return match(vc, g, Collections.singletonList(exp), JexlMissingValueTreatment.NO_MATCH).get(exp); + } + + /** + * Returns true if {@code exp} match {@code vc}, {@code g}. + * See {@link #match(VariantContext, Genotype, Collection)} for full docs. + * @param vc variant context + * @param g genotype + * @param exp expression + * @param howToTreatMissingValues what to do if the jexl expression contains literals that aren't in the context + * @return true if there is a match + */ + public static boolean match(VariantContext vc, Genotype g, JexlVCMatchExp exp, JexlMissingValueTreatment howToTreatMissingValues) { + return match(vc, g, Collections.singletonList(exp), howToTreatMissingValues).get(exp); } /** @@ -333,13 +365,30 @@ public static boolean match(VariantContext vc, Genotype g, JexlVCMatchExp exp) { * This the best way to apply JEXL expressions to {@link VariantContext} records. * Use the various {@code initializeMatchExps()}'s to create the list of {@link JexlVCMatchExp} expressions. * + * Expressions that contain literals not available in the VariantContext or Genotype will be treated as not matching * @param vc variant context * @param g genotype * @param exps expressions * @return true if there is a match */ public static Map match(VariantContext vc, Genotype g, Collection exps) { - return new JEXLMap(exps,vc,g); + return match(vc, g, exps, JexlMissingValueTreatment.NO_MATCH); + } + + /** + * Matches each {@link JexlVCMatchExp} exp against the data contained in {@code vc}, {@code g}, + * and returns a map from these expressions to {@code true} (if they matched) or {@code false} (if they didn't). + * This the best way to apply JEXL expressions to {@link VariantContext} records. + * Use the various {@code initializeMatchExps()}'s to create the list of {@link JexlVCMatchExp} expressions. + * + * @param vc variant context + * @param g genotype + * @param exps expressions + * @param howToTreatMissingValues what to do if the jexl expression contains literals that aren't in the context + * @return true if there is a match + */ + public static Map match(VariantContext vc, Genotype g, Collection exps, JexlMissingValueTreatment howToTreatMissingValues) { + return new JEXLMap(exps, vc, g, howToTreatMissingValues); } /** diff --git a/src/test/java/htsjdk/variant/variantcontext/VariantJEXLContextUnitTest.java b/src/test/java/htsjdk/variant/variantcontext/VariantJEXLContextUnitTest.java index bebd39384..5f7380e5f 100644 --- a/src/test/java/htsjdk/variant/variantcontext/VariantJEXLContextUnitTest.java +++ b/src/test/java/htsjdk/variant/variantcontext/VariantJEXLContextUnitTest.java @@ -31,8 +31,6 @@ import htsjdk.variant.vcf.VCFConstants; import org.testng.Assert; -import org.testng.annotations.BeforeClass; -import org.testng.annotations.BeforeMethod; import org.testng.annotations.Test; import java.util.Arrays; @@ -87,6 +85,25 @@ public void testGetValue() { // eval our known expression Assert.assertTrue(!jexlMap.get(exp)); } + + @Test + public void testMissingBehavior(){ + final JexlVCMatchExp exp = new VariantContextUtils.JexlVCMatchExp( + "Zis10", VariantContextUtils.engine.get().createExpression("Z==10")); + + final List alleles = Arrays.asList(Aref, Talt); + VariantContextBuilder vcb = new VariantContextBuilder("test", "chr1", 10, 10, alleles); + VariantContext noZ = vcb.make(); + VariantContext hasZ = vcb.attribute("Z", 0).make(); + + + Assert.assertFalse(VariantContextUtils.match(noZ, null, exp)); //default missing -> no match + Assert.assertFalse(VariantContextUtils.match(noZ, null, exp, VariantContextUtils.JexlMissingValueTreatment.NO_MATCH)); + Assert.assertTrue(VariantContextUtils.match(noZ, null, exp, VariantContextUtils.JexlMissingValueTreatment.MATCH)); + Assert.assertThrows(IllegalArgumentException.class, () -> VariantContextUtils.match(noZ, null, exp, VariantContextUtils.JexlMissingValueTreatment.THROW)); + + Assert.assertFalse(VariantContextUtils.match(hasZ, null, exp, VariantContextUtils.JexlMissingValueTreatment.THROW)); //sanity check that we don't throw if its not missing + } // Testing the new 'FT' and 'isPassFT' expressions in the JEXL map @Test From 15b77d2c0095afb28bd2c57419feb957ae182da2 Mon Sep 17 00:00:00 2001 From: Louis Bergelson Date: Fri, 9 Dec 2016 13:07:08 -0500 Subject: [PATCH 4/8] making enum method package private --- src/main/java/htsjdk/variant/variantcontext/VariantContextUtils.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/htsjdk/variant/variantcontext/VariantContextUtils.java b/src/main/java/htsjdk/variant/variantcontext/VariantContextUtils.java index 29310d081..35162e1bd 100644 --- a/src/main/java/htsjdk/variant/variantcontext/VariantContextUtils.java +++ b/src/main/java/htsjdk/variant/variantcontext/VariantContextUtils.java @@ -235,7 +235,7 @@ public JexlVCMatchExp(String name, Expression exp) { this.resultSupplier = resultSupplier; } - public boolean getMissingValue(){ + boolean getMissingValue(){ return resultSupplier.get(); } From 2d2d78e84cc5bec6540af5f605ab308b5ca1b585 Mon Sep 17 00:00:00 2001 From: Louis Bergelson Date: Fri, 9 Dec 2016 15:13:51 -0500 Subject: [PATCH 5/8] responding to comments --- .../htsjdk/variant/variantcontext/JEXLMap.java | 56 +++++++++++++++++----- .../variantcontext/VariantContextUtils.java | 27 +++++++++-- .../variantcontext/VariantJEXLContextUnitTest.java | 2 +- 3 files changed, 69 insertions(+), 16 deletions(-) diff --git a/src/main/java/htsjdk/variant/variantcontext/JEXLMap.java b/src/main/java/htsjdk/variant/variantcontext/JEXLMap.java index cadc2bc5e..942302e38 100644 --- a/src/main/java/htsjdk/variant/variantcontext/JEXLMap.java +++ b/src/main/java/htsjdk/variant/variantcontext/JEXLMap.java @@ -13,6 +13,11 @@ */ class JEXLMap implements Map { + /** + * If a JEXL expression contains values that are not available in the given context, the default behavior is to + * treat that expression as a miss match. + */ + public static final VariantContextUtils.JexlMissingValueTreatment DEFAULT_MISSING_VALUE_TREATMENT = VariantContextUtils.JexlMissingValueTreatment.MISMATCH; // our variant context and/or Genotype private final VariantContext vc; @@ -29,19 +34,42 @@ // our context private JexlContext jContext = null; + /** + * Construct a new JEXLMap which can evaluate expressions against a specific genotype and variant context + * @param jexlCollection collection of expressions to be evaluated + * @param vc VariantContext to evaluate expressions against + * @param g genotype to evaluate expressions against, may be null + * @param howToTreatMissingValues how missing values in vc and g should be treated + */ public JEXLMap(final Collection jexlCollection, final VariantContext vc, final Genotype g, final VariantContextUtils.JexlMissingValueTreatment howToTreatMissingValues) { - this.jexl = initialize(jexlCollection); + this.jexl = initializeMap(jexlCollection); this.vc = vc; this.g = g; this.howToTreatMissingValues = howToTreatMissingValues; } + + /** + * Construct a new JEXLMap which can evaluate expressions against a specific genotype and variant context + * @param jexlCollection collection of expressions to be evaluated + * @param vc VariantContext to evaluate expressions against + * @param g genotype to evaluate expressions against, may be null + * + * missing values are treated as false + */ public JEXLMap(final Collection jexlCollection, final VariantContext vc, final Genotype g) { - this(jexlCollection, vc, g, VariantContextUtils.JexlMissingValueTreatment.NO_MATCH); + this(jexlCollection, vc, g, DEFAULT_MISSING_VALUE_TREATMENT); } + /** + * Construct a new JEXLMap which can evaluate expressions against a specific VariantContext + * @param jexlCollection collection of expressions to be evaluated + * @param vc VariantContext to evaluate expressions against + * + * missing values are treated as non matches (false) + */ public JEXLMap(final Collection jexlCollection, final VariantContext vc) { - this(jexlCollection, vc, null, VariantContextUtils.JexlMissingValueTreatment.NO_MATCH); + this(jexlCollection, vc, null, DEFAULT_MISSING_VALUE_TREATMENT); } /** @@ -56,8 +84,9 @@ public Boolean get(Object key) { } // if we've already determined the value, return it - if (jexl.containsKey(key) && jexl.get(key) != null) { - return jexl.get(key); + final Boolean value = jexl.get(key); + if (jexl.containsKey(key) && value != null) { + return value; } // otherwise cast the expression and try again @@ -115,10 +144,12 @@ public void putAll(Map map) { } /** - * Initializes all keys with null values indicating that they have not yet been evaluated. + * Initializes a map and give all keys with null values indicating that they have not yet been evaluated. * The actual value will be computed only when the key is requested via {@link #get(Object)} or {@link #values()}. + * + * @return an initialized map of jexlExpression -> null */ - private static Map initialize(final Collection jexlCollection) { + private static Map initializeMap(final Collection jexlCollection) { final Map jexlMap = new HashMap<>(jexlCollection.size()); for (final JexlVCMatchExp exp: jexlCollection) { jexlMap.put(exp, null); @@ -131,7 +162,7 @@ public void putAll(Map map) { * Evaluates a {@link JexlVCMatchExp}'s expression, given the current context (and setup the context if it's {@code null}). * * @param exp the {@link JexlVCMatchExp} to evaluate - * + * @return true if the expression matched the context * @throws IllegalArgumentException when {@code exp} is {@code null}, or * when the Jexl expression in {@code exp} fails to evaluate the JexlContext * constructed with the input VC or genotype. @@ -143,10 +174,12 @@ private boolean evaluateExpression(final JexlVCMatchExp exp) { } try { + //TODO figure out of this can ever evaluate to null or if that isn't actually possible final Boolean value = (Boolean) exp.exp.evaluate(jContext); - return value == null ? howToTreatMissingValues.getMissingValue() : value; + return value == null ? howToTreatMissingValues.getMissingValueOrExplode() : value; } catch (final JexlException.Variable e) { - return howToTreatMissingValues.getMissingValue(); + //this occurs when the jexl expression contained a literal that didn't match anything in the given context + return howToTreatMissingValues.getMissingValueOrExplode(); } catch (final JexlException e) { // todo - might be better if no exception is caught here but let's user decide how to deal with them; note this will propagate to get() and values() throw new IllegalArgumentException(String.format("Invalid JEXL expression detected for %s", exp.name), e); @@ -154,8 +187,9 @@ private boolean evaluateExpression(final JexlVCMatchExp exp) { } /** - * Create the internal JexlContext, only when required. + * Create a new JexlContext * This code is where new JEXL context variables should get added. + * @return a new jexl context initialized appropriately */ private JexlContext createContext() { if (vc == null) { diff --git a/src/main/java/htsjdk/variant/variantcontext/VariantContextUtils.java b/src/main/java/htsjdk/variant/variantcontext/VariantContextUtils.java index 35162e1bd..e55864d48 100644 --- a/src/main/java/htsjdk/variant/variantcontext/VariantContextUtils.java +++ b/src/main/java/htsjdk/variant/variantcontext/VariantContextUtils.java @@ -224,9 +224,23 @@ public JexlVCMatchExp(String name, Expression exp) { } } + /** + * + */ public enum JexlMissingValueTreatment { - NO_MATCH(() -> false), + /** + * Treat expressions with a missing value as a mismatch and evaluate to false + */ + MISMATCH(() -> false), + + /** + * Treat expressions with a missing value as a match and evaluate to true + */ MATCH(() -> true), + + /** + * Treat expressions with a missing value as an error and throw an {@link IllegalArgumentException} + */ THROW(() -> {throw new IllegalArgumentException("Jexl Expression couldn't be evaluated because there was a missing value.");}); private final Supplier resultSupplier; @@ -235,7 +249,12 @@ public JexlVCMatchExp(String name, Expression exp) { this.resultSupplier = resultSupplier; } - boolean getMissingValue(){ + /** + * get the missing value that corresponds to this option or throw an exception + * @return the appropriate f + * @throws IllegalArgumentException if this should be treated as an error + */ + boolean getMissingValueOrExplode(){ return resultSupplier.get(); } @@ -343,7 +362,7 @@ public static boolean match(VariantContext vc, JexlVCMatchExp exp) { * @return true if there is a match */ public static boolean match(VariantContext vc, Genotype g, JexlVCMatchExp exp) { - return match(vc, g, Collections.singletonList(exp), JexlMissingValueTreatment.NO_MATCH).get(exp); + return match(vc, g, Collections.singletonList(exp), JEXLMap.DEFAULT_MISSING_VALUE_TREATMENT).get(exp); } /** @@ -372,7 +391,7 @@ public static boolean match(VariantContext vc, Genotype g, JexlVCMatchExp exp, J * @return true if there is a match */ public static Map match(VariantContext vc, Genotype g, Collection exps) { - return match(vc, g, exps, JexlMissingValueTreatment.NO_MATCH); + return match(vc, g, exps, JEXLMap.DEFAULT_MISSING_VALUE_TREATMENT); } /** diff --git a/src/test/java/htsjdk/variant/variantcontext/VariantJEXLContextUnitTest.java b/src/test/java/htsjdk/variant/variantcontext/VariantJEXLContextUnitTest.java index 5f7380e5f..895956a2f 100644 --- a/src/test/java/htsjdk/variant/variantcontext/VariantJEXLContextUnitTest.java +++ b/src/test/java/htsjdk/variant/variantcontext/VariantJEXLContextUnitTest.java @@ -98,7 +98,7 @@ public void testMissingBehavior(){ Assert.assertFalse(VariantContextUtils.match(noZ, null, exp)); //default missing -> no match - Assert.assertFalse(VariantContextUtils.match(noZ, null, exp, VariantContextUtils.JexlMissingValueTreatment.NO_MATCH)); + Assert.assertFalse(VariantContextUtils.match(noZ, null, exp, VariantContextUtils.JexlMissingValueTreatment.MISMATCH)); Assert.assertTrue(VariantContextUtils.match(noZ, null, exp, VariantContextUtils.JexlMissingValueTreatment.MATCH)); Assert.assertThrows(IllegalArgumentException.class, () -> VariantContextUtils.match(noZ, null, exp, VariantContextUtils.JexlMissingValueTreatment.THROW)); From 85736cb73a31e5c60de85b84174b5c66aac4fb62 Mon Sep 17 00:00:00 2001 From: Louis Bergelson Date: Fri, 9 Dec 2016 15:45:34 -0500 Subject: [PATCH 6/8] updated tests --- .../variantcontext/VariantJEXLContextUnitTest.java | 53 +++++++++++++++------- 1 file changed, 37 insertions(+), 16 deletions(-) diff --git a/src/test/java/htsjdk/variant/variantcontext/VariantJEXLContextUnitTest.java b/src/test/java/htsjdk/variant/variantcontext/VariantJEXLContextUnitTest.java index 895956a2f..0c7418f10 100644 --- a/src/test/java/htsjdk/variant/variantcontext/VariantJEXLContextUnitTest.java +++ b/src/test/java/htsjdk/variant/variantcontext/VariantJEXLContextUnitTest.java @@ -31,12 +31,10 @@ import htsjdk.variant.vcf.VCFConstants; import org.testng.Assert; +import org.testng.annotations.DataProvider; import org.testng.annotations.Test; -import java.util.Arrays; -import java.util.Collections; -import java.util.List; -import java.util.Map; +import java.util.*; /** @@ -53,6 +51,10 @@ private static final VariantContextUtils.JexlVCMatchExp exp = new VariantContextUtils.JexlVCMatchExp("name", VariantContextUtils.engine.get().createExpression("QUAL > 500.0")); + private static final JexlVCMatchExp missingValueExpression = new VariantContextUtils.JexlVCMatchExp( + "Zis10", VariantContextUtils.engine.get().createExpression("Z==10")); + + // SNP alleles: A[ref]/T[alt] at chr1:10. One (crappy) sample, one (bare minimum) VC. private static final SimpleFeature eventLoc = new SimpleFeature("chr1", 10, 10); private static final Allele Aref = Allele.create("A", true); @@ -86,25 +88,44 @@ public void testGetValue() { Assert.assertTrue(!jexlMap.get(exp)); } - @Test - public void testMissingBehavior(){ - final JexlVCMatchExp exp = new VariantContextUtils.JexlVCMatchExp( - "Zis10", VariantContextUtils.engine.get().createExpression("Z==10")); + @Test(dataProvider = "getMissingValueTestData") + public void testMissingBehaviorThroughMatch(VariantContext vc, VariantContextUtils.JexlMissingValueTreatment missingValueTreatment, boolean expected, Class expectedException){ + if(expectedException == null) { + Assert.assertEquals(VariantContextUtils.match(vc, null, missingValueExpression, missingValueTreatment), expected); + } else { + Assert.assertThrows(expectedException, () -> VariantContextUtils.match(vc, null, missingValueExpression, missingValueTreatment)); + } + } + @Test(dataProvider = "getMissingValueTestData") + public void testMissingBehavior(VariantContext vc, VariantContextUtils.JexlMissingValueTreatment missingValueTreatment, boolean expected, Class expectedException){ + final JEXLMap jexlMap = new JEXLMap(Collections.singletonList(missingValueExpression), vc, null, missingValueTreatment); + if(expectedException == null) { + Assert.assertEquals((boolean) jexlMap.get(missingValueExpression), expected); + } else { + Assert.assertThrows(expectedException, () -> jexlMap.get(missingValueExpression)); + } + } + + @DataProvider + public Object[][] getMissingValueTestData(){ final List alleles = Arrays.asList(Aref, Talt); VariantContextBuilder vcb = new VariantContextBuilder("test", "chr1", 10, 10, alleles); VariantContext noZ = vcb.make(); VariantContext hasZ = vcb.attribute("Z", 0).make(); - - Assert.assertFalse(VariantContextUtils.match(noZ, null, exp)); //default missing -> no match - Assert.assertFalse(VariantContextUtils.match(noZ, null, exp, VariantContextUtils.JexlMissingValueTreatment.MISMATCH)); - Assert.assertTrue(VariantContextUtils.match(noZ, null, exp, VariantContextUtils.JexlMissingValueTreatment.MATCH)); - Assert.assertThrows(IllegalArgumentException.class, () -> VariantContextUtils.match(noZ, null, exp, VariantContextUtils.JexlMissingValueTreatment.THROW)); - - Assert.assertFalse(VariantContextUtils.match(hasZ, null, exp, VariantContextUtils.JexlMissingValueTreatment.THROW)); //sanity check that we don't throw if its not missing + return new Object[][]{ + {noZ, JEXLMap.DEFAULT_MISSING_VALUE_TREATMENT, false, null}, + {hasZ, JEXLMap.DEFAULT_MISSING_VALUE_TREATMENT, false, null}, //the value isn't missing but the expression is false + {noZ, VariantContextUtils.JexlMissingValueTreatment.MATCH, true, null}, + {hasZ,VariantContextUtils.JexlMissingValueTreatment.MATCH, false, null}, //the value isn't missing but the expression is false + {noZ, VariantContextUtils.JexlMissingValueTreatment.MISMATCH, false, null}, + {hasZ, VariantContextUtils.JexlMissingValueTreatment.MISMATCH, false, null}, + {noZ, VariantContextUtils.JexlMissingValueTreatment.THROW, false, IllegalArgumentException.class}, + {hasZ, VariantContextUtils.JexlMissingValueTreatment.THROW, false, null} + }; } - + // Testing the new 'FT' and 'isPassFT' expressions in the JEXL map @Test public void testJEXLGenotypeFilters() { From 4988b28bd5dec9d6b615b18be0547b69a23ee2ab Mon Sep 17 00:00:00 2001 From: Louis Bergelson Date: Fri, 9 Dec 2016 15:47:32 -0500 Subject: [PATCH 7/8] moving JexlMissingValueTreatment to top level --- .../htsjdk/variant/variantcontext/JEXLMap.java | 6 ++-- .../variantcontext/JexlMissingValueTreatment.java | 39 ++++++++++++++++++++++ .../variantcontext/VariantContextUtils.java | 37 -------------------- .../variantcontext/VariantJEXLContextUnitTest.java | 16 ++++----- 4 files changed, 50 insertions(+), 48 deletions(-) create mode 100644 src/main/java/htsjdk/variant/variantcontext/JexlMissingValueTreatment.java diff --git a/src/main/java/htsjdk/variant/variantcontext/JEXLMap.java b/src/main/java/htsjdk/variant/variantcontext/JEXLMap.java index 942302e38..33ec59514 100644 --- a/src/main/java/htsjdk/variant/variantcontext/JEXLMap.java +++ b/src/main/java/htsjdk/variant/variantcontext/JEXLMap.java @@ -17,13 +17,13 @@ * If a JEXL expression contains values that are not available in the given context, the default behavior is to * treat that expression as a miss match. */ - public static final VariantContextUtils.JexlMissingValueTreatment DEFAULT_MISSING_VALUE_TREATMENT = VariantContextUtils.JexlMissingValueTreatment.MISMATCH; + public static final JexlMissingValueTreatment DEFAULT_MISSING_VALUE_TREATMENT = JexlMissingValueTreatment.TREAT_AS_MISMATCH; // our variant context and/or Genotype private final VariantContext vc; private final Genotype g; - private final VariantContextUtils.JexlMissingValueTreatment howToTreatMissingValues; + private final JexlMissingValueTreatment howToTreatMissingValues; /** * our mapping from {@link JexlVCMatchExp} to {@link Boolean}s, which will be set to {@code NULL} @@ -41,7 +41,7 @@ * @param g genotype to evaluate expressions against, may be null * @param howToTreatMissingValues how missing values in vc and g should be treated */ - public JEXLMap(final Collection jexlCollection, final VariantContext vc, final Genotype g, final VariantContextUtils.JexlMissingValueTreatment howToTreatMissingValues) { + public JEXLMap(final Collection jexlCollection, final VariantContext vc, final Genotype g, final JexlMissingValueTreatment howToTreatMissingValues) { this.jexl = initializeMap(jexlCollection); this.vc = vc; this.g = g; diff --git a/src/main/java/htsjdk/variant/variantcontext/JexlMissingValueTreatment.java b/src/main/java/htsjdk/variant/variantcontext/JexlMissingValueTreatment.java new file mode 100644 index 000000000..0e6a554ce --- /dev/null +++ b/src/main/java/htsjdk/variant/variantcontext/JexlMissingValueTreatment.java @@ -0,0 +1,39 @@ +package htsjdk.variant.variantcontext; + +import java.util.function.Supplier; + +/** + * How to treat values that appear in a jexl expression but are missing in the context it's applied to + */ +public enum JexlMissingValueTreatment { + /** + * Treat expressions with a missing value as a mismatch and evaluate to false + */ + TREAT_AS_MISMATCH(() -> false), + + /** + * Treat expressions with a missing value as a match and evaluate to true + */ + TREAT_AS_MATCH(() -> true), + + /** + * Treat expressions with a missing value as an error and throw an {@link IllegalArgumentException} + */ + THROW(() -> {throw new IllegalArgumentException("Jexl Expression couldn't be evaluated because there was a missing value.");}); + + private final Supplier resultSupplier; + + JexlMissingValueTreatment(final Supplier resultSupplier){ + this.resultSupplier = resultSupplier; + } + + /** + * get the missing value that corresponds to this option or throw an exception + * @return the appropriate f + * @throws IllegalArgumentException if this should be treated as an error + */ + boolean getMissingValueOrExplode(){ + return resultSupplier.get(); + } + +} diff --git a/src/main/java/htsjdk/variant/variantcontext/VariantContextUtils.java b/src/main/java/htsjdk/variant/variantcontext/VariantContextUtils.java index e55864d48..face55bb9 100644 --- a/src/main/java/htsjdk/variant/variantcontext/VariantContextUtils.java +++ b/src/main/java/htsjdk/variant/variantcontext/VariantContextUtils.java @@ -46,7 +46,6 @@ import java.util.List; import java.util.Map; import java.util.Set; -import java.util.function.Supplier; public class VariantContextUtils { private static Set MISSING_KEYS_WARNED_ABOUT = new HashSet(); @@ -225,42 +224,6 @@ public JexlVCMatchExp(String name, Expression exp) { } /** - * - */ - public enum JexlMissingValueTreatment { - /** - * Treat expressions with a missing value as a mismatch and evaluate to false - */ - MISMATCH(() -> false), - - /** - * Treat expressions with a missing value as a match and evaluate to true - */ - MATCH(() -> true), - - /** - * Treat expressions with a missing value as an error and throw an {@link IllegalArgumentException} - */ - THROW(() -> {throw new IllegalArgumentException("Jexl Expression couldn't be evaluated because there was a missing value.");}); - - private final Supplier resultSupplier; - - JexlMissingValueTreatment(final Supplier resultSupplier){ - this.resultSupplier = resultSupplier; - } - - /** - * get the missing value that corresponds to this option or throw an exception - * @return the appropriate f - * @throws IllegalArgumentException if this should be treated as an error - */ - boolean getMissingValueOrExplode(){ - return resultSupplier.get(); - } - - } - - /** * Method for creating JexlVCMatchExp from input walker arguments names and exps. These two arrays contain * the name associated with each JEXL expression. initializeMatchExps will parse each expression and return * a list of JexlVCMatchExp, in order, that correspond to the names and exps. These are suitable input to diff --git a/src/test/java/htsjdk/variant/variantcontext/VariantJEXLContextUnitTest.java b/src/test/java/htsjdk/variant/variantcontext/VariantJEXLContextUnitTest.java index 0c7418f10..78bf565a7 100644 --- a/src/test/java/htsjdk/variant/variantcontext/VariantJEXLContextUnitTest.java +++ b/src/test/java/htsjdk/variant/variantcontext/VariantJEXLContextUnitTest.java @@ -89,7 +89,7 @@ public void testGetValue() { } @Test(dataProvider = "getMissingValueTestData") - public void testMissingBehaviorThroughMatch(VariantContext vc, VariantContextUtils.JexlMissingValueTreatment missingValueTreatment, boolean expected, Class expectedException){ + public void testMissingBehaviorThroughMatch(VariantContext vc, JexlMissingValueTreatment missingValueTreatment, boolean expected, Class expectedException){ if(expectedException == null) { Assert.assertEquals(VariantContextUtils.match(vc, null, missingValueExpression, missingValueTreatment), expected); } else { @@ -98,7 +98,7 @@ public void testMissingBehaviorThroughMatch(VariantContext vc, VariantContextUti } @Test(dataProvider = "getMissingValueTestData") - public void testMissingBehavior(VariantContext vc, VariantContextUtils.JexlMissingValueTreatment missingValueTreatment, boolean expected, Class expectedException){ + public void testMissingBehavior(VariantContext vc, JexlMissingValueTreatment missingValueTreatment, boolean expected, Class expectedException){ final JEXLMap jexlMap = new JEXLMap(Collections.singletonList(missingValueExpression), vc, null, missingValueTreatment); if(expectedException == null) { Assert.assertEquals((boolean) jexlMap.get(missingValueExpression), expected); @@ -117,12 +117,12 @@ public void testMissingBehavior(VariantContext vc, VariantContextUtils.JexlMissi return new Object[][]{ {noZ, JEXLMap.DEFAULT_MISSING_VALUE_TREATMENT, false, null}, {hasZ, JEXLMap.DEFAULT_MISSING_VALUE_TREATMENT, false, null}, //the value isn't missing but the expression is false - {noZ, VariantContextUtils.JexlMissingValueTreatment.MATCH, true, null}, - {hasZ,VariantContextUtils.JexlMissingValueTreatment.MATCH, false, null}, //the value isn't missing but the expression is false - {noZ, VariantContextUtils.JexlMissingValueTreatment.MISMATCH, false, null}, - {hasZ, VariantContextUtils.JexlMissingValueTreatment.MISMATCH, false, null}, - {noZ, VariantContextUtils.JexlMissingValueTreatment.THROW, false, IllegalArgumentException.class}, - {hasZ, VariantContextUtils.JexlMissingValueTreatment.THROW, false, null} + {noZ, JexlMissingValueTreatment.TREAT_AS_MATCH, true, null}, + {hasZ, JexlMissingValueTreatment.TREAT_AS_MATCH, false, null}, //the value isn't missing but the expression is false + {noZ, JexlMissingValueTreatment.TREAT_AS_MISMATCH, false, null}, + {hasZ, JexlMissingValueTreatment.TREAT_AS_MISMATCH, false, null}, + {noZ, JexlMissingValueTreatment.THROW, false, IllegalArgumentException.class}, + {hasZ, JexlMissingValueTreatment.THROW, false, null} }; } From 8509ba25d0aee67c3888485e48bf7cacd088671b Mon Sep 17 00:00:00 2001 From: Louis Bergelson Date: Fri, 9 Dec 2016 15:49:59 -0500 Subject: [PATCH 8/8] fixing typo --- .../java/htsjdk/variant/variantcontext/JexlMissingValueTreatment.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/htsjdk/variant/variantcontext/JexlMissingValueTreatment.java b/src/main/java/htsjdk/variant/variantcontext/JexlMissingValueTreatment.java index 0e6a554ce..204cc3f2c 100644 --- a/src/main/java/htsjdk/variant/variantcontext/JexlMissingValueTreatment.java +++ b/src/main/java/htsjdk/variant/variantcontext/JexlMissingValueTreatment.java @@ -29,7 +29,7 @@ /** * get the missing value that corresponds to this option or throw an exception - * @return the appropriate f + * @return the value that should be used in case of a missing value * @throws IllegalArgumentException if this should be treated as an error */ boolean getMissingValueOrExplode(){