You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "clintropolis (via GitHub)" <gi...@apache.org> on 2024/11/07 20:55:28 UTC
[PR] projection segment merge fixes (druid)
clintropolis opened a new pull request, #17460:
URL: https://github.com/apache/druid/pull/17460
changes:
* fix issue when merging projections from multiple-incremental persists which was hoping that some 'dim conversion' buffers were not closed, but they already were (by the merging iterator). fix involves selectively persisting these conversion buffers to temp files in the segment write out directory and mapping them and tying them to the segment level closer so that they are available after the lifetime of the parent merger
* modify auto column serializers to use segment write out directory for temp files instead of java.io.tmpdir
* fix queryable index projection to not put the time-like column as a dimension, instead only adding it as __time
--
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] projection segment merge fixes (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #17460:
URL: https://github.com/apache/druid/pull/17460#discussion_r1844300287
##########
processing/src/main/java/org/apache/druid/segment/nested/DictionaryIdLookup.java:
##########
@@ -262,45 +231,73 @@ private void ensureStringDictionaryLoaded()
private void ensureLongDictionaryLoaded()
{
if (longDictionary == null) {
- longDictionaryFile = makeTempFile(name + ColumnSerializerUtils.LONG_DICTIONARY_FILE_NAME);
- longBuffer = SerializerUtils.mapSerializer(longDictionaryFile, longDictionaryWriter);
- longDictionary = FixedIndexed.read(longBuffer, TypeStrategies.LONG, ByteOrder.nativeOrder(), Long.BYTES).get();
- // reset position
- longBuffer.position(0);
+ final String fileName = ColumnSerializerUtils.getInternalFileName(
+ name,
+ ColumnSerializerUtils.LONG_DICTIONARY_FILE_NAME
+ );
+ longDictionaryFile = makeTempDir(fileName);
+ longBufferMapper = closer.register(
+ ColumnSerializerUtils.mapSerializer(longDictionaryFile, longDictionaryWriter, fileName)
+ );
+ try {
+ final ByteBuffer buffer = longBufferMapper.mapFile(fileName);
Review Comment:
All of these seem to be assuming that there will be no secondary smoosh components. Is that a good assumption? Should it be checked? If this a common pattern: maybe add a `mapOnlyFile` to `SmooshFileMapper` that verifies that the mapped file is the only one?
--
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] projection segment merge fixes (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #17460:
URL: https://github.com/apache/druid/pull/17460#discussion_r1844406606
##########
processing/src/main/java/org/apache/druid/segment/nested/DictionaryIdLookup.java:
##########
@@ -262,45 +231,73 @@ private void ensureStringDictionaryLoaded()
private void ensureLongDictionaryLoaded()
{
if (longDictionary == null) {
- longDictionaryFile = makeTempFile(name + ColumnSerializerUtils.LONG_DICTIONARY_FILE_NAME);
- longBuffer = SerializerUtils.mapSerializer(longDictionaryFile, longDictionaryWriter);
- longDictionary = FixedIndexed.read(longBuffer, TypeStrategies.LONG, ByteOrder.nativeOrder(), Long.BYTES).get();
- // reset position
- longBuffer.position(0);
+ final String fileName = ColumnSerializerUtils.getInternalFileName(
+ name,
+ ColumnSerializerUtils.LONG_DICTIONARY_FILE_NAME
+ );
+ longDictionaryFile = makeTempDir(fileName);
+ longBufferMapper = closer.register(
+ ColumnSerializerUtils.mapSerializer(longDictionaryFile, longDictionaryWriter, fileName)
+ );
+ try {
+ final ByteBuffer buffer = longBufferMapper.mapFile(fileName);
Review Comment:
i think this is a fine assumption for now, `FixedIndexed` currently only writes to a single chunk and doesn't support "large" parts that are spread across multiple containers like `GenericIndexed`. Once this changes, we would still map the entry point file container the same way (assuming we are still stuck with ByteBuffer and have to chunk big stuff) and pass in the smoosh mapper to `FixedIndexed.read` similar to how we do for the string case:
```
...
final ByteBuffer stringBuffer = stringBufferMapper.mapFile(fileName);
stringDictionary = StringEncodingStrategies.getStringDictionarySupplier(
stringBufferMapper,
stringBuffer,
ByteOrder.nativeOrder()
).get();
...
```
I could add some checks in case someone updates `FixedIndexedWriter` to allow it to spread across chunks but forgets to update these read call sites, though I imagine that if this is done all of the calls to `FixedIndexed.read` would be updated to take a mapper if we do it in a similar way as `GenericIndexed`.
--
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] projection segment merge fixes (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #17460:
URL: https://github.com/apache/druid/pull/17460#discussion_r1841383680
##########
processing/src/main/java/org/apache/druid/common/utils/SerializerUtils.java:
##########
@@ -224,4 +239,83 @@ public int getSerializedStringByteSize(String str)
{
return Integer.BYTES + StringUtils.toUtf8(str).length;
}
+
+
+ /**
+ * Write a {@link Serializer} to a {@link Path} and then map it as a read-only {@link MappedByteBuffer}
+ */
+ @SuppressFBWarnings("NP_NONNULL_PARAM_VIOLATION")
Review Comment:
IMO, a good approach is to adjust this to pass an actual `FileSmoosher`, and to return `SmooshFileMapper` rather than `MappedByteBuffer`.
--
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] projection segment merge fixes (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #17460:
URL: https://github.com/apache/druid/pull/17460#discussion_r1841057698
##########
processing/src/main/java/org/apache/druid/segment/DimensionMergerV9.java:
##########
@@ -39,6 +39,16 @@ public interface DimensionMergerV9 extends DimensionMerger
*/
ColumnDescriptor makeColumnDescriptor();
+ /**
+ * Sets this merger as the "parent" of another merger for a "projection", allowing for this merger to preserve any
+ * state which might be required for the projection mergers to do their thing. This method MUST be called prior to
+ * performing any merge work
+ */
+ default void markAsParent()
Review Comment:
I found a missing piece for this to "just work" with default implementations, this method is fine doing nothing, but `attachParent` actually needs to be modified to something like
```
default void attachParent(DimensionMergerV9 parent, List<IndexableAdapter> projectionAdapters) throws IOException
{
// by default fall through to writing merged dictionary
writeMergedValueDictionary(projectionAdapters);
}
```
since it is called as an alternative to `writeMergedValueDictionary`, https://github.com/apache/druid/blob/master/processing/src/main/java/org/apache/druid/segment/IndexMergerV9.java#L395, will fix in this PR
--
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] projection segment merge fixes (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #17460:
URL: https://github.com/apache/druid/pull/17460#discussion_r1844406606
##########
processing/src/main/java/org/apache/druid/segment/nested/DictionaryIdLookup.java:
##########
@@ -262,45 +231,73 @@ private void ensureStringDictionaryLoaded()
private void ensureLongDictionaryLoaded()
{
if (longDictionary == null) {
- longDictionaryFile = makeTempFile(name + ColumnSerializerUtils.LONG_DICTIONARY_FILE_NAME);
- longBuffer = SerializerUtils.mapSerializer(longDictionaryFile, longDictionaryWriter);
- longDictionary = FixedIndexed.read(longBuffer, TypeStrategies.LONG, ByteOrder.nativeOrder(), Long.BYTES).get();
- // reset position
- longBuffer.position(0);
+ final String fileName = ColumnSerializerUtils.getInternalFileName(
+ name,
+ ColumnSerializerUtils.LONG_DICTIONARY_FILE_NAME
+ );
+ longDictionaryFile = makeTempDir(fileName);
+ longBufferMapper = closer.register(
+ ColumnSerializerUtils.mapSerializer(longDictionaryFile, longDictionaryWriter, fileName)
+ );
+ try {
+ final ByteBuffer buffer = longBufferMapper.mapFile(fileName);
Review Comment:
i think this is a fine assumption for now, `FixedIndexed` currently only writes to a single chunk and doesn't support "large" parts that are spread across multiple containers like `GenericIndexed`. Once this changes, we would still map the entry point file container the same way (assuming we are still stuck with ByteBuffer and have to chunk big stuff) and pass in the smoosh mapper to `FixedIndexed.read` similar to how we do for the string case:
```
...
final ByteBuffer stringBuffer = stringBufferMapper.mapFile(fileName);
stringDictionary = StringEncodingStrategies.getStringDictionarySupplier(
stringBufferMapper,
stringBuffer,
ByteOrder.nativeOrder()
).get();
...
```
--
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] projection segment merge fixes (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis merged PR #17460:
URL: https://github.com/apache/druid/pull/17460
--
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] projection segment merge fixes (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #17460:
URL: https://github.com/apache/druid/pull/17460#discussion_r1841079053
##########
processing/src/main/java/org/apache/druid/common/utils/SerializerUtils.java:
##########
@@ -224,4 +239,83 @@ public int getSerializedStringByteSize(String str)
{
return Integer.BYTES + StringUtils.toUtf8(str).length;
}
+
+
+ /**
+ * Write a {@link Serializer} to a {@link Path} and then map it as a read-only {@link MappedByteBuffer}
+ */
+ @SuppressFBWarnings("NP_NONNULL_PARAM_VIOLATION")
Review Comment:
the answer to this is a bit complicated, yea some serializers do not require a `FileSmoosher` (basically everything that does not support "large" files greater than 2gb does not need a smoosher). Technically none of the current callers need a smoosher since the new caller is just taking an `IntBuffer` and persisting/mapping it, and the other callers are using `FixedIndexed` which does not support large files yet, but accepting a `Serializer` here is kind of risky for future misuse.
I think I like two options here.
One option is that `mapSerializer` just makes its own `FileSmoosher`/`SmooshedWriter` and just write the serializer into a temporary smoosh at the same name as the temp file name, and then just return the loaded smoosh instead of a mapped byte buffer directly. This is actually already what `DictionaryIdLookup` (where i refactored this code from) is doing for string dictionaries, https://github.com/apache/druid/blob/master/processing/src/main/java/org/apache/druid/segment/nested/DictionaryIdLookup.java#L235 and would let it work with any serializer implementation, and presumably the thing we want to read from the mapped file knows how to load other stuff from the smoosher.
The other option that seems worth considering is as you said, instead of passing in `Serializer` just pass in some other interface that writes with just `WriteableByteChannel` and a length argument with the expectation that its only going to write to the channel and then map the byte buffer and it fits in the buffer. This solves the immediate use case I think, but is sort of limited as long as we are stuck with `ByteBuffer`.
I think I'm sort of leaning towards the 'make a new smoosh' option since it seems the most flexible, its like "make a smoosh file that contains only the contents of this serializer", it does have a little extra overhead for things that do fit in a single buffer, but doesn't seem too bad. Thoughts?
--
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] projection segment merge fixes (druid)
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #17460:
URL: https://github.com/apache/druid/pull/17460#discussion_r1833388289
##########
processing/src/main/java/org/apache/druid/segment/DimensionHandler.java:
##########
@@ -113,16 +134,32 @@
* needed
* @param capabilities The ColumnCapabilities of the dimension represented by this DimensionHandler
* @param progress ProgressIndicator used by the merging process
+ * @param segmentBaseDir segment write out path; temporary files may be created here, though should delete
+ * after merge is finished OR be registered with the Closer parameter
+ * @param closer Closer tied to segment completion. Anything which is not cleaned up inside of the
+ * merger after merge is complete should be registered with this closer. For example,
+ * resources which are required for final serialization of the column
* @return A new DimensionMergerV9 object.
*/
- DimensionMergerV9 makeMerger(
+ default DimensionMergerV9 makeMerger(
String outputName,
IndexSpec indexSpec,
SegmentWriteOutMedium segmentWriteOutMedium,
ColumnCapabilities capabilities,
ProgressIndicator progress,
+ File segmentBaseDir,
Closer closer
- );
+ )
+ {
+ return makeMerger(
+ outputName,
+ indexSpec,
+ segmentWriteOutMedium,
+ capabilities,
+ progress,
+ closer
+ );
Review Comment:
## Deprecated method or constructor invocation
Invoking [DimensionHandler.makeMerger](1) should be avoided because it has been deprecated.
[Show more details](https://github.com/apache/druid/security/code-scanning/8450)
--
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] projection segment merge fixes (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #17460:
URL: https://github.com/apache/druid/pull/17460#discussion_r1840921946
##########
processing/src/main/java/org/apache/druid/segment/DimensionHandler.java:
##########
@@ -100,6 +102,25 @@ default MultiValueHandling getMultivalueHandling()
*/
DimensionIndexer<EncodedType, EncodedKeyComponentType, ActualType> makeIndexer(boolean useMaxMemoryEstimates);
+ /**
+ * @deprecated use {@link #makeMerger(String, IndexSpec, SegmentWriteOutMedium, ColumnCapabilities, ProgressIndicator, File, Closer)}
Review Comment:
Javadoc please about why this still exists. I think it's to avoid breaking `DimensionHandler` impls that exist in extensions.
##########
processing/src/main/java/org/apache/druid/common/utils/SerializerUtils.java:
##########
@@ -224,4 +239,83 @@ public int getSerializedStringByteSize(String str)
{
return Integer.BYTES + StringUtils.toUtf8(str).length;
}
+
+
+ /**
+ * Write a {@link Serializer} to a {@link Path} and then map it as a read-only {@link MappedByteBuffer}
+ */
+ @SuppressFBWarnings("NP_NONNULL_PARAM_VIOLATION")
Review Comment:
`Serializer` is in a package where `EverythingIsNonnullByDefault`. It seems the second arg to `writeTo` can actually be null though. It would be nice to clean this up.
Please consider one of these things:
- Add a `@Nullable` annotation to the second arg, update javadoc describing what happens when the smoosher is null, and remove these warning suppressions. (This one, and the `DataFlowIssue` one.)
- Or, replace `Serializer` here with a more specific subclass or sub-interface that already has `@Nullable` on the second arg. I see that some do.
- Or, update the javadoc for `mapSerializer` to say that not all `Serializer` are supported: the caller must take care to pass in one that supports a `null` Smoosher in `writeTo`.
I'm not sure which approach is best. It depends on whether all `Serializer` are meant to support null `Smoosher` or not.
##########
processing/src/main/java/org/apache/druid/segment/DictionaryEncodedColumnMerger.java:
##########
@@ -101,6 +116,9 @@ public abstract class DictionaryEncodedColumnMerger<T extends Comparable<T>> imp
@Nullable
protected T firstDictionaryValue;
+ protected File segmentBaseDir;
+ @MonotonicNonNull
+ protected PersistedIdConversions persistedIdConversions;
Review Comment:
Javadoc please that this is used for projections.
##########
processing/src/main/java/org/apache/druid/segment/DimensionMergerV9.java:
##########
@@ -39,6 +39,16 @@ public interface DimensionMergerV9 extends DimensionMerger
*/
ColumnDescriptor makeColumnDescriptor();
+ /**
+ * Sets this merger as the "parent" of another merger for a "projection", allowing for this merger to preserve any
+ * state which might be required for the projection mergers to do their thing. This method MUST be called prior to
+ * performing any merge work
+ */
+ default void markAsParent()
Review Comment:
As to types that may exist in extensions: is this default impl ok? Let's say some type exists in an extension with its own dictionaries, and inherits this default impl. What happens then?
--
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] projection segment merge fixes (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #17460:
URL: https://github.com/apache/druid/pull/17460#discussion_r1841055622
##########
processing/src/main/java/org/apache/druid/segment/DimensionHandler.java:
##########
@@ -100,6 +102,25 @@ default MultiValueHandling getMultivalueHandling()
*/
DimensionIndexer<EncodedType, EncodedKeyComponentType, ActualType> makeIndexer(boolean useMaxMemoryEstimates);
+ /**
+ * @deprecated use {@link #makeMerger(String, IndexSpec, SegmentWriteOutMedium, ColumnCapabilities, ProgressIndicator, File, Closer)}
Review Comment:
yea, i'll add, though I do plan to remove this shortly after this PR is merged, just trying to not be disruptive in the event we do a 31.1 (which i'd like to do as mentioned in PR description)
--
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