lgbo-ustc commented on PR #12571: URL: https://github.com/apache/gluten/pull/12571#issuecomment-5018703280
Additional clean-code feedback, excluding the unrelated logging change already discussed separately: 1. HiveSourceSinkFactory.addCompressionParamsFromTableProperties is doing two different jobs: copying compression-like table properties and deriving the native sink.file.compression config. These have different responsibilities and different compatibility constraints. Please split the behavior or narrow this method to only produce the native config that is actually needed. 2. isCompressionProperty is too broad. Matching any key that contains compress or codec makes the planner pass through unrelated or future table properties by accident. A clearer and safer approach is to maintain an explicit allow-list of supported Hive/Flink compression keys. 3. normalizeCompressionKind is too permissive. It uses substring matching and returns unknown normalized values as-is, which lets planner generate configs that native may reject later. It would be cleaner to explicitly map supported values and return empty/null, fallback, or reject for unsupported codecs. 4. The Hive writer factory reflection path is duplicated with the existing format-resolution path. Both depend on bucketsBuilder -> writerFactory -> HiveBulkWriterFactory -> factory. Please consider sharing one small helper so Flink internal field-name changes do not need to be fixed in multiple places. 5. The new unit tests are focused on helper implementation details rather than behavior. The test using orc.compress also makes the expected output look valid even though native filesystem compression currently does not support ORC. Please add behavior-oriented coverage for supported Parquet codecs and unsupported format/codec cases, verifying fallback/no config/error behavior explicitly. -- 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]
