PARQUET-1211: Column indexes: read/write API #456

gszadovszky · 2018-02-13T17:42:48Z

No description provided.

gszadovszky · 2018-02-13T17:47:57Z

Please note that the current parquet-format version is 2.4.1-SNAPSHOT which works only if you build it locally before building parquet. For the final commit we will need a released version of parquet-format that contains my update.

zivanfi

Please change "Parquet 1211" to "PARQUET-1211" in the PR description. I may have more useful suggestions as well once I read through the code. :)

zivanfi · 2018-02-14T13:34:16Z

parquet-column/src/main/java/org/apache/parquet/column/columnindex/ColumnIndexBuilder.java

+public abstract class ColumnIndexBuilder {
+
+  static abstract class ColumnIndexBase implements ColumnIndex {
+    private static ByteBuffer EMPTY_BYTE_BUFFER = ByteBuffer.allocate(0);


legend-hua · 2018-02-23T01:31:22Z

parquet-column/src/test/java/org/apache/parquet/column/columnindex/TestColumnIndexBuilder.java

+            null,
            stringBinary("Slartibartfast")));
    assertEquals(BoundaryOrder.ASCENDING, columnIndex.getBoundaryOrder());
    assertCorrectNullCounts(columnIndex, 1, 2, 3, 4, 5, 6, 7, 8);


The check is passed? why not "assertCorrectNullCounts(columnIndex, 11, 21, 31, 41, 51, 61, 71, 81)"

When the List<Long> is created by using Arrays.asList I have to use long values otherwise a List<Integer> would be created. 1l, 2l etc. are the long literals in java. Others might use capital L as such 1L, 2L etc.

ohhh, I got it wrong. It's letter "l", not number "1".

zivanfi

Only got to OffsetIndexBuilder.java, will continue reviewing from that point later. Only minor nits so far.

zivanfi · 2018-02-26T17:22:03Z

parquet-column/src/main/java/org/apache/parquet/column/columnindex/ColumnIndexBuilder.java

+  static abstract class ColumnIndexBase implements ColumnIndex {
+    private static final ByteBuffer EMPTY_BYTE_BUFFER = ByteBuffer.allocate(0);
+    private static final int MAX_VALUE_LENGTH_FOR_TOSTRING = 40;
+    private static final String INNER_ETC = "(...)";


(nit) I would suggest naming this TOSTRING_TRUNCATION_MARKER.

zivanfi · 2018-02-26T17:27:07Z

parquet-column/src/main/java/org/apache/parquet/column/columnindex/ColumnIndexBuilder.java

+    private static final ByteBuffer EMPTY_BYTE_BUFFER = ByteBuffer.allocate(0);
+    private static final int MAX_VALUE_LENGTH_FOR_TOSTRING = 40;
+    private static final String INNER_ETC = "(...)";
+    private static final int FIRST_LENGTH = (MAX_VALUE_LENGTH_FOR_TOSTRING - INNER_ETC.length()) / 2;


(nit) I would suggest naming this TOSTRING_TRUNCATION_START_POS.

zivanfi · 2018-02-26T17:27:23Z

parquet-column/src/main/java/org/apache/parquet/column/columnindex/ColumnIndexBuilder.java

+    private static final int MAX_VALUE_LENGTH_FOR_TOSTRING = 40;
+    private static final String INNER_ETC = "(...)";
+    private static final int FIRST_LENGTH = (MAX_VALUE_LENGTH_FOR_TOSTRING - INNER_ETC.length()) / 2;
+    private static final int LAST_LENGTH = MAX_VALUE_LENGTH_FOR_TOSTRING - INNER_ETC.length() - FIRST_LENGTH;


(nit) I would suggest naming this TOSTRING_TRUNCATION_END_POS.

zivanfi · 2018-02-26T17:29:15Z

parquet-column/src/main/java/org/apache/parquet/column/columnindex/ColumnIndexBuilder.java

+    public String toString() {
+      try (Formatter formatter = new Formatter()) {
+        formatter.format("Boudary order: %s\n", boundaryOrder);
+        String minMaxPart = "  %-" + MAX_VALUE_LENGTH_FOR_TOSTRING + "s  %-" + MAX_VALUE_LENGTH_FOR_TOSTRING + "s\n";


This looks somewhat scary, but I don't know better either (only in C/C++, where printf supports specifying the desired lengths in parameters).

I don't know a better way either. 😞

zivanfi · 2018-02-26T17:30:31Z

parquet-column/src/main/java/org/apache/parquet/column/columnindex/ColumnIndexBuilder.java

+          String nullCount = nullCounts == null ? "--" : Long.toString(nullCounts[i]);
+          String min, max;
+          if (nullPages[i]) {
+            min = max = "--";


(nit) Replace "--"-s with a constant named TOSTRING_MISSING_VALUE_MARKER or similar.

zivanfi · 2018-02-26T17:39:38Z

parquet-column/src/main/java/org/apache/parquet/column/columnindex/ColumnIndexBuilder.java

+   *          the statistics to be added
+   */
+  public void add(Statistics<?> stats) {
+    if (stats.hasNonNullValue()) {


Does this handle that case correctly when we don't have min/max values in spite of non-null values being present? (For int96-s, for example.)

Currently, we collecting statistics for all the types (even if not supported) only we don't write them to the file. ColumnIndex is working similarly.

zivanfi · 2018-02-26T17:41:03Z

parquet-column/src/main/java/org/apache/parquet/column/columnindex/ColumnIndexBuilder.java

+      List<ByteBuffer> maxValues) {
+    clear();
+    int requiredSize = nullPages.size();
+    if ((nullCounts != null && nullCounts.size() != requiredSize) || minValues.size() != requiredSize


Why is nullCounts checked for being null but minValues and maxValues being used without a similar check?

Okay, I see now, only nullCounts is optional.

zivanfi · 2018-02-26T17:46:29Z

parquet-column/src/main/java/org/apache/parquet/column/columnindex/ColumnIndexBuilder.java

+  }
+
+  /**
+   * Builds the column index. It also resets all the collected data.


I wonder whether the build method resetting the builder is something that API consumers would expect based on its name. Would it make sense to separate the building and the resetting?

zivanfi · 2018-02-26T17:50:01Z

parquet-column/src/main/java/org/apache/parquet/column/columnindex/ColumnIndexBuilder.java

+    }
+  }
+
+  // min_i <= min_i+1 && max_i <= max_i+1


(nit) It is a bit hard to parse at first whether the +1 is in the index. It would be better as:
// min[i] <= min[i+1] && max[i] <= max[i+1]

zivanfi · 2018-02-26T18:09:57Z

parquet-column/src/main/java/org/apache/parquet/column/columnindex/OffsetIndexBuilder.java

+   * @param firstRowIndex
+   *          the index of the first row in the page (within the row group)
+   */
+  public void add(long offset, int compressedPageSize, long firstRowIndex) {


Why is this method public? It seems to me that if one circumvents the other add method that has just 2 parameters, previousRowCount will not be updated thus this add() should only be called by the other add(). Do I miss something?

The two argument add method is used by the writers as that point we don't have the real file offsets. That's why we have the build(long) to shift the related values at build time.
The three argument add method is used by the metadata converter when reading the Parquet file. In this case the build() method is used as already the correct offsets are read and no shifting is required.
I'll add some comments to make it more clear.

legend-hua · 2018-03-23T08:22:14Z

parquet-hadoop/src/test/java/org/apache/parquet/hadoop/TestParquetFileWriter.java

+    w.start();
+    w.startBlock(4);
+    w.startColumn(C1, 7, CODEC);
+    w.writeDataPage(7, 4, BytesInput.from(BYTES3), STATS1, BIT_PACKED, BIT_PACKED, PLAIN);


STATS1? no definition?

BinaryStatistics STATS1 = new BinaryStatistics();

I guess, you've checked my change by rebasing to the actual master where STATS1 and STATS2 are removed and exchanged to EMPTY_STATS. In the current change STATS1 and STATS2 still exist so it is correct as is.
I did not rebase this change yet because it would cause loosing the pointers of the review findings. I'll do the rebase as soon as we'll have the required parquet-format release and this change can be finalized.

Yes, I build the branch which merging your patch, and found the issue.

Thanks for drawing my attention to this one. I'll keep in mind when I'm rebasing.

legend-hua · 2018-03-23T08:22:40Z

parquet-hadoop/src/test/java/org/apache/parquet/hadoop/TestParquetFileWriter.java

+    w.writeDataPage(7, 4, BytesInput.from(BYTES3), STATS1, BIT_PACKED, BIT_PACKED, PLAIN);
+    w.endColumn();
+    w.startColumn(C2, 8, CODEC);
+    w.writeDataPage(8, 4, BytesInput.from(BYTES4), STATS2, BIT_PACKED, BIT_PACKED, PLAIN);


STATS2? no definition?

BinaryStatistics STATS2 = new BinaryStatistics();

See STATS1.

zivanfi · 2018-05-04T12:12:16Z

parquet-column/src/test/java/org/apache/parquet/column/columnindex/TestColumnIndexBuilder.java

+        stringBinary("Slartibartfast"),
+        null,
+        null,
+        stringBinary("Perfect"),


There's a serious typo here: The THHGTTG character is called Prefect, not Perfect. :)

zivanfi · 2018-05-04T12:23:00Z

parquet-column/src/test/java/org/apache/parquet/column/columnindex/TestOffsetIndexBuilder.java

+        3000, 3000, 29,
+        6000, 1200, 56);
+
+    builder = OffsetIndexBuilder.getBuilder();


I may be mistaken, but recreating the builder with the same values seems unnecessary to me.

zivanfi · 2018-05-04T12:24:19Z

parquet-column/src/test/java/org/apache/parquet/column/columnindex/TestOffsetIndexBuilder.java

+    builder.add(5, 6);
+    builder.add(7, 8);
+    assertNull(builder.build());
+    builder.add(1, 2);


Are these repeated adds intentional?

zivanfi · 2018-05-04T12:24:45Z

parquet-column/src/test/java/org/apache/parquet/column/columnindex/TestOffsetIndexBuilder.java

+        48000, 22000, 211,
+        90000, 30000, 361);
+
+    builder = OffsetIndexBuilder.getBuilder();


Like before, this seems unnecessary.

zivanfi · 2018-05-04T12:25:57Z

parquet-column/src/test/java/org/apache/parquet/column/columnindex/TestOffsetIndexBuilder.java

+    builder.add(7, 8, 9);
+    builder.add(10, 11, 12);
+    assertNull(builder.build());
+    builder.add(1, 2, 3);


Seems like unintentional duplication.

zivanfi · 2018-05-08T14:59:27Z

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

+  /**
+   * @param column
+   *          the column chunk which the column index is to be returned for
+   * @return the column index for the specified column chunk or {@code null} if the there is no index


(nit) s/the there/there/ (in this line and in another line below as well)

zivanfi · 2018-05-08T15:24:41Z

...t-hadoop/src/test/java/org/apache/parquet/format/converter/TestParquetMetadataConverter.java

+
+    assertNull("Should handle null column index", ParquetMetadataConverter
+        .toParquetColumnIndex(Types.required(PrimitiveTypeName.INT32).named("test_int32"), null));
+    assertNull("Should handle unsupported types", ParquetMetadataConverter


Should ignore unsupported types.

zivanfi · 2018-05-08T15:26:21Z

parquet-hadoop/src/test/java/org/apache/parquet/hadoop/TestColumnChunkPageWriteStore.java


 public class TestColumnChunkPageWriteStore {

+  // OutputFile implementation to reach out the PositionOutputStream internally used by the writer


s/reach out/expose/

…once

rdblue · 2018-09-27T23:18:29Z

parquet-column/src/main/java/org/apache/parquet/column/page/PageWriter.java

+   * @deprecated will be removed in 2.0.0. This method does not support writing column indexes; Use
+   *             {@link #writePage(BytesInput, int, int, Statistics, Encoding, Encoding, Encoding)} instead
   */
+  @Deprecated


I don't think this requires deprecation. It's part of the column API that is internal.

Unfortunately, it is not documented anywhere and we do know that at least two different projects use it directly (Spark and Hive for vectorization).
I think, it is better to be on the safe side and handle every java-public API members as Public so we won't break any API consumer.

This is a squashed feature branch merge including the changes listed below. The detailed history can be found in the 'column-indexes' branch. * PARQUET-1211: Column indexes: read/write API (#456) * PARQUET-1212: Column indexes: Show indexes in tools (#479) * PARQUET-1213: Column indexes: Limit index size (#480) * PARQUET-1214: Column indexes: Truncate min/max values (#481) * PARQUET-1364: Invalid row indexes for pages starting with nulls (#507) * PARQUET-1310: Column indexes: Filtering (#509) * PARQUET-1386: Fix issues of NaN and +-0.0 in case of float/double column indexes (#515) * PARQUET-1389: Improve value skipping at page synchronization (#514) * PARQUET-1381: Fix missing endRecord after merging columnIndex

ConeyLiu · 2023-03-31T09:27:40Z

...-column/src/main/java/org/apache/parquet/internal/column/columnindex/ColumnIndexBuilder.java

+    }
+
+    @Override
+    int compareMinValues(PrimitiveComparator<Binary> comparator, int index1, int index2) {


Hi @gszadovszky, why does the type of the PrimitiveComparator use Binary for all types? Or just a mistake?

@ConeyLiu, PrimitiveComparator<Binary> covers all potential use case since the only non-java-primitive value type in parquet-mr is Binary and the related methods for these primitive types are part of the PrimitiveComparator interface.

@gszadovszky Thanks for the response. It is a little stranger for IntColumnIndexBuilder uses a comparator typed PrimitiveComparator<Binary>.
https://github.com/apache/parquet-mr/blob/5608695f5777de1eb0899d9075ec9411cfdf31d3/parquet-column/src/main/java/org/apache/parquet/internal/column/columnindex/IntColumnIndexBuilder.java#L123

gszadovszky changed the title ~~Parquet 1211~~ Parquet 1211: Write column indexes: read/write API Feb 13, 2018

zivanfi reviewed Feb 14, 2018

View reviewed changes

gszadovszky changed the title ~~Parquet 1211: Write column indexes: read/write API~~ PARQUET-1211: Write column indexes: read/write API Feb 15, 2018

legend-hua reviewed Feb 23, 2018

View reviewed changes

zivanfi reviewed Feb 26, 2018

View reviewed changes

legend-hua reviewed Mar 23, 2018

View reviewed changes

gszadovszky force-pushed the PARQUET-1211 branch from 6ee8e3b to 7139654 Compare April 18, 2018 12:49

zivanfi reviewed May 4, 2018

View reviewed changes

zivanfi reviewed May 8, 2018

View reviewed changes

zivanfi approved these changes May 9, 2018

View reviewed changes

zivanfi approved these changes May 15, 2018

View reviewed changes

Gabor Szadovszky added 10 commits May 17, 2018 12:02

PARQUET-1211: Write column indexes: read/write API

ef4648a

PARQUET-1211: Additional unit tests + fixes

9b68a3a

PARQUET-1211: Fixes for zi's comments

c94b08e

PARQUET-1211: Update min/max order calculation

fca8dbe

PARQUET-1211: Update for zi's review comments

eb2b8b8

PARQUET-1211: fix invalid page offset when writing a column chunk at …

26f6352

…once

PARQUET-1211: Rebase fixes

cb99da0

PARQUET-1211: Upgrade parquet-format dependency to the latest 2.5.0

ea79ca8

PARQUET-1211: Update for review comments

89d1c3a

PARQUET-1211: Hiding/annotating the internal classes/methods

ab46a98

gszadovszky force-pushed the PARQUET-1211 branch from 77123ff to ab46a98 Compare May 17, 2018 10:03

gszadovszky changed the base branch from master to column-indexes May 17, 2018 10:03

gszadovszky changed the title ~~PARQUET-1211: Write column indexes: read/write API~~ PARQUET-1211: Column indexes: read/write API May 17, 2018

zivanfi merged commit aa571d7 into apache:column-indexes May 17, 2018

rdblue reviewed Sep 27, 2018

View reviewed changes

ConeyLiu reviewed Mar 31, 2023

View reviewed changes

asfimport mentioned this pull request Jun 23, 2024

Column indexes: read/write API #2129

Closed


		public class TestColumnChunkPageWriteStore {

		// OutputFile implementation to reach out the PositionOutputStream internally used by the writer

PARQUET-1211: Column indexes: read/write API #456

PARQUET-1211: Column indexes: read/write API #456

Uh oh!

Conversation

gszadovszky commented Feb 13, 2018

Uh oh!

gszadovszky commented Feb 13, 2018

Uh oh!

zivanfi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zivanfi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!