kgyrtkirk commented on code in PR #16382:
URL: https://github.com/apache/druid/pull/16382#discussion_r1591898272
##########
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:
I've renamed it.
I believe the reason I've shortened this to `Supplier` was due to the fact
that these could be specified in the jdbc uri; and long key names could take up
a lot of extra line-space - at the cost of readability.
Right now I've "accepted" to follow a convention of only accepting the
configuration key if it fully matches with the annotation's name;
so`druidtest:///?ComponentSupplier=Something` instead of
`druidtest:///?componentsupplier=Something` or
`druidtest:///?Componentsupplier=Something` ; I was wondering about adding the
ability to match the options in an ignore case manner...but writing all this
down....it might be more annoying to typo an option name (whenever its short or
long) and not getting the expected behaviour....maybe I should instead add a
validation for the provided config keys
--
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]
