gaborkaszab commented on code in PR #16317:
URL: https://github.com/apache/iceberg/pull/16317#discussion_r3577566094
##########
parquet/src/test/java/org/apache/iceberg/parquet/TestParquet.java:
##########
@@ -23,6 +23,7 @@
import static
org.apache.iceberg.TableProperties.PARQUET_COLUMN_STATS_ENABLED_PREFIX;
import static
org.apache.iceberg.TableProperties.PARQUET_ROW_GROUP_CHECK_MAX_RECORD_COUNT;
import static
org.apache.iceberg.TableProperties.PARQUET_ROW_GROUP_CHECK_MIN_RECORD_COUNT;
+import static org.apache.iceberg.TableProperties.PARQUET_ROW_GROUP_ROW_LIMIT;
Review Comment:
The delete counterpart is not covered with tests
##########
parquet/src/main/java/org/apache/iceberg/parquet/ParquetWriter.java:
##########
@@ -204,6 +204,12 @@ public List<Long> splitOffsets() {
}
private void checkSize() {
+ // This comparison is cheap, so we don't need the "spacing out checks"
logic below.
+ if (recordCount >= props.getRowGroupRowCountLimit()) {
+ flushRowGroup(false);
+ return;
Review Comment:
instead of return here, make the `if` on line 213 an `else if` to follow the
existing pattern in this function
##########
docs/docs/configuration.md:
##########
@@ -48,6 +48,7 @@ Iceberg tables support table properties to configure table
behavior, like the de
| write.parquet.page-size-bytes | 1048576 (1 MB)
| Parquet page size
|
| write.parquet.page-version | v1
| Parquet data page version: v1 (DataPage V1) or v2 (DataPage V2)
|
| write.parquet.page-row-limit | 20000
| Parquet page row limit
|
+| write.parquet.row-group-row-limit | Integer.MAX_VALUE (no
limit) | Parquet row group row count limit; row groups are flushed once this
many rows have been written
|
Review Comment:
The delete counterpart is not added to the doc
##########
parquet/src/main/java/org/apache/iceberg/parquet/ParquetWriter.java:
##########
@@ -204,6 +204,12 @@ public List<Long> splitOffsets() {
}
private void checkSize() {
+ // This comparison is cheap, so we don't need the "spacing out checks"
logic below.
Review Comment:
I don't think we need this comment
##########
parquet/src/test/java/org/apache/iceberg/parquet/TestParquet.java:
##########
@@ -114,6 +115,38 @@ public void testRowGroupSizeConfigurableWithWriter()
throws IOException {
}
}
+ @Test
+ public void rowGroupRowLimitConfigurable() throws IOException {
+ Schema schema = new Schema(optional(1, "intCol", IntegerType.get()));
+
+ int recordCount = 25;
+ int rowGroupRowLimit = 10;
+
+ List<GenericData.Record> records =
Lists.newArrayListWithCapacity(recordCount);
+ org.apache.avro.Schema avroSchema =
AvroSchemaUtil.convert(schema.asStruct());
+ for (int i = 1; i <= recordCount; i++) {
+ GenericData.Record record = new GenericData.Record(avroSchema);
+ record.put("intCol", i);
+ records.add(record);
+ }
+
+ File file = createTempFile(temp);
+ write(
+ file,
+ schema,
+ ImmutableMap.of(PARQUET_ROW_GROUP_ROW_LIMIT,
Integer.toString(rowGroupRowLimit)),
+ ParquetAvroWriter::buildWriter,
+ records.toArray(new GenericData.Record[] {}));
+
+ try (ParquetFileReader reader =
ParquetFileReader.open(ParquetIO.file(localInput(file)))) {
+ List<BlockMetaData> blocks = reader.getFooter().getBlocks();
+ assertThat(blocks).hasSize(3);
+ for (BlockMetaData block : blocks) {
+ assertThat(block.getRowCount()).isLessThanOrEqualTo(rowGroupRowLimit);
+ }
+ }
+ }
Review Comment:
Would it make sense to test that the new row count config has precedence
over the other row group size settings?
##########
parquet/src/test/java/org/apache/iceberg/parquet/TestParquet.java:
##########
@@ -114,6 +115,38 @@ public void testRowGroupSizeConfigurableWithWriter()
throws IOException {
}
}
+ @Test
+ public void rowGroupRowLimitConfigurable() throws IOException {
+ Schema schema = new Schema(optional(1, "intCol", IntegerType.get()));
Review Comment:
Instead of using a hard-coded "intCol" can you introduce a `NestedField`
variable to this test and use it below?
--
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]
