amaechler commented on code in PR #20765:
URL: https://github.com/apache/druid/pull/20765#discussion_r4406686410
##########
extensions-contrib/tdigestsketch/pom.xml:
##########
@@ -56,7 +56,7 @@
<dependency>
<groupId>com.tdunning</groupId>
<artifactId>t-digest</artifactId>
- <version>3.2</version>
+ <version>3.3</version>
Review Comment:
`TDigestSketchUtils.getMaxIntermdiateTDigestSize` says it is "current as of
3.2", and 3.3 changes the sizing: the temp buffer is now `5 * size` (size = `2
* compression + 10`) instead of `5 * compression`. At compression 100 a fresh
`MergingDigest` allocates 24,360 B of arrays on 3.3, compared with 13,360 B on
3.2, while the estimate is 14,360 B. I ran a probe on this branch:
`OnheapIncrementalIndex` counts 14,368 B per row for the sketch, because the
default `factorizeWithSize` uses this estimate. So ingestion with a
`tDigestSketch` metric undercounts by about 10 KB per row and writes to disk
later than `maxBytesInMemory` intends.
Could the formula be updated for 3.3 (and the "as of 3.2" note dropped)? A
guard test like this in `TDigestSketchAggregatorFactoryTest` would catch the
next library change. It fails on this branch (`compression 10: estimate 2480 <
allocated 5800`) and passes with 3.2:
```java
@Test
public void testMaxIntermediateSizeCoversDigestBuffers() throws
IllegalAccessException
{
for (int compression : new int[]{10, 100, 200, 1000}) {
final MergingDigest digest = new MergingDigest(compression);
long arrayBytes = 0;
for (Field field : MergingDigest.class.getDeclaredFields()) {
if (field.getType().isArray() &&
!Modifier.isStatic(field.getModifiers())) {
field.setAccessible(true);
final Object array = field.get(digest);
if (array != null) {
final int elementBytes = field.getType().getComponentType() ==
double.class ? Double.BYTES : Integer.BYTES;
arrayBytes += (long) Array.getLength(array) * elementBytes;
}
}
}
final TDigestSketchAggregatorFactory factory =
new TDigestSketchAggregatorFactory("sketch", "col", compression, new
TDigestConfig(null));
Assertions.assertTrue(
factory.getMaxIntermediateSize() >= arrayBytes,
"compression " + compression + ": estimate " +
factory.getMaxIntermediateSize() + " < allocated " + arrayBytes
);
}
}
```
##########
licenses.yaml:
##########
@@ -4051,7 +4051,7 @@ name: XZ
license_category: binary
module: java-core
license_name: Public Domain
Review Comment:
xz-java has been 0BSD since 1.10 (from its NEWS.md: "From version 1.10
onwards, XZ for Java is under the BSD Zero Clause License (0BSD)"), so this
entry should change too. On master the check doesn't notice, because it skips
licence names it doesn't recognize. #20764 maps `0BSD` to `Zero-Clause BSD`,
but it doesn't touch this entry. So whichever of the two PRs merges second will
fail the licence check with a `Zero-Clause BSD` vs `Public Domain` mismatch.
Could this PR change the entry to:
```yaml
license_name: Zero-Clause BSD
license_file_path: licenses/bin/xz.0BSD
```
and add `licenses/bin/xz.0BSD` with xz-java's `COPYING` text ("Copyright (C)
The XZ for Java authors and contributors" plus the 0BSD terms)? That depends on
the mapping in #20764, so merging #20764 first would keep CI green.
--
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]
