PARQUET-2432: Use ByteBufferAllocator over hardcoded heap allocation #1278

gszadovszky · 2024-02-21T13:19:32Z

Updated BytesInput implementations to rely on a ByteBufferAllocator instance for allocating/releasing ByteBuffer objects.
Extend the usage of a ByteBufferAllocator instead of the hardcoded usage of heap (e.g. byte[], ByteBuffer.allocate etc.)
parquet-cli related code parts including ParquetRewriter and tests are not changed in this effort

Make sure you have checked all steps below.

Jira

My PR addresses the following PARQUET-2432 issues and references
them in the PR title. For example, "PARQUET-1234: My Parquet PR"
- https://issues.apache.org/jira/browse/PARQUET-XXX
- In case you are adding a dependency, check if the license complies with
  the ASF 3rd Party License Policy.

Tests

My PR adds the following unit tests OR does not need testing for this extremely good reason:

Commits

My commits all reference Jira issues in their subject lines. In addition, my commits follow the guidelines
from "How to write a good git commit message":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters (not including Jira issue reference)
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"

Style

My contribution adheres to the code style guidelines and Spotless passes.
- To apply the necessary changes, run mvn spotless:apply -Pvector-plugins

Documentation

In case of new functionality, my PR adds documentation that describes how to use it.
- All the public functions and the classes in the PR contain Javadoc that explain what it does

* Updated BytesInput implementations to rely on a ByteBufferAllocator instance for allocating/releasing ByteBuffer objects. * Extend the usage of a ByteBufferAllocator instead of the hardcoded usage of heap (e.g. byte[], ByteBuffer.allocate etc.) * parquet-cli related code parts including ParquetRewriter and tests are not changed in this effort

gszadovszky · 2024-02-21T14:11:10Z

@wgtmac, if you have some time, could you check this out?

wgtmac · 2024-02-21T14:52:20Z

Sure, I will take a look by the end of this week.

wgtmac

I didn't check the test thoroughly but this overall LGTM.

wgtmac · 2024-02-23T04:50:01Z

parquet-common/src/main/java/org/apache/parquet/bytes/BytesInput.java

@@ -207,10 +211,18 @@ public static BytesInput copy(BytesInput bytesInput) throws IOException {
   */
  public abstract void writeAllTo(OutputStream out) throws IOException;

+  /**
+   * For internal use only. It is expected that the buffer is large enough to fit the content of this {@link BytesInput}


Should we add a comment for what to expect if the content does not fit into the ByteBuffer?

wgtmac · 2024-02-23T05:22:04Z

parquet-common/src/main/java/org/apache/parquet/bytes/ConcatenatingByteBufferCollector.java

+   * @return a text representation of the memory usage of this structure
+   */
+  public String memUsageString(String prefix) {
+    return format("%s %s %d slabs, %,d bytes", prefix, getClass().getSimpleName(), slabs.size(), size);


Suggested change

return format("%s %s %d slabs, %,d bytes", prefix, getClass().getSimpleName(), slabs.size(), size);

return format("%s %s %d slabs, %d bytes", prefix, getClass().getSimpleName(), slabs.size(), size);

I've just copy-pasted this from ConcatenatingByteArrayCollector but it seems to be intentional. %,d adds separators to the value representation (e.g. 123,456,789).

wgtmac · 2024-02-23T05:22:33Z

parquet-common/src/main/java/org/apache/parquet/bytes/ReusingByteBufferAllocator.java

+import java.nio.ByteBuffer;
+
+/**
+ * A special {@link ByteBufferAllocator} implementation that keeps one {@link ByteBuffer} object and reuse it at the


Suggested change

* A special {@link ByteBufferAllocator} implementation that keeps one {@link ByteBuffer} object and reuse it at the

* A special {@link ByteBufferAllocator} implementation that keeps one {@link ByteBuffer} object and reuses it at the

wgtmac · 2024-02-23T05:32:03Z

parquet-hadoop/src/main/java/org/apache/parquet/hadoop/ColumnChunkPageReadStore.java

-    this.allocator = allocator;
-    this.toRelease = toRelease;
+  void setReleaser(ByteBufferReleaser releaser) {
+    this.releaser = releaser;


Should we check if the passed releaser is null?

This is internal (both the method and the class are package private). I wouldn't do additional checks.

wgtmac · 2024-02-23T05:38:40Z

parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java

    int footerSignatureLength = AesCipher.NONCE_LENGTH + AesCipher.GCM_TAG_LENGTH;
    byte[] serializedFooter = new byte[combinedFooterLength - footerSignatureLength];
-    System.arraycopy(footerAndSignature, 0, serializedFooter, 0, serializedFooter.length);
+    // Resetting to the beginning of the footer
+    from.reset();


Should we check from.markSupported() before calling reset() and mark()?

wgtmac · 2024-02-23T05:44:49Z

parquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetFileWriter.java

+        allocator);
+  }
+
+  @Deprecated


The argument list grows longer now. Should we use an options class instead to avoid frequent deprecation?

From one hand I completely agree. From the other hand, ParquetFileWriter should be an internal class. It is unfortunate that it is public. I would not create yet another parameters builder for ParquetFileWriter.
I'll think about a solution somewhere in between.

gszadovszky · 2024-02-23T16:11:35Z

Thank you, @wgtmac

* Reuse temporary ByteBuffer instead of keep allocating/releasing

cb5bd9c

wgtmac reviewed Feb 23, 2024

View reviewed changes

gszadovszky added 2 commits February 23, 2024 10:05

Addressing comments

f5d9ae6

Fix issues with the previous change

543c189

wgtmac approved these changes Feb 23, 2024

View reviewed changes

gszadovszky merged commit 274dc51 into apache:master Feb 23, 2024
9 checks passed

This was referenced Nov 3, 2024

DictionaryFilter.canDrop may return false positive result when dict size exceeds 8k #3040

Closed

GH-3040: DictionaryFilter.canDrop may return false positive result when dict size exceeds 8k #3041

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PARQUET-2432: Use ByteBufferAllocator over hardcoded heap allocation #1278

PARQUET-2432: Use ByteBufferAllocator over hardcoded heap allocation #1278

gszadovszky commented Feb 21, 2024 •

edited

Loading

gszadovszky commented Feb 21, 2024

wgtmac commented Feb 21, 2024

wgtmac left a comment

wgtmac Feb 23, 2024

wgtmac Feb 23, 2024

gszadovszky Feb 23, 2024

wgtmac Feb 23, 2024

wgtmac Feb 23, 2024

gszadovszky Feb 23, 2024

wgtmac Feb 23, 2024

wgtmac Feb 23, 2024

gszadovszky Feb 23, 2024

gszadovszky commented Feb 23, 2024

	return format("%s %s %d slabs, %,d bytes", prefix, getClass().getSimpleName(), slabs.size(), size);
	return format("%s %s %d slabs, %d bytes", prefix, getClass().getSimpleName(), slabs.size(), size);

	* A special {@link ByteBufferAllocator} implementation that keeps one {@link ByteBuffer} object and reuse it at the
	* A special {@link ByteBufferAllocator} implementation that keeps one {@link ByteBuffer} object and reuses it at the

PARQUET-2432: Use ByteBufferAllocator over hardcoded heap allocation #1278

PARQUET-2432: Use ByteBufferAllocator over hardcoded heap allocation #1278

Conversation

gszadovszky commented Feb 21, 2024 • edited Loading

Jira

Tests

Commits

Style

Documentation

gszadovszky commented Feb 21, 2024

wgtmac commented Feb 21, 2024

wgtmac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gszadovszky commented Feb 23, 2024

gszadovszky commented Feb 21, 2024 •

edited

Loading