Copilot commented on code in PR #18562:
URL: https://github.com/apache/pinot/pull/18562#discussion_r3285623110
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/PercentileSmartTDigestAggregationFunction.java:
##########
@@ -167,15 +167,15 @@ private void aggregateIntoValueList(int length,
AggregationResultHolder aggregat
if (blockValSet.isSingleValue()) {
double[] doubleValues = blockValSet.getDoubleValuesSV();
forEachNotNull(length, blockValSet, (from, toEx) ->
- valueList.addElements(valueList.size(), doubleValues, from, toEx -
from)
+ valueList.addElements(valueList.size(), doubleValues, from, toEx -
from)
);
} else {
double[][] doubleValues = blockValSet.getDoubleValuesMV();
forEachNotNull(length, blockValSet, (from, toEx) -> {
- for (int i = 0; i < length; i++) {
- valueList.addElements(valueList.size(), doubleValues[i]);
- }
+ for (int i = 0; i < length; i++) {
+ valueList.addElements(valueList.size(), doubleValues[i]);
}
+ }
);
Review Comment:
In the multi-value branch of aggregateIntoValueList(), the inner loop
iterates `i` from 0..length for every (from,toEx) range passed by
forEachNotNull. This ignores the (from,toEx) bounds and will add values
multiple times (and can skew results/perf dramatically). Iterate only over the
current non-null range (from..toEx) like the other aggregation paths.
##########
pinot-core/src/test/java/org/apache/pinot/core/operator/docidsets/OrDocIdSetTest.java:
##########
@@ -38,100 +38,100 @@
public class OrDocIdSetTest {
- @Test
+ @Test
public void
iteratorReturnsBitmapDocIdIteratorWhenOnlyIndexBasedIteratorsExist() {
// All the idsets are index-base BlockDocIdIterator
(SortedDocIdIterator or BitmapBasedDocIdIterator)
- SortedDocIdSet sortedDocIdSet = mock(SortedDocIdSet.class);
- SortedDocIdIterator sortedIterator = mock(SortedDocIdIterator.class);
- BitmapDocIdSet bitmapDocIdSet = mock(BitmapDocIdSet.class);
- BitmapDocIdIterator bitmapIterator = mock(BitmapDocIdIterator.class);
-
- when(sortedDocIdSet.iterator()).thenReturn(sortedIterator);
- when(bitmapDocIdSet.iterator()).thenReturn(bitmapIterator);
-
when(sortedIterator.getDocIdRanges()).thenReturn(Collections.singletonList(new
Pairs.IntPair(1, 10)));
- when(bitmapIterator.getDocIds()).thenReturn(new
MutableRoaringBitmap());
-
- List<BlockDocIdSet> docIdSets = Arrays.asList(sortedDocIdSet,
bitmapDocIdSet);
- OrDocIdSet orDocIdSet = new OrDocIdSet(docIdSets, 100);
- BlockDocIdIterator iterator = orDocIdSet.iterator();
+ SortedDocIdSet sortedDocIdSet = mock(SortedDocIdSet.class);
+ SortedDocIdIterator sortedIterator = mock(SortedDocIdIterator.class);
Review Comment:
This test file still has inconsistent indentation (e.g., method
declarations/bodies are indented 4+ spaces under the class). With the new
2-space Indentation check enabled, this will likely fail checkstyle. Please
reformat the methods and their bodies to match the enforced 2-space indentation
(annotation and method at +2, method body at +4, etc.).
##########
pinot-core/src/main/java/org/apache/pinot/core/api/ServiceAutoDiscoveryFeature.java:
##########
@@ -70,25 +70,25 @@
* </code>
*/
public class ServiceAutoDiscoveryFeature implements Feature {
- private static final Logger LOGGER =
LoggerFactory.getLogger(ServiceAutoDiscoveryFeature.class);
+ private static final Logger LOGGER =
LoggerFactory.getLogger(ServiceAutoDiscoveryFeature.class);
- @Inject
+ @Inject
ServiceLocator _serviceLocator;
- @Override
+ @Override
public boolean configure(FeatureContext context) {
- DynamicConfigurationService dcs =
+ DynamicConfigurationService dcs =
_serviceLocator.getService(DynamicConfigurationService.class);
- Populator populator = dcs.getPopulator();
- try {
+ Populator populator = dcs.getPopulator();
+ try {
Review Comment:
Indentation in this class is inconsistent (e.g., the injected field and
configure() method are indented deeper than the class level, and the wrapped
assignment/calls have excessive leading spaces). With the new Indentation check
enabled, this is likely to fail checkstyle; please align members/methods to
2-space indentation and wrap continuation lines using the configured
lineWrappingIndentation.
##########
pinot-core/src/main/java/org/apache/pinot/core/common/ObjectSerDeUtils.java:
##########
@@ -733,21 +733,21 @@ public VarianceTuple deserialize(ByteBuffer byteBuffer) {
public static final ObjectSerDe<PinotFourthMoment>
PINOT_FOURTH_MOMENT_OBJECT_SER_DE
= new ObjectSerDe<PinotFourthMoment>() {
- @Override
+ @Override
public byte[] serialize(PinotFourthMoment value) {
- return value.serialize();
- }
+ return value.serialize();
+ }
- @Override
+ @Override
public PinotFourthMoment deserialize(byte[] bytes) {
Review Comment:
The anonymous ObjectSerDe implementation for
PINOT_FOURTH_MOMENT_OBJECT_SER_DE has inconsistent indentation: the `@Override`
annotations and method signatures are indented differently from the surrounding
anonymous classes. With Indentation enforcement enabled, this block is likely
to fail checkstyle; please align the indentation within the anonymous class
consistently (annotations and methods at the same nesting level).
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/config/QueryWorkloadConfigUtils.java:
##########
@@ -217,12 +217,12 @@ public static List<String>
validateQueryWorkloadConfig(QueryWorkloadConfig confi
} else {
long enforcementCpu = enforcementProfile.getCpuCostNs();
long enforcementMem = enforcementProfile.getMemoryCostBytes();
- if (enforcementCpu <= 0) {
- errors.add(prefix + ".enforcementProfile.cpuCostNs has to
positive");
- }
- if (enforcementMem <= 0) {
- errors.add(prefix + ".enforcementProfile.memoryCostBytes has to
positive");
- }
+ if (enforcementCpu <= 0) {
+ errors.add(prefix + ".enforcementProfile.cpuCostNs has to
positive");
+ }
+ if (enforcementMem <= 0) {
+ errors.add(prefix + ".enforcementProfile.memoryCostBytes has to
positive");
Review Comment:
These validation error messages have grammatical issues: "has to positive"
should be "has to be positive" (or similar). Since these strings are surfaced
to users/operators when configs are invalid, please fix the wording for clarity.
##########
config/checkstyle.xml:
##########
@@ -46,6 +46,13 @@
<module name="AnnotationUseStyle">
<property name="elementStyle" value="ignore"/>
</module>
+ <!-- Method annotations belong on their own line before modifiers -->
+ <module name="AnnotationLocation">
+ <property name="tokens" value="METHOD_DEF"/>
+ <property name="allowSamelineSingleParameterlessAnnotation"
value="false"/>
+ <property name="allowSamelineParameterizedAnnotation" value="false"/>
+ <property name="allowSamelineMultipleAnnotations" value="false"/>
+ </module>
Review Comment:
The PR description focuses on adding the Checkstyle Indentation rule, but
this change also adds an AnnotationLocation rule for METHOD_DEF (disallowing
same-line method annotations). If this is intended, please update the PR
description to mention the additional enforcement so reviewers/users understand
the scope change.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/BaseBooleanAggregationFunction.java:
##########
@@ -120,12 +120,12 @@ public void aggregate(int length, AggregationResultHolder
aggregationResultHolde
Integer aggResult = foldNotNull(length, blockValSet,
aggregationResultHolder.getResult(),
(acum, from, to) -> {
- int innerBool = acum == null ? _merger.getDefaultValue() : acum;
- for (int i = from; i < to; i++) {
- innerBool = _merger.merge(innerBool, bools[i]);
- }
- return innerBool;
- });
+ int innerBool = acum == null ? _merger.getDefaultValue() : acum;
+ for (int i = from; i < to; i++) {
+ innerBool = _merger.merge(innerBool, bools[i]);
+ }
+ return innerBool;
+ });
Review Comment:
The block lambda passed to foldNotNull() is not indented relative to the
opening `{` (the statements inside the lambda align with the lambda line). With
Indentation enforcement enabled (basicOffset=2), the statements inside the
lambda block should be indented one level deeper than the line containing the
`{` to avoid checkstyle violations.
--
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]
