Copilot commented on code in PR #17419:
URL: https://github.com/apache/pinot/pull/17419#discussion_r2642984195
##########
pinot-common/src/main/java/org/apache/pinot/common/datatable/StatMap.java:
##########
@@ -166,6 +166,25 @@ public Object getAny(K key) {
}
}
+ /// Returns the value associated with the key name.
+ ///
+ /// In general, it is better to use the type-specific getters with the enum
key directly, but sometimes it is
+ /// impossible or requires complex to read code (like complex unsafe casts).
+ ///
+ /// @param keyName The name of the key.
+ /// @param defaultValue The default value to return if the key is not found.
+ /// @throws ClassCastException if the value cannot be cast to the same
static type as the default value.
+ public <E> E getUnsafe(String keyName, E defaultValue)
Review Comment:
The method name `getUnsafe` is ambiguous. It's unclear whether 'unsafe'
refers to the lack of type safety, potential for exceptions, or unchecked
access. Consider `getByName` or `getByKeyName` to better convey that it
performs string-based lookup.
```suggestion
public <E> E getByKeyName(String keyName, E defaultValue)
```
##########
pinot-common/src/main/java/org/apache/pinot/common/datatable/StatMap.java:
##########
@@ -166,6 +166,25 @@ public Object getAny(K key) {
}
}
+ /// Returns the value associated with the key name.
+ ///
+ /// In general, it is better to use the type-specific getters with the enum
key directly, but sometimes it is
+ /// impossible or requires complex to read code (like complex unsafe casts).
+ ///
+ /// @param keyName The name of the key.
+ /// @param defaultValue The default value to return if the key is not found.
+ /// @throws ClassCastException if the value cannot be cast to the same
static type as the default value.
+ public <E> E getUnsafe(String keyName, E defaultValue)
+ throws ClassCastException {
+ K[] keys = keys();
+ for (K key : keys) {
+ if (key.name().equals(keyName)) {
+ return (E) getAny(key);
+ }
+ }
+ return defaultValue;
+ }
Review Comment:
The linear search through all keys on every call is inefficient. For better
performance, consider caching a map of key names to enum keys during
initialization, especially since this method appears to be called frequently
(once per opchain completion).
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/executor/OpChainSchedulerService.java:
##########
@@ -266,4 +273,24 @@ private int countOperators(MultiStageOperator root) {
private ReadWriteLock getQueryLock(long requestId) {
return _queryLocks[(int) (requestId & QUERY_LOCK_MASK)];
}
+
+ private static class Metrics {
+ private final PinotMeter _startedOpchains =
ServerMeter.MSE_OPCHAINS_STARTED.getGlobalMeter();
+ private final PinotMeter _competedOpchains =
ServerMeter.MSE_OPCHAINS_COMPLETED.getGlobalMeter();
+ private final PinotMeter _emittedRows =
ServerMeter.MSE_EMITTED_ROWS.getGlobalMeter();
+ private final PinotMeter _cpuExecutionTimeMs =
ServerMeter.MSE_CPU_EXECUTION_TIME_MS.getGlobalMeter();
+ private final PinotMeter _memoryAllocatedBytes =
ServerMeter.MSE_MEMORY_ALLOCATED_BYTES.getGlobalMeter();
+
+ public void onOpChainStarted() {
+ _startedOpchains.mark();
+ }
+
+ public void onOpChainFinished(MultiStageOperator rootOperator) {
+ _competedOpchains.mark();
+ StatMap<?> operatorStats = rootOperator.copyStatMaps();
+ _emittedRows.mark(operatorStats.getUnsafe("EMITTED_ROWS", 0L));
+ _cpuExecutionTimeMs.mark(operatorStats.getUnsafe("EXECUTION_TIME_MS",
0L));
+
_memoryAllocatedBytes.mark(operatorStats.getUnsafe("ALLOCATED_MEMORY_BYTES",
0L));
Review Comment:
Hard-coded string keys like 'EMITTED_ROWS', 'EXECUTION_TIME_MS', and
'ALLOCATED_MEMORY_BYTES' are error-prone and difficult to maintain. These
should be defined as constants or enum values to ensure consistency and enable
compile-time checking.
##########
pinot-common/src/main/java/org/apache/pinot/common/datatable/StatMap.java:
##########
@@ -166,6 +166,25 @@ public Object getAny(K key) {
}
}
+ /// Returns the value associated with the key name.
+ ///
+ /// In general, it is better to use the type-specific getters with the enum
key directly, but sometimes it is
+ /// impossible or requires complex to read code (like complex unsafe casts).
+ ///
+ /// @param keyName The name of the key.
+ /// @param defaultValue The default value to return if the key is not found.
+ /// @throws ClassCastException if the value cannot be cast to the same
static type as the default value.
Review Comment:
The method documentation uses `///` style comments but should consistently
use `/** ... */` for public API methods according to Java conventions. The
`@throws` clause is missing from the documentation.
```suggestion
/**
* Returns the value associated with the key name.
* <p>
* In general, it is better to use the type-specific getters with the enum
key directly, but sometimes it is
* impossible or requires complex to read code (like complex unsafe casts).
*
* @param keyName The name of the key.
* @param defaultValue The default value to return if the key is not found.
* @param <E> The static type of the value to return.
* @return The value associated with the key name, or {@code defaultValue}
if the key is not found.
* @throws ClassCastException if the value cannot be cast to the same
static type as the default value.
*/
```
--
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]
