You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "gianm (via GitHub)" <gi...@apache.org> on 2025/01/31 20:16:15 UTC
[PR] Various fixes for large columns. (druid)
gianm opened a new pull request, #17691:
URL: https://github.com/apache/druid/pull/17691
This patch fixes a class of bugs where various primitive column readers were not providing a SmooshedFileMapper to GenericIndexed, even though the corresponding writer could potentially write multi-file columns. For example, #7943 is an instance of this bug.
This patch also includes a fix for an issue on the writer for compressed multi-value string columns, V3CompressedVSizeColumnarMultiIntsSerializer, where it would use the same base filename for both the offset and values sections. This bug would only be triggered for segments in excess of 500 million rows. When a segment has fewer rows than that, it could potentially have a values section that needs to be split over multiple files, but the offset is never more than 4 bytes per row. This bug was triggered by the new tests, which use a smaller fileSizeLimit.
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Various fixes for large columns. (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #17691:
URL: https://github.com/apache/druid/pull/17691#discussion_r1937970941
##########
extensions-contrib/compressed-bigdecimal/src/main/java/org/apache/druid/compressedbigdecimal/CompressedBigDecimalLongColumnSerializer.java:
##########
@@ -66,7 +67,8 @@ public static CompressedBigDecimalLongColumnSerializer create(
segmentWriteOutMedium,
String.format(Locale.ROOT, "%s.magnitude", filenameBase),
Integer.MAX_VALUE,
- CompressionStrategy.LZ4
+ CompressionStrategy.LZ4,
+ GenericIndexedWriter.MAX_FILE_SIZE
Review Comment:
I thought about this, but this seemed fine enough and makes the `GenericIndexedWriter` API less cluttered. Having two overloads of the method with slightly different arguments seemed like it might be confusing.
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Various fixes for large columns. (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #17691:
URL: https://github.com/apache/druid/pull/17691#discussion_r1938012649
##########
extensions-contrib/compressed-bigdecimal/src/main/java/org/apache/druid/compressedbigdecimal/CompressedBigDecimalLongColumnSerializer.java:
##########
@@ -66,7 +67,8 @@ public static CompressedBigDecimalLongColumnSerializer create(
segmentWriteOutMedium,
String.format(Locale.ROOT, "%s.magnitude", filenameBase),
Integer.MAX_VALUE,
- CompressionStrategy.LZ4
+ CompressionStrategy.LZ4,
+ GenericIndexedWriter.MAX_FILE_SIZE
Review Comment:
I added a javadoc to the `GenericIndexedWriter#ofCompressedByteBuffers` and `V3CompressedVSizeColumnarMultiIntsSerializer#create` methods. There are some other methods that now accept `fileSizeLimit`, but they are package-private so are really only used by tests and by closely-related code. I didn't add javadocs to those since it didn't seem necessary.
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Various fixes for large columns. (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #17691:
URL: https://github.com/apache/druid/pull/17691#discussion_r1937974847
##########
extensions-contrib/compressed-bigdecimal/src/main/java/org/apache/druid/compressedbigdecimal/CompressedBigDecimalLongColumnSerializer.java:
##########
@@ -66,7 +67,8 @@ public static CompressedBigDecimalLongColumnSerializer create(
segmentWriteOutMedium,
String.format(Locale.ROOT, "%s.magnitude", filenameBase),
Integer.MAX_VALUE,
- CompressionStrategy.LZ4
+ CompressionStrategy.LZ4,
+ GenericIndexedWriter.MAX_FILE_SIZE
Review Comment:
yea, i guess new callers of these methods aren’t very frequent so maybe its ok. I was just afraid it just seems more confusing for the production code to be accepting an argument where there is basically one reasonable value to ever pass in, so any new callers should hopefully check around to see what the other callers are doing to pass in the same constant. Maybe we should add javadocs on these create methods to indicate that most callers should specify `GenericIndexedWriter.MAX_FILE_SIZE` as the argument?
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Various fixes for large columns. (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #17691:
URL: https://github.com/apache/druid/pull/17691#discussion_r1939794731
##########
processing/src/main/java/org/apache/druid/segment/data/V3CompressedVSizeColumnarMultiIntsSerializer.java:
##########
@@ -35,33 +35,47 @@ public class V3CompressedVSizeColumnarMultiIntsSerializer extends ColumnarMultiI
{
private static final byte VERSION = V3CompressedVSizeColumnarMultiIntsSupplier.VERSION;
+ /**
+ * Creates a new serializer.
+ *
+ * @param columnName name of the column to write
+ * @param segmentWriteOutMedium supplier of temporary files
+ * @param filenameBase base filename to be used for secondary files, if multiple files are needed
+ * @param maxValue maximum integer value that will be written to the column
+ * @param compression compression strategy to apply
+ * @param fileSizeLimit limit for files created by the writer. In production code, this should always be
+ * {@link GenericIndexedWriter#MAX_FILE_SIZE}. The parameter is exposed only for testing.
+ */
public static V3CompressedVSizeColumnarMultiIntsSerializer create(
final String columnName,
final SegmentWriteOutMedium segmentWriteOutMedium,
final String filenameBase,
final int maxValue,
- final CompressionStrategy compression
+ final CompressionStrategy compression,
+ final int fileSizeLimit
)
{
return new V3CompressedVSizeColumnarMultiIntsSerializer(
columnName,
new CompressedColumnarIntsSerializer(
columnName,
segmentWriteOutMedium,
- filenameBase,
+ filenameBase + ".offsets",
Review Comment:
This was fixing another bug, mentioned in the PR description:
> This patch also includes a fix for an issue on the writer for compressed multi-value string columns, `V3CompressedVSizeColumnarMultiIntsSerializer`, where it would use the same base filename for both the offset and values sections. This bug would only be triggered for segments in excess of 500 million rows. When a segment has fewer rows than that, it could potentially have a values section that needs to be split over multiple files, but the offset is never more than 4 bytes per row. This bug was triggered by the new tests, which use a smaller fileSizeLimit.
Backward compatibility would be OK, because the seconary filenames are written into the primary file. They aren't hard-coded on the reader side. So, older readers will still be able to read these differently-named files.
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Various fixes for large columns. (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #17691:
URL: https://github.com/apache/druid/pull/17691#discussion_r1937974847
##########
extensions-contrib/compressed-bigdecimal/src/main/java/org/apache/druid/compressedbigdecimal/CompressedBigDecimalLongColumnSerializer.java:
##########
@@ -66,7 +67,8 @@ public static CompressedBigDecimalLongColumnSerializer create(
segmentWriteOutMedium,
String.format(Locale.ROOT, "%s.magnitude", filenameBase),
Integer.MAX_VALUE,
- CompressionStrategy.LZ4
+ CompressionStrategy.LZ4,
+ GenericIndexedWriter.MAX_FILE_SIZE
Review Comment:
yea, i guess new callers of these methods aren’t very frequent so maybe its ok. I was just afraid it just seems more confusing for the production code to be accepting an argument where there is basically one reasonable value to ever pass in, so any new callers should hopefully check around to see what the other callers are doing to pass in the same constant. Maybe we should add javadocs on these create methods to indicate that most callers should specify `GenericIndexedWriter.MAX_FILE_SIZE` as the argument except for testing?
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Various fixes for large columns. (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #17691:
URL: https://github.com/apache/druid/pull/17691#discussion_r1939795608
##########
processing/src/main/java/org/apache/druid/segment/data/GenericIndexedWriter.java:
##########
@@ -72,18 +73,31 @@ public class GenericIndexedWriter<T> implements DictionaryWriter<T>
.writeByteArray(x -> x.fileNameByteArray);
+ /**
+ * Creates a new writer that accepts byte buffers and compresses them.
+ *
+ * @param segmentWriteOutMedium supplier of temporary files
+ * @param filenameBase base filename to be used for secondary files, if multiple files are needed
+ * @param compressionStrategy compression strategy to apply
+ * @param bufferSize size of the buffers that will be passed in
+ * @param fileSizeLimit limit for files created by the writer. In production code, this should always be
+ * {@link GenericIndexedWriter#MAX_FILE_SIZE}. The parameter is exposed only for testing.
+ * @param closer closer to attach temporary compression buffers to
+ */
public static GenericIndexedWriter<ByteBuffer> ofCompressedByteBuffers(
final SegmentWriteOutMedium segmentWriteOutMedium,
final String filenameBase,
final CompressionStrategy compressionStrategy,
final int bufferSize,
+ final int fileSizeLimit,
final Closer closer
)
{
GenericIndexedWriter<ByteBuffer> writer = new GenericIndexedWriter<>(
segmentWriteOutMedium,
filenameBase,
- compressedByteBuffersWriteObjectStrategy(compressionStrategy, bufferSize, closer)
+ compressedByteBuffersWriteObjectStrategy(compressionStrategy, bufferSize, closer),
+ fileSizeLimit
Review Comment:
The crux of the fix was passing in a `SmooshedFileMapper` to `GenericIndexed#read` in various call sites. The new `fileSizeLimit` parameter is really just there for testing.
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Various fixes for large columns. (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #17691:
URL: https://github.com/apache/druid/pull/17691#discussion_r1937956015
##########
extensions-contrib/compressed-bigdecimal/src/main/java/org/apache/druid/compressedbigdecimal/CompressedBigDecimalLongColumnSerializer.java:
##########
@@ -66,7 +67,8 @@ public static CompressedBigDecimalLongColumnSerializer create(
segmentWriteOutMedium,
String.format(Locale.ROOT, "%s.magnitude", filenameBase),
Integer.MAX_VALUE,
- CompressionStrategy.LZ4
+ CompressionStrategy.LZ4,
+ GenericIndexedWriter.MAX_FILE_SIZE
Review Comment:
is there any reason to call this in non-test code with any value other than `GenericIndexedWriter.MAX_FILE_SIZE`? like wondering if we should make a version of the creators that automatically passes this argument in and mark the one that takes the size argument as for tests?
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Various fixes for large columns. (druid)
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe commented on code in PR #17691:
URL: https://github.com/apache/druid/pull/17691#discussion_r1938661068
##########
processing/src/main/java/org/apache/druid/segment/data/V3CompressedVSizeColumnarMultiIntsSerializer.java:
##########
@@ -35,33 +35,47 @@ public class V3CompressedVSizeColumnarMultiIntsSerializer extends ColumnarMultiI
{
private static final byte VERSION = V3CompressedVSizeColumnarMultiIntsSupplier.VERSION;
+ /**
+ * Creates a new serializer.
+ *
+ * @param columnName name of the column to write
+ * @param segmentWriteOutMedium supplier of temporary files
+ * @param filenameBase base filename to be used for secondary files, if multiple files are needed
+ * @param maxValue maximum integer value that will be written to the column
+ * @param compression compression strategy to apply
+ * @param fileSizeLimit limit for files created by the writer. In production code, this should always be
+ * {@link GenericIndexedWriter#MAX_FILE_SIZE}. The parameter is exposed only for testing.
+ */
public static V3CompressedVSizeColumnarMultiIntsSerializer create(
final String columnName,
final SegmentWriteOutMedium segmentWriteOutMedium,
final String filenameBase,
final int maxValue,
- final CompressionStrategy compression
+ final CompressionStrategy compression,
+ final int fileSizeLimit
)
{
return new V3CompressedVSizeColumnarMultiIntsSerializer(
columnName,
new CompressedColumnarIntsSerializer(
columnName,
segmentWriteOutMedium,
- filenameBase,
+ filenameBase + ".offsets",
Review Comment:
Could you please explain why the `offsets` and `value` need to be added. Is it `QOL` improvements and not directly related to this patch ?
How does backward compatibility look post this change, ie can older segment reading code still read these files ?
##########
processing/src/main/java/org/apache/druid/segment/data/GenericIndexedWriter.java:
##########
@@ -72,18 +73,31 @@ public class GenericIndexedWriter<T> implements DictionaryWriter<T>
.writeByteArray(x -> x.fileNameByteArray);
+ /**
+ * Creates a new writer that accepts byte buffers and compresses them.
+ *
+ * @param segmentWriteOutMedium supplier of temporary files
+ * @param filenameBase base filename to be used for secondary files, if multiple files are needed
+ * @param compressionStrategy compression strategy to apply
+ * @param bufferSize size of the buffers that will be passed in
+ * @param fileSizeLimit limit for files created by the writer. In production code, this should always be
+ * {@link GenericIndexedWriter#MAX_FILE_SIZE}. The parameter is exposed only for testing.
+ * @param closer closer to attach temporary compression buffers to
+ */
public static GenericIndexedWriter<ByteBuffer> ofCompressedByteBuffers(
final SegmentWriteOutMedium segmentWriteOutMedium,
final String filenameBase,
final CompressionStrategy compressionStrategy,
final int bufferSize,
+ final int fileSizeLimit,
final Closer closer
)
{
GenericIndexedWriter<ByteBuffer> writer = new GenericIndexedWriter<>(
segmentWriteOutMedium,
filenameBase,
- compressedByteBuffersWriteObjectStrategy(compressionStrategy, bufferSize, closer)
+ compressedByteBuffersWriteObjectStrategy(compressionStrategy, bufferSize, closer),
+ fileSizeLimit
Review Comment:
@gianm It looks like this is the crux of the change?
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Various fixes for large columns. (druid)
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #17691:
URL: https://github.com/apache/druid/pull/17691#discussion_r1937997662
##########
processing/src/main/java/org/apache/druid/segment/MetricHolder.java:
##########
@@ -72,7 +79,7 @@
private static <T> GenericIndexed<T> read(ByteBuffer buf, ComplexMetricSerde serde)
{
- return GenericIndexed.read(buf, serde.getObjectStrategy());
+ return GenericIndexed.read(buf, serde.getObjectStrategy(), null);
Review Comment:
## Deprecated method or constructor invocation
Invoking [ComplexMetricSerde.getObjectStrategy](1) should be avoided because it has been deprecated.
[Show more details](https://github.com/apache/druid/security/code-scanning/8662)
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Various fixes for large columns. (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm merged PR #17691:
URL: https://github.com/apache/druid/pull/17691
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org