Resolve review-changes that dont require refactoring

Author: Laschoking

nemo-physical/src/resource.rs

@@ -1,7 +1,7 @@
 //! This modules defines the [Resource] type, which is part of the public interface of this crate.
 //!
 
-use core::fmt;
+use std::fmt;
 
 /// Resource that can be referenced in source declarations in Nemo programs
 /// Resources are resolved using `nemo::io::resource_providers::ResourceProviders`

nemo/src/io/format_builder.rs

@@ -243,14 +243,16 @@ pub(crate) trait FormatBuilder: Sized + Into<AnyImportExportBuilder> {
         tag: Self::Tag,
         parameters: &Parameters<Self>,
         direction: Direction,
-    ) -> Result<(Self, Option<ResourceSpec>), ValidationErrorKind>;
+    ) -> Result<Self, ValidationErrorKind>;
 
     fn expected_arity(&self) -> Option<usize>;
 
     fn supports_tag(tag: &str) -> bool {
         Self::Tag::from_str(tag).is_ok()
     }
-
+    fn override_resource_spec(&self, _direction: Direction) -> Option<ResourceSpec> {
+        None
+    }
     fn build_import(&self, arity: usize) -> Arc<dyn ImportHandler + Send + Sync + 'static>;
     fn build_export(&self, arity: usize) -> Arc<dyn ExportHandler + Send + Sync + 'static>;
 }
@@ -405,10 +407,6 @@ impl ImportExportBuilder {
                             Ok(resource) => Some(resource), // Success: return Some(ResourceSpec)
                             Err(kind) => {
                                 // Report the error and return None
-                                println!(
-                                    "throw error, resource not correct: {:?} {:?}",
-                                    origin, kind
-                                );
                                 builder.report_error(origin, kind);
                                 None
                             }
@@ -423,28 +421,22 @@ impl ImportExportBuilder {
             })
             .unwrap_or_default();
 
-        let (inner, new_res) = match B::new(tag, &parameters, direction) {
+        let inner = match B::new(tag, &parameters, direction) {
             Ok(res) => Some(res),
             Err(kind) => {
                 builder.report_error(origin, kind);
                 None
             }
         }?;
 
+        let new_resource = inner.override_resource_spec(direction);
+
         // Use the [ResourceSpec] from the Builder if available
-        if new_res.is_some() {
-            Some(ImportExportBuilder {
-                inner: inner.into(),
-                resource: new_res,
-                compression,
-            })
-        } else {
-            Some(ImportExportBuilder {
-                inner: inner.into(),
-                resource,
-                compression,
-            })
-        }
+        Some(ImportExportBuilder {
+            inner: inner.into(),
+            resource: new_resource.or(resource),
+            compression,
+        })
     }
 
     pub(crate) fn new(

nemo/src/io/formats/dsv.rs

@@ -31,9 +31,7 @@ use crate::{
     syntax::import_export::{attribute, file_format},
 };
 
-use super::{
-    ExportHandler, FileFormatMeta, FormatBuilder, ImportHandler, ResourceSpec, TableWriter,
-};
+use super::{ExportHandler, FileFormatMeta, FormatBuilder, ImportHandler, TableWriter};
 
 /// Implements [ImportHandler] and [ExportHandler] for delimiter-separated values.
 #[derive(Debug, Clone)]
@@ -200,10 +198,10 @@ impl FormatBuilder for DsvBuilder {
         tag: Self::Tag,
         parameters: &Parameters<DsvBuilder>,
         _direction: Direction,
-    ) -> Result<(Self, Option<ResourceSpec>), ValidationErrorKind> {
+    ) -> Result<Self, ValidationErrorKind> {
         let value_formats = parameters
             .get_optional(DsvParameter::Format)
-            .map(|value| DsvValueFormats::try_from(value).unwrap());
+            .map(|value| DsvValueFormats::try_from(value).expect("value formats have already been validated"));
 
         let limit = parameters
             .get_optional(DsvParameter::Limit)
@@ -226,15 +224,12 @@ impl FormatBuilder for DsvBuilder {
             .map(AnyDataValue::to_boolean_unchecked)
             .unwrap_or(false);
 
-        Ok((
-            Self {
-                delimiter,
-                value_formats,
-                limit,
-                ignore_headers,
-            },
-            None,
-        ))
+        Ok(Self {
+            delimiter,
+            value_formats,
+            limit,
+            ignore_headers,
+        })
     }
 
     fn expected_arity(&self) -> Option<usize> {

nemo/src/io/formats/json.rs

@@ -17,7 +17,7 @@ use crate::{
     syntax::import_export::file_format,
 };
 
-use super::{FileFormatMeta, ImportHandler, ResourceSpec};
+use super::{FileFormatMeta, ImportHandler};
 
 #[derive(Debug, Clone)]
 pub(crate) struct JsonHandler;
@@ -58,12 +58,12 @@ impl FormatBuilder for JsonHandler {
         _tag: Self::Tag,
         _parameters: &Parameters<Self>,
         direction: Direction,
-    ) -> Result<(Self, Option<ResourceSpec>), ValidationErrorKind> {
+    ) -> Result<Self, ValidationErrorKind> {
         if matches!(direction, Direction::Export) {
             return Err(ValidationErrorKind::UnsupportedJsonExport);
         }
 
-        Ok((Self, None))
+        Ok(Self)
     }
 
     fn expected_arity(&self) -> Option<usize> {

nemo/src/io/formats/rdf.rs

@@ -42,7 +42,7 @@ use crate::{
 };
 
 use super::FileFormatMeta;
-use super::{ExportHandler, FormatBuilder, ImportHandler, ResourceSpec, TableWriter};
+use super::{ExportHandler, FormatBuilder, ImportHandler, TableWriter};
 
 /// The different supported variants of the RDF format.
 #[derive(Assoc, Debug, Clone, Copy, PartialEq, Eq, VariantArray)]
@@ -198,7 +198,7 @@ impl FormatBuilder for RdfHandler {
         tag: Self::Tag,
         parameters: &Parameters<RdfHandler>,
         _direction: Direction,
-    ) -> Result<(Self, Option<ResourceSpec>), ValidationErrorKind> {
+    ) -> Result<Self, ValidationErrorKind> {
         let variant = match tag {
             RdfTag::Rdf => {
                 let value = parameters
@@ -243,15 +243,12 @@ impl FormatBuilder for RdfHandler {
             .as_ref()
             .map(AnyDataValue::to_u64_unchecked);
 
-        Ok((
-            Self {
-                base,
-                variant,
-                value_formats,
-                limit,
-            },
-            None,
-        ))
+        Ok(Self {
+            base,
+            variant,
+            value_formats,
+            limit,
+        })
     }
 
     fn expected_arity(&self) -> Option<usize> {

nemo/src/io/formats/sparql.rs

@@ -1,15 +1,11 @@
-//! Handler for resources of type Sparql (java script object notation).
+//! Handler for resources of type SPARQL (SPARQL query language for RDF)).
 
 use spargebra::Query;
 use std::sync::Arc;
 
 use nemo_physical::datavalues::{AnyDataValue, DataValue};
 use oxiri::Iri;
 
-//use reader::DsvReader;
-//use value_format::DsvValueFormats;
-//use writer::DsvWriter;
-
 use crate::{
     io::format_builder::{
         format_parameter, format_tag, value_type_matches, AnyImportExportBuilder, FormatParameter,
@@ -39,18 +35,12 @@ format_parameter! {
         Base(name = attribute::BASE, supported_types = &[ValueType::Constant]),
         Endpoint(name = attribute::ENDPOINT, supported_types = &[ValueType::Constant]),
         Query(name = attribute::QUERY, supported_types = &[ValueType::String]),
-        IgnoreHeaders(name = attribute::IGNORE_HEADERS, supported_types = &[ValueType::Boolean]),
-
     }
 }
 
 impl FormatParameter<SparqlTag> for SparqlParameter {
     fn required_for(&self, tag: SparqlTag) -> bool {
-        if matches!(tag, SparqlTag::Sparql) {
-            matches!(self, SparqlParameter::Endpoint) && matches!(self, SparqlParameter::Query)
-        } else {
-            false
-        }
+        matches!(tag, SparqlTag::Sparql) && matches!(self, Self::Endpoint | Self::Query)
     }
 
     fn is_value_valid(&self, value: AnyDataValue) -> Result<(), ValidationErrorKind> {
@@ -67,10 +57,9 @@ impl FormatParameter<SparqlTag> for SparqlParameter {
             SparqlParameter::Base => Ok(()),
             SparqlParameter::Format => DsvValueFormats::try_from(value).and(Ok(())).map_err(|_| {
                 ValidationErrorKind::ImportExportValueFormat {
-                    file_format: "rdf".to_string(),
+                    file_format: "sparql".to_string(),
                 }
             }),
-            SparqlParameter::IgnoreHeaders => Ok(()),
             SparqlParameter::Endpoint => value
                 .to_iri()
                 .ok_or(ValidationErrorKind::ImportExportInvalidIri)
@@ -79,27 +68,23 @@ impl FormatParameter<SparqlTag> for SparqlParameter {
                         .map(|_| ())
                         .map_err(|_| ValidationErrorKind::ImportExportInvalidIri)
                 }),
-            SparqlParameter::Query => value
-                .to_plain_string()
-                .ok_or(ValidationErrorKind::ImportExportLimitNegative)
-                .and_then(|query| {
-                    Query::parse(query.as_str(), None).map(|_| ()).map_err(|e| {
-                        ValidationErrorKind::ImportExportInvalidSparqlQuery {
-                            oxi_error: e.to_string(),
-                        }
+            SparqlParameter::Query => {
+                Query::parse(value.to_plain_string_unchecked().as_str(), None)
+                    .map(|_| ())
+                    .map_err(|e| ValidationErrorKind::ImportExportInvalidSparqlQuery {
+                        oxi_error: e.to_string(),
                     })
-                }),
+            }
         }
     }
 }
 
 #[derive(Clone)]
 pub(crate) struct SparqlBuilder {
     limit: Option<u64>,
-    delimiter: u8,
     value_formats: Option<DsvValueFormats>,
-    ignore_headers: bool,
-    parsed_query: Query,
+    endpoint: Iri<String>,
+    query: Query,
 }
 
 impl From<SparqlBuilder> for AnyImportExportBuilder {
@@ -114,52 +99,53 @@ impl FormatBuilder for SparqlBuilder {
         _tag: Self::Tag,
         parameters: &Parameters<SparqlBuilder>,
         _direction: Direction,
-    ) -> Result<(Self, Option<ResourceSpec>), ValidationErrorKind> {
+    ) -> Result<Self, ValidationErrorKind> {
         // Copied from DsvBuilder
         let value_formats = parameters
             .get_optional(SparqlParameter::Format)
-            .map(|value| DsvValueFormats::try_from(value).unwrap());
+            .map(|value| DsvValueFormats::try_from(value).expect("value formats have already been validated"));
 
         let limit = parameters
             .get_optional(SparqlParameter::Limit)
             .map(|value| value.to_u64_unchecked());
 
-        let ignore_headers = parameters
-            .get_optional(SparqlParameter::IgnoreHeaders)
-            .as_ref()
-            .map(AnyDataValue::to_boolean_unchecked)
-            .unwrap_or(false);
-
-        // Sparql-specific fields
+        // SPARQL-specific fields
         let endpoint = Iri::parse_unchecked(
             parameters
-                .get_required(SparqlParameter::Endpoint.into())
+                .get_required(SparqlParameter::Endpoint)
                 .to_iri_unchecked(),
         );
 
         let query = parameters
-            .get_required(SparqlParameter::Query.into())
+            .get_required(SparqlParameter::Query)
             .to_plain_string_unchecked();
 
-        let parsed_query = Query::parse(query.as_str(), None).unwrap();
+        let query = Query::parse(query.as_str(), None).expect("query has already been validated");
 
         // Create and return a new ResourceSpec to replace the existing ResourceSpec
-        let resource = ResourceSpec::from_endpoint(endpoint, query);
-        Ok((
-            Self {
-                delimiter: b'\t',
-                value_formats,
-                limit,
-                ignore_headers,
-                parsed_query,
-            },
-            Some(resource),
-        ))
+        Ok(Self {
+            value_formats,
+            limit,
+            endpoint,
+            query,
+        })
+    }
+
+    // TODO: do we need to parse parameters here again? that would be annoying
+    fn override_resource_spec(&self, direction: Direction) -> Option<ResourceSpec> {
+        if matches!(direction, Direction::Export) {
+            None
+        } else {
+            Some(ResourceSpec::from_endpoint(
+                self.endpoint.clone(),
+                self.query.to_sse(),
+            ))
+        }
     }
 
     fn expected_arity(&self) -> Option<usize> {
         let mut var_count: usize = 0;
-        match &self.parsed_query {
+        match &self.query {
             Query::Select { pattern, .. }
             | Query::Describe { pattern, .. }
             | Query::Construct { pattern, .. }
@@ -170,17 +156,17 @@ impl FormatBuilder for SparqlBuilder {
 
     fn build_import(&self, arity: usize) -> Arc<dyn ImportHandler + Send + Sync + 'static> {
         Arc::new(DsvHandler {
-            delimiter: self.delimiter,
+            delimiter: b'\t',
             value_formats: self
                 .value_formats
                 .clone()
                 .unwrap_or(DsvValueFormats::default(arity)),
             limit: self.limit,
-            ignore_headers: self.ignore_headers,
+            ignore_headers: true,
         })
     }
 
     fn build_export(&self, _arity: usize) -> Arc<dyn ExportHandler + Send + Sync + 'static> {
-        unimplemented!("Sparql export is currently not supported")
+        unimplemented!("SPARQL export is currently not supported")
     }
 }