[GitHub] [arrow-datafusion] alamb commented on issue #4517: Make ConfigOption names into an Enum

[GitBox <gi...@apache.org>]

alamb commented on issue #4517:
URL: https://github.com/apache/arrow-datafusion/issues/4517#issuecomment-1338130834

   So I started bashing this out, with the incremental way like:
   
   ```rust
   
   /// DataFusion specific configuration names.
   pub enum ConfigName
   {
       /// Configuration option "datafusion.execution.target_partitions"
       TargetPartitions,
       /// Configuration option "datafusion.catalog.create_default_catalog_and_schema"
       CreateDefaultCatalogAndSchema,
       /// Configuration option "datafusion.catalog.information_schema"
       InformationSchema,
       /// Configuration option "datafusion.optimizer.repartition_joins"
       RepartitionJoins,
       /// Configuration option "datafusion.optimizer.repartition_aggregations"
       RepartitionAggregates,
   
       /// Configuration option "datafusion.optimizer.repartition_windows"
       RepartitionWindows,
   ...}
   ```
   
   But then I was thinking, if we are going to change the API anyways, why not go all the way with something fully statically typed and removing `ConfigDefinition` entirely:
   
   ```rust
   
   /// DataFusion specific configuration names.
   pub enum ConfigValue
   {
       /// Configuration option "datafusion.execution.target_partitions"
       TargetPartitions(usize),
   
       /// Configuration option for arbitrary user defined data
       UserDefined {
         name: String,
         value: Option<ScalarValue>
       },
   ...}
   
   impl ConfigValue {
     /// Return the name of this configuration value
     fn name(&self) -> &str {
       match self {
         Self::TargetPartitions(_) => "datafusion.execution.target_partitions",
         ...
       }
   
     /// Return the human readable description for this configuration value
     fn description(&self) -> &str {
       match self {
         Self::TargetPartitions(_) =>
                       "Number of partitions for query execution. Increasing partitions can increase \
                    concurrency. Defaults to the number of cpu cores on the system.",
   ...
     }
   
     /// set the value of the configuration value
    fn set_value(&mut self, new_value: ScalarValue) ->Result<()>{
        match (self, value) {
         (Self::TargetPartitions(v), ScalarValue::UInt64(Some(new_value))) => *v = new_value,
         (Self::TargetPartitions(v), _)  => return Err("Expected uint64 for {} but got {:?}", self.name(), new_value)).
   ...
   }
   ```
   
   But before I go crank that through the process I wanted to get some feedback if that was a desirable way to go. I would retain the ability to store arbitrary name/value pairs in the metadata. 
   
   What do you think @thinkharderdev @yahoNanJing @andygrove @avantgardnerio ?
   
   


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org