You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "Copilot (via GitHub)" <gi...@apache.org> on 2026/05/22 03:24:11 UTC
Re: [PR] [codex] Enforce Java Checkstyle layout rules [pinot]
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org