imply-cheddar commented on code in PR #16431:
URL: https://github.com/apache/druid/pull/16431#discussion_r1604143352
##########
sql/src/test/quidem/org.apache.druid.quidem.SqlQuidemTest/join.iq:
##########
@@ -1,4 +1,4 @@
-!use druidtest://?numMergeBuffers=3
Review Comment:
Why the change in case? keeping camel-case-for-variables as the convention
seems good to me to avoid random errors from people forgetting that they need
to capitalize the first letter in this one place.
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/PlannerConfig.java:
##########
@@ -428,4 +430,28 @@ public PlannerConfig build()
return config;
}
}
+
+ public Map<String, Object> getNonDefaultAsQueryContext()
+ {
+ Map<String, Object> overrides = new HashMap<>();
+ PlannerConfig def = new PlannerConfig();
+ if (def.useApproximateCountDistinct != useApproximateCountDistinct) {
+ overrides.put(
+ CTX_KEY_USE_APPROXIMATE_COUNT_DISTINCT,
+ String.valueOf(useApproximateCountDistinct)
+ );
+ }
+ if (def.useGroupingSetForExactDistinct != useGroupingSetForExactDistinct) {
+ overrides.put(
+ CTX_KEY_USE_GROUPING_SET_FOR_EXACT_DISTINCT,
+ String.valueOf(useGroupingSetForExactDistinct)
+ );
+ }
+
+ PlannerConfig newConfig =
PlannerConfig.builder().withOverrides(overrides).build();
+ if (!equals(newConfig)) {
+ throw new IAE("Some configs are not handled in this method or not
persistable as QueryContext keys!\nold: %s\nnew: %s", this, newConfig);
+ }
Review Comment:
This error message is just dumping the maps, but it's unclear what you are
supposed to do with that? Who is this error message for? Is the expectation
that the maps have the same keys? Which one had a bad key?
Generally speaking, can you make this a `DruidException` with relevant
persona (is this something a user will see?), explain a bit more about what is
expected and what the person is supposed to do?
##########
sql/src/test/java/org/apache/druid/sql/calcite/SqlTestFrameworkConfig.java:
##########
@@ -146,6 +155,13 @@ public Class<? extends QueryComponentSupplier>
fromString(String name) throws Ex
Class<? extends QueryComponentSupplier> value();
}
+ private static final Set<String> KNOWN_CONFIG_KEYS =
ImmutableSet.<String>builder()
+ .add(NumMergeBuffers.class.getSimpleName())
+ .add(MinTopNThreshold.class.getSimpleName())
+ .add(ResultCache.class.getSimpleName())
+ .add(ComponentSupplier.class.getSimpleName())
Review Comment:
I see where the uppercase is coming from. I think it would be better to
make these have a lowercase letter at the beginning, as I think that there will
be more people trying to write these tests then there are adding things here.
If we take a few minutes to create a simple static method that can take the
Class, grab the simple name and lowercase the first letter, these lines can
become
```
.add(toConfigName(NumMergeBuffers.class))
```
Which is equivalently easy to write.
--
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]
