16pierre commented on PR #19928: URL: https://github.com/apache/datafusion/pull/19928#issuecomment-3800513817
> I'm not as familiar with the context here, but my 2c from reading the original issue is that this config is meant to be a temporary solution until a more proper fix is in place. Yes indeed this config is a bit of a quickfix, leaks low-level implementation, hopefully we find first-class solutions as a replacement soon. We should definitely try to minimize pollution footprint and optimize for code removal. > I wonder if we should be introducing a struct in that case, or if its easier for now to have a flat config that we can more easily remove in the future if we manage the more elegant fix? In the current implementation, the config remains flat on the core Parquet configuration structs. These flat configs are converted to a struct when wired to `PruningPredicate`. As mentioned [here](https://github.com/apache/datafusion/pull/19928#discussion_r2712153874), I don't have a strong opinion on whether to use struct vs a flat argument in these `PruningPredicate` codepaths, I'll defer to your best judgement. As the devil's advocate one could argue: - the maintenance overhead of the struct feels negligible, removing the struct will be just as fast as removing a flat type - the struct brings a slightly higher level of abstraction: this PR is already very leaky, but it felt even more awkward to propagate a flat `inListLimit: usize` in a bunch of APIs. This implementation detail argument feels like it doesn't belong at the same level as more important args, the config pollutes the APIs a bit less by hiding this flag behind an opaque `PredicateConfig` struct -- 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]
