rohangarg commented on code in PR #16382:
URL: https://github.com/apache/druid/pull/16382#discussion_r1591064457
##########
sql/src/test/java/org/apache/druid/sql/calcite/SqlTestFrameworkConfig.java:
##########
@@ -36,77 +39,200 @@
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;
import java.lang.reflect.Constructor;
+import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
+import java.util.ArrayList;
+import java.util.Arrays;
import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
import java.util.Map;
import java.util.Objects;
+import java.util.Set;
import java.util.function.Function;
/**
- * Annotation to specify desired framework settings.
+ * Specifies current framework settings.
*
- * This class provides junit rule facilities to build the framework accordingly
- * to the annotation. These rules also cache the previously created frameworks.
+ * Intended usage from tests is via the annotations:
+ * @SqlTestFrameworkConfig.MinTopNThreshold(33)
+ *
+ * In case of annotations used; it picks up all annotations from:
+ * * the method
+ * * its enclosing class and its parents
+ * if none contains a specific setting the default is being taken.
+ *
+ * All configurable setting should have:
+ * * an annotation with `value` with the desired type
+ * * the annotation itself should be annotated with itslef to set the
default value
+ * * a field should be added to the main config class
*/
-@Retention(RetentionPolicy.RUNTIME)
-@Target({ElementType.METHOD})
-public @interface SqlTestFrameworkConfig
+public class SqlTestFrameworkConfig
{
- int numMergeBuffers() default 0;
+ @Retention(RetentionPolicy.RUNTIME)
+ @Target({ElementType.METHOD, ElementType.TYPE})
+ @NumMergeBuffers(0)
+ public @interface NumMergeBuffers
+ {
+ int value();
+ }
- int minTopNThreshold() default TopNQueryConfig.DEFAULT_MIN_TOPN_THRESHOLD;
+ @Retention(RetentionPolicy.RUNTIME)
+ @Target({ElementType.METHOD, ElementType.TYPE})
+ @MinTopNThreshold(TopNQueryConfig.DEFAULT_MIN_TOPN_THRESHOLD)
+ public @interface MinTopNThreshold
+ {
+ int value();
+ }
- ResultCacheMode resultCache() default ResultCacheMode.DISABLED;
+ @Retention(RetentionPolicy.RUNTIME)
+ @Target({ElementType.METHOD, ElementType.TYPE})
+ @ResultCache(ResultCacheMode.DISABLED)
+ public @interface ResultCache
+ {
+ ResultCacheMode value();
+ }
/**
- * Non-annotation version of {@link SqlTestFrameworkConfig}.
- *
- * Makes it less convoluted to work with configurations created at runtime.
+ * Declares which {@link QueryComponentSupplier} must be used for the class.
*/
- class SqlTestFrameworkConfigInstance
+ @Retention(RetentionPolicy.RUNTIME)
+ @Target({ElementType.METHOD, ElementType.TYPE})
+ @Supplier(StandardComponentSupplier.class)
+ public @interface Supplier
Review Comment:
should this be named `ComponentSupplier` for clarity?
##########
sql/src/test/java/org/apache/druid/sql/calcite/SqlTestFrameworkConfig.java:
##########
@@ -36,77 +39,200 @@
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;
import java.lang.reflect.Constructor;
+import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
+import java.util.ArrayList;
+import java.util.Arrays;
import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
import java.util.Map;
import java.util.Objects;
+import java.util.Set;
import java.util.function.Function;
/**
- * Annotation to specify desired framework settings.
+ * Specifies current framework settings.
*
- * This class provides junit rule facilities to build the framework accordingly
- * to the annotation. These rules also cache the previously created frameworks.
+ * Intended usage from tests is via the annotations:
+ * @SqlTestFrameworkConfig.MinTopNThreshold(33)
+ *
+ * In case of annotations used; it picks up all annotations from:
+ * * the method
+ * * its enclosing class and its parents
+ * if none contains a specific setting the default is being taken.
+ *
+ * All configurable setting should have:
+ * * an annotation with `value` with the desired type
+ * * the annotation itself should be annotated with itslef to set the
default value
+ * * a field should be added to the main config class
*/
-@Retention(RetentionPolicy.RUNTIME)
-@Target({ElementType.METHOD})
-public @interface SqlTestFrameworkConfig
+public class SqlTestFrameworkConfig
{
- int numMergeBuffers() default 0;
+ @Retention(RetentionPolicy.RUNTIME)
+ @Target({ElementType.METHOD, ElementType.TYPE})
+ @NumMergeBuffers(0)
+ public @interface NumMergeBuffers
+ {
+ int value();
+ }
- int minTopNThreshold() default TopNQueryConfig.DEFAULT_MIN_TOPN_THRESHOLD;
+ @Retention(RetentionPolicy.RUNTIME)
+ @Target({ElementType.METHOD, ElementType.TYPE})
+ @MinTopNThreshold(TopNQueryConfig.DEFAULT_MIN_TOPN_THRESHOLD)
+ public @interface MinTopNThreshold
Review Comment:
is there any plan to support a query context map syntax instead of
annotations per config? that might be helpful in writing newer tests without
changing the core code.
##########
sql/src/test/java/org/apache/druid/sql/calcite/SqlTestFrameworkConfig.java:
##########
@@ -36,77 +39,200 @@
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;
import java.lang.reflect.Constructor;
+import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
+import java.util.ArrayList;
+import java.util.Arrays;
import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
import java.util.Map;
import java.util.Objects;
+import java.util.Set;
import java.util.function.Function;
/**
- * Annotation to specify desired framework settings.
+ * Specifies current framework settings.
*
- * This class provides junit rule facilities to build the framework accordingly
- * to the annotation. These rules also cache the previously created frameworks.
+ * Intended usage from tests is via the annotations:
+ * @SqlTestFrameworkConfig.MinTopNThreshold(33)
+ *
+ * In case of annotations used; it picks up all annotations from:
+ * * the method
+ * * its enclosing class and its parents
+ * if none contains a specific setting the default is being taken.
+ *
+ * All configurable setting should have:
+ * * an annotation with `value` with the desired type
+ * * the annotation itself should be annotated with itslef to set the
default value
+ * * a field should be added to the main config class
*/
-@Retention(RetentionPolicy.RUNTIME)
-@Target({ElementType.METHOD})
-public @interface SqlTestFrameworkConfig
+public class SqlTestFrameworkConfig
{
- int numMergeBuffers() default 0;
+ @Retention(RetentionPolicy.RUNTIME)
+ @Target({ElementType.METHOD, ElementType.TYPE})
+ @NumMergeBuffers(0)
+ public @interface NumMergeBuffers
+ {
+ int value();
+ }
- int minTopNThreshold() default TopNQueryConfig.DEFAULT_MIN_TOPN_THRESHOLD;
+ @Retention(RetentionPolicy.RUNTIME)
+ @Target({ElementType.METHOD, ElementType.TYPE})
+ @MinTopNThreshold(TopNQueryConfig.DEFAULT_MIN_TOPN_THRESHOLD)
+ public @interface MinTopNThreshold
+ {
+ int value();
+ }
- ResultCacheMode resultCache() default ResultCacheMode.DISABLED;
+ @Retention(RetentionPolicy.RUNTIME)
+ @Target({ElementType.METHOD, ElementType.TYPE})
+ @ResultCache(ResultCacheMode.DISABLED)
+ public @interface ResultCache
+ {
+ ResultCacheMode value();
+ }
/**
- * Non-annotation version of {@link SqlTestFrameworkConfig}.
- *
- * Makes it less convoluted to work with configurations created at runtime.
+ * Declares which {@link QueryComponentSupplier} must be used for the class.
*/
- class SqlTestFrameworkConfigInstance
+ @Retention(RetentionPolicy.RUNTIME)
+ @Target({ElementType.METHOD, ElementType.TYPE})
+ @Supplier(StandardComponentSupplier.class)
+ public @interface Supplier
{
- public final int numMergeBuffers;
- public final int minTopNThreshold;
- public final ResultCacheMode resultCache;
+ Class<? extends QueryComponentSupplier> value();
+ }
- public SqlTestFrameworkConfigInstance(SqlTestFrameworkConfig annotation)
- {
- numMergeBuffers = annotation.numMergeBuffers();
- minTopNThreshold = annotation.minTopNThreshold();
- resultCache = annotation.resultCache();
+ public final int numMergeBuffers;
+ public final int minTopNThreshold;
+ public final ResultCacheMode resultCache;
+ public final Class<? extends QueryComponentSupplier> supplier;
+
+ public SqlTestFrameworkConfig(List<Annotation> annotations)
+ {
+ try {
+ numMergeBuffers = getValueFromAnnotation(annotations,
NumMergeBuffers.class);
+ minTopNThreshold = getValueFromAnnotation(annotations,
MinTopNThreshold.class);
+ resultCache = getValueFromAnnotation(annotations, ResultCache.class);
+ supplier = getValueFromAnnotation(annotations, Supplier.class);
+ }
+ catch (NoSuchMethodException | SecurityException | IllegalAccessException
| IllegalArgumentException
+ | InvocationTargetException e) {
+ throw new RuntimeException(e);
}
+ }
- @Override
- public int hashCode()
- {
- return Objects.hash(minTopNThreshold, numMergeBuffers, resultCache);
+ public SqlTestFrameworkConfig(Map<String, String> queryParams)
+ {
+ try {
+ numMergeBuffers = getValueFromMap(queryParams, NumMergeBuffers.class);
+ minTopNThreshold = getValueFromMap(queryParams, MinTopNThreshold.class);
+ resultCache = getValueFromMap(queryParams, ResultCache.class);
+ supplier = getValueFromMap(queryParams, Supplier.class);
}
+ catch (NoSuchMethodException | SecurityException | IllegalAccessException
| IllegalArgumentException
+ | InvocationTargetException e) {
+ throw new RuntimeException(e);
+ }
+ }
- @Override
- public boolean equals(Object obj)
- {
- if (obj == null || getClass() != obj.getClass()) {
- return false;
+ @SuppressWarnings("unchecked")
+ private <T> T getValueFromMap(Map<String, String> map, Class<? extends
Annotation> annotationClass)
+ throws IllegalAccessException, InvocationTargetException,
NoSuchMethodException, SecurityException
+ {
+ String value = map.get(annotationClass.getSimpleName());
+ if (value == null) {
+ return defaultValue(annotationClass);
+ }
+ Class<?> type = annotationClass.getMethod("value").getReturnType();
+
+ if (type == int.class) {
+ return (T) Integer.valueOf(value);
+ }
+ if (type == Class.class) {
+
+ Object clazz = getQueryComponentSupplierForName(value);
+ if (clazz != null) {
+ return (T) clazz;
+ }
+
+ }
+ throw new RuntimeException("don't know how to handle conversion to " +
type);
Review Comment:
maybe `Cannot handle conversion of key [%s] with value [%s] of type [%s]` -
any other wording with the info is also ok to me.
##########
sql/src/test/java/org/apache/druid/sql/calcite/SqlTestFrameworkConfigTest.java:
##########
@@ -20,15 +20,85 @@
package org.apache.druid.sql.calcite;
import nl.jqno.equalsverifier.EqualsVerifier;
+import org.apache.druid.sql.calcite.SqlTestFrameworkConfig.MinTopNThreshold;
+import org.apache.druid.sql.calcite.SqlTestFrameworkConfig.NumMergeBuffers;
+import org.apache.druid.sql.calcite.SqlTestFrameworkConfig.ResultCache;
+import org.apache.druid.sql.calcite.util.CacheTestHelperModule.ResultCacheMode;
import org.junit.jupiter.api.Test;
+import java.lang.annotation.Annotation;
+import java.util.List;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
public class SqlTestFrameworkConfigTest
{
@Test
public void testEquals()
{
-
EqualsVerifier.forClass(SqlTestFrameworkConfig.SqlTestFrameworkConfigInstance.class)
+ EqualsVerifier.forClass(SqlTestFrameworkConfig.class)
.usingGetClass()
.verify();
}
+
+ @ResultCache(ResultCacheMode.ENABLED)
+ static class B
+ {
+ }
+
+ @NumMergeBuffers(3)
+ static class C extends B
+ {
+ @MinTopNThreshold(1)
+ public void imaginaryTestMethod1()
+ {
+ }
+
+ public void imaginaryTestMethod2()
+ {
+ }
+ }
+
+ @MinTopNThreshold(2)
+ static class D extends C
+ {
+ @NumMergeBuffers(1)
Review Comment:
maybe could have another method `imaginaryTestMethod4` which has
annotations `ResultCache(ResultCacheMode.DISABLED)` and `MinTopNThreshold(1)`
##########
sql/src/test/java/org/apache/druid/sql/calcite/SqlTestFrameworkConfig.java:
##########
@@ -36,77 +39,200 @@
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;
import java.lang.reflect.Constructor;
+import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
+import java.util.ArrayList;
+import java.util.Arrays;
import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
import java.util.Map;
import java.util.Objects;
+import java.util.Set;
import java.util.function.Function;
/**
- * Annotation to specify desired framework settings.
+ * Specifies current framework settings.
*
- * This class provides junit rule facilities to build the framework accordingly
- * to the annotation. These rules also cache the previously created frameworks.
+ * Intended usage from tests is via the annotations:
+ * @SqlTestFrameworkConfig.MinTopNThreshold(33)
+ *
+ * In case of annotations used; it picks up all annotations from:
+ * * the method
+ * * its enclosing class and its parents
+ * if none contains a specific setting the default is being taken.
+ *
+ * All configurable setting should have:
+ * * an annotation with `value` with the desired type
+ * * the annotation itself should be annotated with itslef to set the
default value
+ * * a field should be added to the main config class
*/
-@Retention(RetentionPolicy.RUNTIME)
-@Target({ElementType.METHOD})
-public @interface SqlTestFrameworkConfig
+public class SqlTestFrameworkConfig
{
- int numMergeBuffers() default 0;
+ @Retention(RetentionPolicy.RUNTIME)
+ @Target({ElementType.METHOD, ElementType.TYPE})
+ @NumMergeBuffers(0)
+ public @interface NumMergeBuffers
+ {
+ int value();
+ }
- int minTopNThreshold() default TopNQueryConfig.DEFAULT_MIN_TOPN_THRESHOLD;
+ @Retention(RetentionPolicy.RUNTIME)
+ @Target({ElementType.METHOD, ElementType.TYPE})
+ @MinTopNThreshold(TopNQueryConfig.DEFAULT_MIN_TOPN_THRESHOLD)
+ public @interface MinTopNThreshold
+ {
+ int value();
+ }
- ResultCacheMode resultCache() default ResultCacheMode.DISABLED;
+ @Retention(RetentionPolicy.RUNTIME)
+ @Target({ElementType.METHOD, ElementType.TYPE})
+ @ResultCache(ResultCacheMode.DISABLED)
+ public @interface ResultCache
+ {
+ ResultCacheMode value();
+ }
/**
- * Non-annotation version of {@link SqlTestFrameworkConfig}.
- *
- * Makes it less convoluted to work with configurations created at runtime.
+ * Declares which {@link QueryComponentSupplier} must be used for the class.
*/
- class SqlTestFrameworkConfigInstance
+ @Retention(RetentionPolicy.RUNTIME)
+ @Target({ElementType.METHOD, ElementType.TYPE})
+ @Supplier(StandardComponentSupplier.class)
+ public @interface Supplier
{
- public final int numMergeBuffers;
- public final int minTopNThreshold;
- public final ResultCacheMode resultCache;
+ Class<? extends QueryComponentSupplier> value();
+ }
- public SqlTestFrameworkConfigInstance(SqlTestFrameworkConfig annotation)
- {
- numMergeBuffers = annotation.numMergeBuffers();
- minTopNThreshold = annotation.minTopNThreshold();
- resultCache = annotation.resultCache();
+ public final int numMergeBuffers;
+ public final int minTopNThreshold;
+ public final ResultCacheMode resultCache;
+ public final Class<? extends QueryComponentSupplier> supplier;
+
+ public SqlTestFrameworkConfig(List<Annotation> annotations)
+ {
+ try {
+ numMergeBuffers = getValueFromAnnotation(annotations,
NumMergeBuffers.class);
+ minTopNThreshold = getValueFromAnnotation(annotations,
MinTopNThreshold.class);
+ resultCache = getValueFromAnnotation(annotations, ResultCache.class);
+ supplier = getValueFromAnnotation(annotations, Supplier.class);
+ }
+ catch (NoSuchMethodException | SecurityException | IllegalAccessException
| IllegalArgumentException
+ | InvocationTargetException e) {
+ throw new RuntimeException(e);
}
+ }
- @Override
- public int hashCode()
- {
- return Objects.hash(minTopNThreshold, numMergeBuffers, resultCache);
+ public SqlTestFrameworkConfig(Map<String, String> queryParams)
+ {
+ try {
+ numMergeBuffers = getValueFromMap(queryParams, NumMergeBuffers.class);
+ minTopNThreshold = getValueFromMap(queryParams, MinTopNThreshold.class);
+ resultCache = getValueFromMap(queryParams, ResultCache.class);
+ supplier = getValueFromMap(queryParams, Supplier.class);
}
+ catch (NoSuchMethodException | SecurityException | IllegalAccessException
| IllegalArgumentException
+ | InvocationTargetException e) {
+ throw new RuntimeException(e);
+ }
+ }
- @Override
- public boolean equals(Object obj)
- {
- if (obj == null || getClass() != obj.getClass()) {
- return false;
+ @SuppressWarnings("unchecked")
+ private <T> T getValueFromMap(Map<String, String> map, Class<? extends
Annotation> annotationClass)
+ throws IllegalAccessException, InvocationTargetException,
NoSuchMethodException, SecurityException
+ {
+ String value = map.get(annotationClass.getSimpleName());
+ if (value == null) {
+ return defaultValue(annotationClass);
+ }
+ Class<?> type = annotationClass.getMethod("value").getReturnType();
+
+ if (type == int.class) {
+ return (T) Integer.valueOf(value);
+ }
+ if (type == Class.class) {
+
+ Object clazz = getQueryComponentSupplierForName(value);
+ if (clazz != null) {
Review Comment:
can throw with an appropriate error message in else
##########
sql/src/test/java/org/apache/druid/sql/calcite/SqlTestFrameworkConfig.java:
##########
@@ -36,77 +39,200 @@
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;
import java.lang.reflect.Constructor;
+import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
+import java.util.ArrayList;
+import java.util.Arrays;
import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
import java.util.Map;
import java.util.Objects;
+import java.util.Set;
import java.util.function.Function;
/**
- * Annotation to specify desired framework settings.
+ * Specifies current framework settings.
*
- * This class provides junit rule facilities to build the framework accordingly
- * to the annotation. These rules also cache the previously created frameworks.
+ * Intended usage from tests is via the annotations:
+ * @SqlTestFrameworkConfig.MinTopNThreshold(33)
+ *
+ * In case of annotations used; it picks up all annotations from:
+ * * the method
+ * * its enclosing class and its parents
+ * if none contains a specific setting the default is being taken.
+ *
+ * All configurable setting should have:
+ * * an annotation with `value` with the desired type
+ * * the annotation itself should be annotated with itslef to set the
default value
+ * * a field should be added to the main config class
*/
-@Retention(RetentionPolicy.RUNTIME)
-@Target({ElementType.METHOD})
-public @interface SqlTestFrameworkConfig
+public class SqlTestFrameworkConfig
{
- int numMergeBuffers() default 0;
+ @Retention(RetentionPolicy.RUNTIME)
+ @Target({ElementType.METHOD, ElementType.TYPE})
+ @NumMergeBuffers(0)
+ public @interface NumMergeBuffers
+ {
+ int value();
+ }
- int minTopNThreshold() default TopNQueryConfig.DEFAULT_MIN_TOPN_THRESHOLD;
+ @Retention(RetentionPolicy.RUNTIME)
+ @Target({ElementType.METHOD, ElementType.TYPE})
+ @MinTopNThreshold(TopNQueryConfig.DEFAULT_MIN_TOPN_THRESHOLD)
+ public @interface MinTopNThreshold
+ {
+ int value();
+ }
- ResultCacheMode resultCache() default ResultCacheMode.DISABLED;
+ @Retention(RetentionPolicy.RUNTIME)
+ @Target({ElementType.METHOD, ElementType.TYPE})
+ @ResultCache(ResultCacheMode.DISABLED)
+ public @interface ResultCache
+ {
+ ResultCacheMode value();
+ }
/**
- * Non-annotation version of {@link SqlTestFrameworkConfig}.
- *
- * Makes it less convoluted to work with configurations created at runtime.
+ * Declares which {@link QueryComponentSupplier} must be used for the class.
*/
- class SqlTestFrameworkConfigInstance
+ @Retention(RetentionPolicy.RUNTIME)
+ @Target({ElementType.METHOD, ElementType.TYPE})
+ @Supplier(StandardComponentSupplier.class)
+ public @interface Supplier
{
- public final int numMergeBuffers;
- public final int minTopNThreshold;
- public final ResultCacheMode resultCache;
+ Class<? extends QueryComponentSupplier> value();
+ }
- public SqlTestFrameworkConfigInstance(SqlTestFrameworkConfig annotation)
- {
- numMergeBuffers = annotation.numMergeBuffers();
- minTopNThreshold = annotation.minTopNThreshold();
- resultCache = annotation.resultCache();
+ public final int numMergeBuffers;
+ public final int minTopNThreshold;
+ public final ResultCacheMode resultCache;
+ public final Class<? extends QueryComponentSupplier> supplier;
+
+ public SqlTestFrameworkConfig(List<Annotation> annotations)
+ {
+ try {
+ numMergeBuffers = getValueFromAnnotation(annotations,
NumMergeBuffers.class);
+ minTopNThreshold = getValueFromAnnotation(annotations,
MinTopNThreshold.class);
+ resultCache = getValueFromAnnotation(annotations, ResultCache.class);
+ supplier = getValueFromAnnotation(annotations, Supplier.class);
+ }
+ catch (NoSuchMethodException | SecurityException | IllegalAccessException
| IllegalArgumentException
+ | InvocationTargetException e) {
+ throw new RuntimeException(e);
}
+ }
- @Override
- public int hashCode()
- {
- return Objects.hash(minTopNThreshold, numMergeBuffers, resultCache);
+ public SqlTestFrameworkConfig(Map<String, String> queryParams)
+ {
+ try {
+ numMergeBuffers = getValueFromMap(queryParams, NumMergeBuffers.class);
+ minTopNThreshold = getValueFromMap(queryParams, MinTopNThreshold.class);
+ resultCache = getValueFromMap(queryParams, ResultCache.class);
+ supplier = getValueFromMap(queryParams, Supplier.class);
}
+ catch (NoSuchMethodException | SecurityException | IllegalAccessException
| IllegalArgumentException
+ | InvocationTargetException e) {
+ throw new RuntimeException(e);
+ }
+ }
- @Override
- public boolean equals(Object obj)
- {
- if (obj == null || getClass() != obj.getClass()) {
- return false;
+ @SuppressWarnings("unchecked")
+ private <T> T getValueFromMap(Map<String, String> map, Class<? extends
Annotation> annotationClass)
+ throws IllegalAccessException, InvocationTargetException,
NoSuchMethodException, SecurityException
+ {
+ String value = map.get(annotationClass.getSimpleName());
+ if (value == null) {
+ return defaultValue(annotationClass);
+ }
+ Class<?> type = annotationClass.getMethod("value").getReturnType();
+
+ if (type == int.class) {
+ return (T) Integer.valueOf(value);
+ }
+ if (type == Class.class) {
+
+ Object clazz = getQueryComponentSupplierForName(value);
+ if (clazz != null) {
+ return (T) clazz;
+ }
+
+ }
+ throw new RuntimeException("don't know how to handle conversion to " +
type);
+ }
+
+ private Object getQueryComponentSupplierForName(String name)
+ {
+ Set<Class<? extends QueryComponentSupplier>> subTypes = new
Reflections("org.apache.druid")
+ .getSubTypesOf(QueryComponentSupplier.class);
+ Set<String> knownNames = new HashSet<String>();
+
+ for (Class<? extends QueryComponentSupplier> cl : subTypes) {
+ if (cl.getSimpleName().equals(name)) {
+ return cl;
+ }
+ knownNames.add(cl.getSimpleName());
+ }
+ throw new IAE("supplier [%s] is not known; known are [%s]", name,
knownNames);
Review Comment:
suggestion for better wording - like using `Query component supplier`
instead of `supplier`
##########
sql/src/test/java/org/apache/druid/sql/calcite/SqlTestFrameworkConfig.java:
##########
@@ -36,77 +39,200 @@
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;
import java.lang.reflect.Constructor;
+import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
+import java.util.ArrayList;
+import java.util.Arrays;
import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
import java.util.Map;
import java.util.Objects;
+import java.util.Set;
import java.util.function.Function;
/**
- * Annotation to specify desired framework settings.
+ * Specifies current framework settings.
*
- * This class provides junit rule facilities to build the framework accordingly
- * to the annotation. These rules also cache the previously created frameworks.
+ * Intended usage from tests is via the annotations:
+ * @SqlTestFrameworkConfig.MinTopNThreshold(33)
+ *
+ * In case of annotations used; it picks up all annotations from:
+ * * the method
+ * * its enclosing class and its parents
+ * if none contains a specific setting the default is being taken.
+ *
+ * All configurable setting should have:
+ * * an annotation with `value` with the desired type
+ * * the annotation itself should be annotated with itslef to set the
default value
+ * * a field should be added to the main config class
*/
-@Retention(RetentionPolicy.RUNTIME)
-@Target({ElementType.METHOD})
-public @interface SqlTestFrameworkConfig
+public class SqlTestFrameworkConfig
{
- int numMergeBuffers() default 0;
+ @Retention(RetentionPolicy.RUNTIME)
+ @Target({ElementType.METHOD, ElementType.TYPE})
+ @NumMergeBuffers(0)
+ public @interface NumMergeBuffers
+ {
+ int value();
+ }
- int minTopNThreshold() default TopNQueryConfig.DEFAULT_MIN_TOPN_THRESHOLD;
+ @Retention(RetentionPolicy.RUNTIME)
+ @Target({ElementType.METHOD, ElementType.TYPE})
+ @MinTopNThreshold(TopNQueryConfig.DEFAULT_MIN_TOPN_THRESHOLD)
+ public @interface MinTopNThreshold
+ {
+ int value();
+ }
- ResultCacheMode resultCache() default ResultCacheMode.DISABLED;
+ @Retention(RetentionPolicy.RUNTIME)
+ @Target({ElementType.METHOD, ElementType.TYPE})
+ @ResultCache(ResultCacheMode.DISABLED)
+ public @interface ResultCache
+ {
+ ResultCacheMode value();
+ }
/**
- * Non-annotation version of {@link SqlTestFrameworkConfig}.
- *
- * Makes it less convoluted to work with configurations created at runtime.
+ * Declares which {@link QueryComponentSupplier} must be used for the class.
*/
- class SqlTestFrameworkConfigInstance
+ @Retention(RetentionPolicy.RUNTIME)
+ @Target({ElementType.METHOD, ElementType.TYPE})
+ @Supplier(StandardComponentSupplier.class)
+ public @interface Supplier
{
- public final int numMergeBuffers;
- public final int minTopNThreshold;
- public final ResultCacheMode resultCache;
+ Class<? extends QueryComponentSupplier> value();
+ }
- public SqlTestFrameworkConfigInstance(SqlTestFrameworkConfig annotation)
- {
- numMergeBuffers = annotation.numMergeBuffers();
- minTopNThreshold = annotation.minTopNThreshold();
- resultCache = annotation.resultCache();
+ public final int numMergeBuffers;
+ public final int minTopNThreshold;
+ public final ResultCacheMode resultCache;
+ public final Class<? extends QueryComponentSupplier> supplier;
+
+ public SqlTestFrameworkConfig(List<Annotation> annotations)
+ {
+ try {
+ numMergeBuffers = getValueFromAnnotation(annotations,
NumMergeBuffers.class);
+ minTopNThreshold = getValueFromAnnotation(annotations,
MinTopNThreshold.class);
+ resultCache = getValueFromAnnotation(annotations, ResultCache.class);
+ supplier = getValueFromAnnotation(annotations, Supplier.class);
+ }
+ catch (NoSuchMethodException | SecurityException | IllegalAccessException
| IllegalArgumentException
+ | InvocationTargetException e) {
+ throw new RuntimeException(e);
}
+ }
- @Override
- public int hashCode()
- {
- return Objects.hash(minTopNThreshold, numMergeBuffers, resultCache);
+ public SqlTestFrameworkConfig(Map<String, String> queryParams)
+ {
+ try {
+ numMergeBuffers = getValueFromMap(queryParams, NumMergeBuffers.class);
+ minTopNThreshold = getValueFromMap(queryParams, MinTopNThreshold.class);
+ resultCache = getValueFromMap(queryParams, ResultCache.class);
+ supplier = getValueFromMap(queryParams, Supplier.class);
}
+ catch (NoSuchMethodException | SecurityException | IllegalAccessException
| IllegalArgumentException
+ | InvocationTargetException e) {
+ throw new RuntimeException(e);
+ }
+ }
- @Override
- public boolean equals(Object obj)
- {
- if (obj == null || getClass() != obj.getClass()) {
- return false;
+ @SuppressWarnings("unchecked")
+ private <T> T getValueFromMap(Map<String, String> map, Class<? extends
Annotation> annotationClass)
+ throws IllegalAccessException, InvocationTargetException,
NoSuchMethodException, SecurityException
+ {
+ String value = map.get(annotationClass.getSimpleName());
+ if (value == null) {
+ return defaultValue(annotationClass);
+ }
+ Class<?> type = annotationClass.getMethod("value").getReturnType();
+
+ if (type == int.class) {
+ return (T) Integer.valueOf(value);
+ }
+ if (type == Class.class) {
+
+ Object clazz = getQueryComponentSupplierForName(value);
+ if (clazz != null) {
+ return (T) clazz;
+ }
+
+ }
+ throw new RuntimeException("don't know how to handle conversion to " +
type);
+ }
+
+ private Object getQueryComponentSupplierForName(String name)
+ {
+ Set<Class<? extends QueryComponentSupplier>> subTypes = new
Reflections("org.apache.druid")
Review Comment:
this would not support a component supplier defined in an extension which
has a proprietary package name.
also, we should cache this set probably
##########
sql/src/test/java/org/apache/druid/sql/calcite/SqlTestFrameworkConfig.java:
##########
@@ -169,40 +280,25 @@ public void afterAll(ExtensionContext context)
@Override
public void beforeEach(ExtensionContext context)
{
- method = context.getTestMethod().get();
- setConfig(method.getAnnotation(SqlTestFrameworkConfig.class));
+ setConfig(context);
}
- @SqlTestFrameworkConfig
- public SqlTestFrameworkConfig defaultConfig()
+ private void setConfig(ExtensionContext context)
{
- try {
- SqlTestFrameworkConfig annotation = getClass()
- .getMethod("defaultConfig")
- .getAnnotation(SqlTestFrameworkConfig.class);
- return annotation;
- }
- catch (NoSuchMethodException | SecurityException e) {
- throw new RuntimeException(e);
- }
- }
-
- private void setConfig(SqlTestFrameworkConfig annotation)
- {
- if (annotation == null) {
- annotation = defaultConfig();
- }
- config = new SqlTestFrameworkConfigInstance(annotation);
+ method = context.getTestMethod().get();
+ Class<?> testClass = context.getTestClass().get();
+ List<Annotation> annotations = collectAnnotations(testClass, method);
+ config = new SqlTestFrameworkConfig(annotations);
}
- public SqlTestFrameworkConfigInstance getConfig()
+ public SqlTestFrameworkConfig getConfig()
{
return config;
}
- public SqlTestFramework get()
+ public SqlTestFramework get() throws Exception
{
- return configStore.getConfigurationInstance(config,
testHostSupplier).framework;
+ return configStore.getConfigurationInstance(config, x -> x).framework;
Review Comment:
maybe we can eliminate the identity lambda?
##########
sql/src/test/java/org/apache/druid/sql/calcite/SqlTestFrameworkConfig.java:
##########
@@ -36,77 +39,200 @@
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;
import java.lang.reflect.Constructor;
+import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
+import java.util.ArrayList;
+import java.util.Arrays;
import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
import java.util.Map;
import java.util.Objects;
+import java.util.Set;
import java.util.function.Function;
/**
- * Annotation to specify desired framework settings.
+ * Specifies current framework settings.
*
- * This class provides junit rule facilities to build the framework accordingly
- * to the annotation. These rules also cache the previously created frameworks.
+ * Intended usage from tests is via the annotations:
+ * @SqlTestFrameworkConfig.MinTopNThreshold(33)
+ *
+ * In case of annotations used; it picks up all annotations from:
+ * * the method
+ * * its enclosing class and its parents
+ * if none contains a specific setting the default is being taken.
+ *
+ * All configurable setting should have:
+ * * an annotation with `value` with the desired type
+ * * the annotation itself should be annotated with itslef to set the
default value
+ * * a field should be added to the main config class
*/
-@Retention(RetentionPolicy.RUNTIME)
-@Target({ElementType.METHOD})
-public @interface SqlTestFrameworkConfig
+public class SqlTestFrameworkConfig
{
- int numMergeBuffers() default 0;
+ @Retention(RetentionPolicy.RUNTIME)
+ @Target({ElementType.METHOD, ElementType.TYPE})
+ @NumMergeBuffers(0)
+ public @interface NumMergeBuffers
+ {
+ int value();
+ }
- int minTopNThreshold() default TopNQueryConfig.DEFAULT_MIN_TOPN_THRESHOLD;
+ @Retention(RetentionPolicy.RUNTIME)
+ @Target({ElementType.METHOD, ElementType.TYPE})
+ @MinTopNThreshold(TopNQueryConfig.DEFAULT_MIN_TOPN_THRESHOLD)
+ public @interface MinTopNThreshold
+ {
+ int value();
+ }
- ResultCacheMode resultCache() default ResultCacheMode.DISABLED;
+ @Retention(RetentionPolicy.RUNTIME)
+ @Target({ElementType.METHOD, ElementType.TYPE})
+ @ResultCache(ResultCacheMode.DISABLED)
+ public @interface ResultCache
+ {
+ ResultCacheMode value();
+ }
/**
- * Non-annotation version of {@link SqlTestFrameworkConfig}.
- *
- * Makes it less convoluted to work with configurations created at runtime.
+ * Declares which {@link QueryComponentSupplier} must be used for the class.
*/
- class SqlTestFrameworkConfigInstance
+ @Retention(RetentionPolicy.RUNTIME)
+ @Target({ElementType.METHOD, ElementType.TYPE})
+ @Supplier(StandardComponentSupplier.class)
+ public @interface Supplier
{
- public final int numMergeBuffers;
- public final int minTopNThreshold;
- public final ResultCacheMode resultCache;
+ Class<? extends QueryComponentSupplier> value();
+ }
- public SqlTestFrameworkConfigInstance(SqlTestFrameworkConfig annotation)
- {
- numMergeBuffers = annotation.numMergeBuffers();
- minTopNThreshold = annotation.minTopNThreshold();
- resultCache = annotation.resultCache();
+ public final int numMergeBuffers;
+ public final int minTopNThreshold;
+ public final ResultCacheMode resultCache;
+ public final Class<? extends QueryComponentSupplier> supplier;
+
+ public SqlTestFrameworkConfig(List<Annotation> annotations)
+ {
+ try {
+ numMergeBuffers = getValueFromAnnotation(annotations,
NumMergeBuffers.class);
+ minTopNThreshold = getValueFromAnnotation(annotations,
MinTopNThreshold.class);
+ resultCache = getValueFromAnnotation(annotations, ResultCache.class);
+ supplier = getValueFromAnnotation(annotations, Supplier.class);
+ }
+ catch (NoSuchMethodException | SecurityException | IllegalAccessException
| IllegalArgumentException
+ | InvocationTargetException e) {
+ throw new RuntimeException(e);
}
+ }
- @Override
- public int hashCode()
- {
- return Objects.hash(minTopNThreshold, numMergeBuffers, resultCache);
+ public SqlTestFrameworkConfig(Map<String, String> queryParams)
+ {
+ try {
+ numMergeBuffers = getValueFromMap(queryParams, NumMergeBuffers.class);
+ minTopNThreshold = getValueFromMap(queryParams, MinTopNThreshold.class);
+ resultCache = getValueFromMap(queryParams, ResultCache.class);
+ supplier = getValueFromMap(queryParams, Supplier.class);
}
+ catch (NoSuchMethodException | SecurityException | IllegalAccessException
| IllegalArgumentException
+ | InvocationTargetException e) {
+ throw new RuntimeException(e);
+ }
+ }
- @Override
- public boolean equals(Object obj)
- {
- if (obj == null || getClass() != obj.getClass()) {
- return false;
+ @SuppressWarnings("unchecked")
+ private <T> T getValueFromMap(Map<String, String> map, Class<? extends
Annotation> annotationClass)
+ throws IllegalAccessException, InvocationTargetException,
NoSuchMethodException, SecurityException
+ {
+ String value = map.get(annotationClass.getSimpleName());
+ if (value == null) {
+ return defaultValue(annotationClass);
+ }
+ Class<?> type = annotationClass.getMethod("value").getReturnType();
+
+ if (type == int.class) {
Review Comment:
unfortunately, we don't have the serde logic of each property attached to
the property object - rather that is handled by the `QueryContext` class.
I think maybe taking input a whole `QueryContext` might be helpful with
managing these serde too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
