vbhanuchander-lang commented on PR #28780:
URL: https://github.com/apache/flink/pull/28780#issuecomment-5837804314
@nikitasinghvi I built it this time rather than reading it, so I can replace
my earlier "looks right to me" with something checked. **The production change
is correct and does what you say.** One problem though: the test does not guard
it.
### The change is real
I counted the pages Parquet actually wrote, with your conf (4 KB page size,
`min = max = 1`) and 100 rows of a 40 KB binary payload:
| | pages written |
|---|---|
| your branch | **100** — one page per row, which is what `min = max = 1`
asks for |
| the same branch with your two `withMinRowCountForPageSizeCheck` /
`withMaxRowCountForPageSizeCheck` lines removed | **1** |
So the keys really are ignored on `master` and really are honoured with your
change. That is the behaviour worth pinning.
### The test passes without your change
`testRowCountForPageSizeCheckConfIsHonoured` passes on your branch **and**
with the two production lines deleted — I ran it both ways, 4/4 either time. It
contains no assertion: it writes 100 rows and calls `flush()`/`finish()`, and
the comment says "Should complete without OutOfMemoryError". 100 × 40 KB is
about 4 MB, so nothing approaches `Integer.MAX_VALUE` and it cannot OOM with
the defaults either. A reviewer reverting your production lines to check the
test earns its place would find it green.
I say this as someone who had the identical thing pointed out on one of my
own PRs recently, so it is a common shape rather than a criticism of the idea —
the test was pinning the writer's tolerance rather than your wiring.
### What fixed it for me
Assert the observable effect instead of the absence of a crash. Reading the
page count back out of the file gives 100 versus 1, so it fails on `master` and
passes with your change:
```java
private static int countPages(org.apache.hadoop.fs.Path file, Configuration
conf)
throws IOException {
int pages = 0;
try (ParquetFileReader reader =
ParquetFileReader.open(HadoopInputFile.fromPath(file, conf))) {
ColumnDescriptor column =
reader.getFileMetaData().getSchema().getColumns().get(0);
PageReadStore rowGroup;
while ((rowGroup = reader.readNextRowGroup()) != null) {
PageReader pageReader = rowGroup.getPageReader(column);
while (pageReader.readPage() != null) {
pages++;
}
}
}
return pages;
}
```
and then, after `writer.finish()`:
```java
assertThat(countPages(new org.apache.hadoop.fs.Path(path.toUri()), conf))
.as("with min=max=1 and a 4KB page size, each 40KB row should get
its own page")
.isGreaterThan(50);
```
`> 50` rather than `== 100` so it does not become brittle if the flush
heuristics change. Everything it needs is already on the test classpath. Feel
free to take it as is.
Two smaller things while I was in there: the test declares `@TempDir
java.nio.file.Path folder` inline as a fully-qualified name where an import
would do, and `DataFormatConverters` is deprecated in favour of
`DataStructureConverters` — neither matters much, and I would not hold the PR
for either.
### Routing
On your follow-up: I am not a committer, so I cannot merge or assign this,
and FLINK-40245 is still unassigned. @MartijnVisser, you were tagged here in
July — would you be able to point this at whoever owns `flink-parquet`, or
assign the ticket to Nikita? It is a two-line wiring fix with a real
behavioural difference behind it, it has been open since 20 July, and the one
thing it needs is a test that fails on `master`, which the snippet above
provides.
--
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]
