Jackie-Jiang opened a new pull request, #19629: URL: https://github.com/apache/pinot/pull/19629
## Summary `SegmentMetadata.getCrc()` and `getDataCrc()` returned `String`, but the CRC is a `long` everywhere it actually lives: - `SegmentMetadataImpl` reads both from `creation.meta` with `DataInputStream.readLong()` and stores them in `long` fields. - `SegmentZKMetadata.getCrc()` / `getDataCrc()` already return `long`. - `SegmentMetadataImpl.toJson` emits them as JSON numbers. The accessors were the only place rendering that `long` as a `String`, so most callers parsed it straight back — `Long.parseLong(segmentMetadata.getCrc())` appears in the ZK metadata writer, the upload path, the CRC check on segment load, and several minion task executors. This changes both accessors to return `long` and drops the parse at those call sites. `SegmentMetadataImpl` is the only implementation in the repo. ### Boundaries deliberately left as `String` These convert with `String.valueOf` at the call site rather than being widened, because each is a separate contract: - `ValidDocIdsBitmapResponse.segmentCrc` / `segmentDataCrc` and the `segmentCrc` / `segmentDataCrc` fields of the `validDocIdsMetadata` response. These are on the wire between server and minion/controller; changing them would alter response bytes in a mixed-version cluster. - `SegmentDirectoryLoaderContext.setSegmentCrc(String)` — the CRC is used as a path component by tiered-storage segment directory loaders. - `MinionTaskUtils.getValidDocIdFromServerMatchingCrc(..., String expectedCrc, @Nullable String expectedDataCrc, ...)` — its other callers pass a CRC read out of task config, so `String` is the right input type there. - `SegmentDataManager.getCrc()` — backs the `/segments/crc` endpoint, which returns `Map<String, String>`. ### Behavior notes - `SegmentCrcVirtualColumnProvider` loses its `null` check and its `NumberFormatException` fallback; both are unreachable once the accessor is typed. The `Long.MIN_VALUE` "no CRC yet" sentinel for CONSUMING segments is unchanged, so `$crc` still reads as `NULL` there. - `TablesResource.toReportableDataCrc` now takes a `long` and formats, instead of taking a `String` and parsing to range-check it. Same output. - `UpsertCompactMergeTaskExecutor.validateCRCForInputSegments` compares against a `List<String>` built from task config, so it stays a string comparison rather than parsing the expected value — a malformed config entry keeps failing as a CRC mismatch instead of throwing `NumberFormatException`. - Two cases in `UpsertCompactMergeTaskExecutorTest` covered a `null` and an empty-string segment CRC. Neither is representable now, so the `null` one is dropped and the other is repurposed to cover a blank *expected* CRC coming from task config, which is still reachable. No wire format, ZK payload, REST response, or query answer changes. The signature change is source-incompatible for out-of-tree code that implements or calls `SegmentMetadata`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- 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]
