xiangfu0 commented on code in PR #20232:
URL: https://github.com/apache/pinot/pull/20232#discussion_r4558076563
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java:
##########
@@ -135,7 +135,7 @@
public abstract class BaseTableDataManager implements TableDataManager {
protected static final Logger LOGGER =
LoggerFactory.getLogger(BaseTableDataManager.class);
- protected final ConcurrentHashMap<String, SegmentDataManager>
_segmentDataManagerMap = new ConcurrentHashMap<>();
+ @io.tesseracta.annotation.TesseractaDoNotLog protected final
ConcurrentHashMap<String, SegmentDataManager> _segmentDataManagerMap = new
ConcurrentHashMap<>();
Review Comment:
This puts a proprietary annotation in production code, so the main code
needs the third-party jar to compile. The line is also 160 characters, which
fails checkstyle `LineLength` and breaks `./mvnw validate`. Please keep
tool-specific annotations out of `src/main`.
AI-generated review via IntentLab · Model: Claude Opus 5.5 (claude-opus-5-5).
##########
pinot-core/pom.xml:
##########
@@ -34,9 +34,40 @@
</properties>
<dependencies>
+ <dependency>
+ <groupId>io.tesseracta</groupId>
+ <artifactId>tesseracta-annotation</artifactId>
Review Comment:
`tesseracta-annotation` is published under a "Proprietary License" (ASF
Category X). At compile scope it flows through `pinot-core` into `pinot-tools`
and `pinot-distribution` (confirmed with `dependency:tree`). Please keep it out
of the default build, for example by declaring it only inside the
`tesseracta-build` profile.
AI-generated review via IntentLab · Model: Claude Opus 5.5 (claude-opus-5-5).
##########
.github/workflows/scripts/pr-tests/.pinot_tests_unit.sh:
##########
@@ -69,24 +69,14 @@ if [ "$RUN_TEST_SET" == "1" ]; then
# No -am on this command, so only the listed modules test.
mvn test ${FORK_OPTS} \
-pl 'pinot-spi' \
- -pl 'pinot-segment-spi' \
- -pl 'pinot-common' \
- -pl ':pinot-yammer' \
- -pl 'pinot-core' \
- -pl 'pinot-query-planner' \
- -pl 'pinot-query-runtime' \
- -P github-actions,no-integration-tests${COVERAGE_PROFILE} || exit 1
+ -Dtest=WorkloadBudgetManagerTest \
Review Comment:
Both unit-test sets now run only `-Dtest=WorkloadBudgetManagerTest`, and set
2 (line 78) excludes `pinot-spi`, which holds the only matching test class. If
merged, Pinot's PR unit-test CI would effectively run one test class.
AI-generated review via IntentLab · Model: Claude Opus 5.5 (claude-opus-5-5).
##########
.github/workflows/pinot_unit_tests.yml:
##########
@@ -43,6 +44,10 @@ on:
- "licenses-binary/**"
- "**.md"
+permissions:
+ id-token: write # This is required for requesting the JWT
Review Comment:
This grants `id-token: write` (permission to request OIDC tokens) to every
job in the workflow, and nothing in this workflow uses it. Together with the
commented-out repository guard (line 92) and the push trigger for a personal
branch (line 26), this shouldn't land in `apache/pinot`.
AI-generated review via IntentLab · Model: Claude Opus 5.5 (claude-opus-5-5).
##########
.github/workflows/scripts/pr-tests/.pinot_tests_build.sh:
##########
@@ -43,14 +43,9 @@ else
-DskipTests -Dcheckstyle.skip -Dspotless.skip -Denforcer.skip
-Dlicense.skip -Dmaven.plugin.appassembler.skip=true \
-am -B -T 16 -ntp \
-P github-actions \
+ -P tesseracta-build \
-pl 'pinot-bom' \
- -pl 'pinot-spi' \
- -pl 'pinot-segment-spi' \
- -pl 'pinot-common' \
- -pl 'pinot-segment-local' \
- -pl 'pinot-core' \
- -pl 'pinot-query-planner' \
- -pl 'pinot-query-runtime' || exit 1
+ -pl 'pinot-spi' || exit 1
Review Comment:
Set 1 now builds only `pinot-bom`/`pinot-spi`, and it uses the experimental
`tesseracta-build` profile (line 46), which replaces the default compile step.
That shouldn't land in the shared PR build script.
AI-generated review via IntentLab · Model: Claude Opus 5.5 (claude-opus-5-5).
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentDedupTest.java:
##########
@@ -231,14 +231,14 @@ public void testDedupWithMetadataTTLWithDedupTimeColumn()
throws Exception {
setup(true, 1000, "dedupTime");
Assert.assertEquals(_mutableSegmentImpl.getNumDocsIndexed(), 2);
- List<Map<String, String>> rawData = loadJsonFile(DATA_FILE_PATH);
+ List<Map<String, Object>> rawData = loadJsonFile(DATA_FILE_PATH);
for (int i = 0; i < 2; i++) {
verifyGeneratedSegmentDataAgainstRawData(i, i, rawData);
}
}
private void verifyGeneratedSegmentDataAgainstRawData(int docId, int
rawDataIndex,
- List<Map<String, String>> rawData) {
+ List<Map<String,Object>> rawData) {
Review Comment:
Checkstyle `WhitespaceAfter` fails here (`Map<String,Object>`). Along with
the same-line `@Test @Tesseracta...` annotations, this makes `./mvnw validate`
fail in `pinot-segment-local`.
AI-generated review via IntentLab · Model: Claude Opus 5.5 (claude-opus-5-5).
--
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]
