Repository: apache/datafusion (language: rust)

Issue:
Make SqlToRel respect parser options from ContextProvider
`SqlToRel` provides two constructors: `new` and `new_with_options`. The former uses default `ParserOptions`, while the latter accepts `ParserOptions` as an input parameter.

On the other hand, `SqlToRel` always requires a `ContextProvider` parameter, and the `ContextProvider` trait offers an `ContextProvider::options` interface to retrieve `ConfigOptions`, which includes a `SqlParserOptions` with functionality equivalent to `ParserOptions`.

I believe we should modify `SqlToRel::new` to derive `ParserOptions` from `SqlParserOptions`, rather than relying on the default `ParserOptions`. This approach would allow us to consistently use a single set of parser options throughout the session's lifetime, rather than setting parser options for each query. Doing so would reduce redundancy and the risk of inconsistency, making the code less error-prone. (Actually, we have encountered an error due to inconsistency between the SessionConfig and the ParserOptions)

Possible implementation:
``` rust
impl From<SqlParserOptions> for ParserOptions { ... }

impl<'a, S: ContextProvider> SqlToRel<'a, S> {
    /// Create a new query planner
    pub fn new(context_provider: &'a S) -> Self {
        let sql_paser_options = context_provider.options().sql_parser;
        Self::new_with_options(context_provider, ParserOptions::from(sql_parser_options))
    }

    ...
}
```



Discussion / hints:
If this modification makes sense, I'd like to file a PR.

Fix patch:
diff --git a/datafusion/sql/src/planner.rs b/datafusion/sql/src/planner.rs
index 5fb6ef913d8c..68462aa9b69f 100644
--- a/datafusion/sql/src/planner.rs
+++ b/datafusion/sql/src/planner.rs
@@ -21,6 +21,7 @@ use std::sync::Arc;
 use std::vec;
 
 use arrow::datatypes::*;
+use datafusion_common::config::SqlParserOptions;
 use datafusion_common::error::add_possible_columns_to_diag;
 use datafusion_common::{
     field_not_found, internal_err, plan_datafusion_err, DFSchemaRef, Diagnostic,
@@ -62,6 +63,19 @@ impl Default for ParserOptions {
     }
 }
 
+impl From<&SqlParserOptions> for ParserOptions {
+    fn from(options: &SqlParserOptions) -> Self {
+        Self {
+            parse_float_as_decimal: options.parse_float_as_decimal,
+            enable_ident_normalization: options.enable_ident_normalization,
+            support_varchar_with_length: options.support_varchar_with_length,
+            enable_options_value_normalization: options
+                .enable_options_value_normalization,
+            collect_spans: options.collect_spans,
+        }
+    }
+}
+
 /// Ident Normalizer
 #[derive(Debug)]
 pub struct IdentNormalizer {
@@ -249,12 +263,18 @@ pub struct SqlToRel<'a, S: ContextProvider> {
 }
 
 impl<'a, S: ContextProvider> SqlToRel<'a, S> {
-    /// Create a new query planner
+    /// Create a new query planner.
+    ///
+    /// The query planner derives the parser options from the context provider.
     pub fn new(context_provider: &'a S) -> Self {
-        Self::new_with_options(context_provider, ParserOptions::default())
+        let parser_options = ParserOptions::from(&context_provider.options().sql_parser);
+        Self::new_with_options(context_provider, parser_options)
     }
 
-    /// Create a new query planner
+    /// Create a new query planner with the given parser options.
+    ///
+    /// The query planner ignores the parser options from the context provider
+    /// and uses the given parser options instead.
     pub fn new_with_options(context_provider: &'a S, options: ParserOptions) -> Self {
         let ident_normalize = options.enable_ident_normalization;
 


Test patch: