comphead commented on code in PR #6996:
URL: https://github.com/apache/datafusion-comet/pull/6996#discussion_r4361446150
##########
spark/src/main/scala/org/apache/comet/serde/operator/IcebergWriteProtoTranslation.scala:
##########
@@ -100,6 +100,9 @@ object IcebergWriteProtoTranslation {
}
}
+ /** True when the write compresses with brotli. Unlike `resolveCompression`,
never throws. */
+ def compressesWithBrotli(props: Map[String, String]): Boolean =
codecName(props) == "brotli"
Review Comment:
`compressesWithBrotli` compares the trimmed, lowercased `codecName` to
`"brotli"`. #7027 (open) rewrites the same code so that `codecName` no longer
trims or lowercases, to match iceberg-java's
`valueOf(name.toUpperCase(Locale.ENGLISH))`, and the two PRs conflict in this
file and in `iceberg.md`. If the conflict is resolved by taking #7027's
`codecName`, then `BROTLI` is accepted by the gate but fails the `== "brotli"`
check, which skips `requireLoadableBrotliCodec` and lets the native writer
commit files that cannot be read. Keeping this PR's trimming helper would
instead undo #7027's fix. Could brotli detection go through the resolved codec,
for example `name.toUpperCase(Locale.ENGLISH) == "BROTLI"`, and could a
detection case with `BROTLI` pin it? I did not build either merged result.
--
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]
