gortiz commented on code in PR #15610:
URL: https://github.com/apache/pinot/pull/15610#discussion_r2055437922
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/sql/fun/PinotOperatorTable.java:
##########
@@ -67,10 +67,11 @@
*/
@SuppressWarnings("unused") // unused fields are accessed by reflection
public class PinotOperatorTable implements SqlOperatorTable {
- private static final Supplier<PinotOperatorTable> INSTANCE =
Suppliers.memoize(PinotOperatorTable::new);
+ /// Lookup map containing instances of this class keyed by configuration.
+ private static final Map<Map<String, String>, PinotOperatorTable> INSTANCES
= new ConcurrentHashMap<>();
Review Comment:
Using maps as keys may be expensive. Given that this has to be called at
least once for each query, we may need to reconsider this implementation.
I was expecting something like named libraries. A library would be a named
collection of operators, and each query could decide which library should be
used (with a default defined at the cluster level). Then we would have a
Map<String, PinotOperatorTable> here.
In the future we can modify the discovery method to be able to annotate
functions to be visible for different libraries, but right now we can modify
this code to use this named library system and just statically support two
different libraries: one where `now` returns long and another where `now`
returns timestamps.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -112,6 +120,10 @@ public class MultiStageBrokerRequestHandler extends
BaseBrokerRequestHandler {
private final MultiStageQueryThrottler _queryThrottler;
private final ExecutorService _queryCompileExecutor;
+ private HelixAdmin _helixAdmin;
+ private HelixConfigScope _helixConfigScope;
+ private volatile Map<String, String> _operatorTableConfig = new HashMap<>();
Review Comment:
not a bug, but having a volatile mutable map looks like a code smell. We
actually want to have an immutable table here. We can make that cheap by
storing it in a Collections.unmodifiableMap (assuming helix will never mutate
the table) or safely by using a Guava ImmutableMap
--
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]
