vbhanuchander-lang commented on PR #28780:
URL: https://github.com/apache/flink/pull/28780#issuecomment-5526709132
@nikitasinghvi this looks right to me, and it reads as an omission rather
than a decision.
`FlinkParquetBuilder.createWriter` already wires seven conf values through
the same
`withX(getX(conf))` shape — `withRowGroupSize(getBlockSize(...))`,
`withPageSize`,
`withDictionaryPageSize`, `withDictionaryEncoding`, `withValidation`,
`withWriterVersion`,
`withCompressionCodec`. These two are the only `ParquetOutputFormat.get*`
accessors of that family
missing from the chain, so the change follows the file's own convention
rather than introducing one.
Checked what I could without building:
- `master`'s `ParquetRowDataBuilder` contains no reference to
`withMinRowCountForPageSizeCheck`,
`withMaxRowCountForPageSizeCheck`, or the two conf keys, so those keys are
genuinely ignored today.
- Both accessors and both builder methods exist with the signatures used
here —
`ParquetOutputFormat.getMinRowCountForPageSizeCheck(Configuration)` /
`getMaxRowCountForPageSizeCheck(Configuration)` returning `int`, and
`ParquetWriter.Builder.withMinRowCountForPageSizeCheck(int)` /
`withMaxRowCountForPageSizeCheck(int)` (verified against
`parquet-hadoop-1.16.0`).
**One thing I could not settle and would want a reviewer to confirm:** the
`.withConf(conf)` call
sits *after* the two new calls in the chain. For the seven existing values
that ordering is
evidently fine, so it very likely is here too, but it is the one way this
change could silently do
nothing — worth a reviewer's eye or an assertion in the test that the
configured value actually
reaches the writer, rather than only that the write succeeds.
@MartijnVisser you merged the most recent `flink-parquet` change (#28565) —
would you be willing to
take a look? This has been sitting since 28 July with no review, and it is
+52/-0 with a test.
--
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]
