xiangfu0 commented on PR #18953: URL: https://github.com/apache/pinot/pull/18953#issuecomment-4959023781
@gortiz thanks for the careful read ā the recursion blocker was a real hole. Addressed in `ed8144ac`. **š“ Blocker ā unbounded recursion / `StackOverflowError` ā segment ERROR.** Fixed. `readElement`/`readArray`/`readObject` now thread a depth counter and reject nesting past **1000** (Jackson `StreamReadConstraints`' default) with `IllegalArgumentException`, so deep nesting flows through the normal decode-error handling and drops the one bad record instead of escalating to a `StackOverflowError` that escapes `catch (Exception)` and takes the consuming segment to ERROR. Exactly your diagnosis ā the `Error`-vs-`Exception` distinction is what made it a poison pill. Added `testSqliteRejectsExcessiveNestingDepth` (builds a 1200-deep exactly-filling payload, asserts `IllegalArgumentException`, not SOE; a 50-deep one still decodes), since the prior tests only exercised shallow nesting. **š” Numeric-token length.** Fixed for parity. `BigInteger(String)` is ~O(nĀ²) and was bounded only by payload size; numeric tokens are now capped at **1000** chars (matching Jackson's number-length limit) before the parse runs. Added `testSqliteRejectsOversizedNumericToken` (1000 digits OK ā `BigInteger`, 1001 rejected). **š” Bare `0x0C` ā empty row under `AUTO`.** Documented. Added a line to the README's AUTO caveats noting a stray `0x0C` is a valid exactly-filling empty object and decodes to an empty row under `AUTO` ā pin `TEXT`/your format to avoid it. **š” Trailing bytes after Smile/CBOR.** No change, as you noted ā it matches the existing text-JSON behavior via `JsonUtils.bytesToMap` (no `FAIL_ON_TRAILING_TOKENS`), so tightening only Smile/CBOR would make the binary paths *stricter* than text and inconsistent. Happy to add `FAIL_ON_TRAILING_TOKENS` across all Jackson-backed formats (text included) as a follow-up if you'd prefer that direction. Module is at 72 tests; spotless/checkstyle/license clean. (Two CI compat jobs are red on an unrelated Apache Kafka-tarball mirror download failure in test setup, not this change ā auto-retries have been re-hitting the mirror outage.) _š¤ Addressed by [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]
