Jefffrey commented on PR #17697: URL: https://github.com/apache/datafusion/pull/17697#issuecomment-3319338919
> Upon reviewing the requirements and the example for `ExplainFormat` I identified several distinct cases: > > 1. **Parameters with Numeric/Boolean Types/Date formats or Free-Text Strings:** > No changes are needed for these parameters. > > 2. **Parameters with Existing Validation:** > The main consideration here is whether to standardize error formatting for consistency. I’ve left a few TODOs to revisit this. > > 3. **Parameters Similar to "ExplainFormat":** > These strings can be converted to enums, following a clear approach. I’ve already applied a similar change to "NullOrdering". > Determining the best location for enum declarations within the config can be challenging. Unless we establish a rule-of-thumb (e.g., placing each enum in a separate file), I’ll experiment further and propose a solution. > For existing enums (like "NullOrdering"), reusing them should be the priority. However, I’m unsure if it’s beneficial to move specific logic (such as the "nulls_first" method) closer to the config. > > 4. **Parameters Similar to "DurationFormat":** > These involve strings in the Datafusion config that map to enums from other crates (e.g., "datafusion.format.duration_format", "datafusion.sql_parser.dialect"). > Due to the orphan rule, I couldn’t implement the "Display" or "FromStr" traits directly. My current solution is to create wrapper types (named as DF, such as "DFDurationFormat"). I’m open to suggestions for a better naming convention. > > > @Jefffrey could you please have a look at the plan above and tell me what do you think? Thank you! For point 1 I wouldn't be so quick to dismiss numeric fields; we might need to put some checks on them (e.g. nonzero) for sanity. Although we could look into changing the type to [`NonZeroUsize`](https://doc.rust-lang.org/std/num/type.NonZeroUsize.html) for some of them and maybe that would enforce it automatically for us 🤔 Point 2 sounds good, but yeah it might need a follow-up ticket as I assume it might be quite a bit of work itself. Point 3 I don't think we want to go too extreme and have each enum in it's own file. For moving existing, perhaps it's better to leave them where they are for the moment 🤔 Point 4 I've only skimmed the code a bit so far, but that makes sense to me. -- 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]
